Add conda dev environment
tobiasdiez opened this issue · 77 comments
We add a conda enviroment that is meant for development, i.e. includes tools that one probably needs while working on sage development such as linters, ssh (for contributing to trac) etc.
Moreover, we extract the conda-related things from docs/bootstrap to a new bootstrap-conda script in order to simplify the conda workflow (see updated documentation).
Github workflow run: https://github.com/sagemath/sagetrac-mirror/actions/runs/2268985510
CC: @isuruf @dimpase @saraedum @mkoeppe @slel
Component: build
Author: Tobias Diez
Branch: 87f360c
Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/33740
This looks quite odd to me.
Why can't the yml file in the branch be generated from what's already known about Conda packages?
I agree, not a good idea to treat this file different from all other of our files that are generated at bootstrap time.
Also jupyter_sphinx should really be added by adding a distros/ file, not ad hoc like this.
Replying to @dimpase:
This looks quite odd to me.
Why can't the yml file in the branch be generated from what's already known about Conda packages?
The issue is not so much to generate the file but how it is used. Running bootstrap requires a few dependencies which however are only specified in the environment file. So its a classical chicken-egg problem.
Currently this is resolved by requiring the developers to first create a conda env with the necessary tools for bootstrap, and then later update the env with all dependencies from the env file. With my proposal the overhead occurs only when dependencies are changed (which then requires to commit the changed env file). Overall this should be less work and more convenient.
I agree, not a good idea to treat this file different from all other of our files that are generated at bootstrap time.
The dev environment is special in this respect, since its the only file that you would possibly like to use before the bootstrap.
Branch pushed to git repo; I updated commit sha1. New commits:
aa144e6 | Add distros/conda for jupyter_sphinx |
Replying to @mkoeppe:
Also
jupyter_sphinxshould really be added by adding adistros/file, not ad hoc like this.
Done.
ok, but please make the initial egg (say, ./bootstrap-conda-dev) minimal - apart from m4 and other autotools, not much else is needed for ./bootstrap.
Mhh, having a conda environment just for the bootstrap would also work. But then one still has to run conda create sage-dev -f bootstrap-conda-dev.yml, bootstrap and conda sage-dev update -f environment.yml.
Having one conda environment config containing all necessary tools for bootstrap, compiling and running sage as well as a few additional dev tools is the most convenient, don't you think so?
Replying to @dimpase:
ok, but please make the initial egg (say,
./bootstrap-conda-dev) minimal - apart from m4 and other autotools, not much else is needed for./bootstrap.
+1 on just splitting the generation of the yml files from src/doc/bootstrap (it doesn't really belong there anyway) into a new script. It won't need any of the dependencies that the full bootstrap has.
perhaps this should rather become a conda-forge package?
Branch pushed to git repo; I updated commit sha1. New commits:
2811559 | Introduce new bootstrap-conda command |
Replying to @mkoeppe:
Replying to @dimpase:
ok, but please make the initial egg (say,
./bootstrap-conda-dev) minimal - apart from m4 and other autotools, not much else is needed for./bootstrap.+1 on just splitting the generation of the yml files from
src/doc/bootstrap(it doesn't really belong there anyway) into a new script. It won't need any of the dependencies that the fullbootstraphas.
Like this?
that's much nicer, but where is the top-level invocation of bootstrap-conda?
Do you mean it's an alternative to ./bootstrap ?
I wasn't sure about this one. The procedure now looks as follows:
bootstrap-condaconda createbootstrapconfigure- ...
So there is no point in invoking bootstrap-conda during bootstrap.
Still need to invoke this so that the generated files end up in the sdist.
(See the shell function save in bootstrap)
Replying to @tobiasdiez:
I wasn't sure about this one. The procedure now looks as follows:
bootstrap-condaconda createbootstrapconfigure- ...
So there is no point in invoking
bootstrap-condaduringbootstrap.
I thought that ./bootstrap recognises whether we're on conda, or not.
If so, it can invoke bootstrap-conda+conda create.
Otherwise, I'd like to see ./bootstrap getting an optional argument, conda,
to do the calling of bootstrap-conda+conda create and then proceeding.
Replying to @dimpase:
I thought that
./bootstraprecognises whether we're on conda, or not.
Not a good idea. The output of bootstrap must not depend on the system configuration. It is packed into the sdist.
shouldn't ./bootstrap-conda run on #!/bin/sh, just like ./bootstrap ?
Replying to @dimpase:
shouldn't
./bootstrap-condarun on#!/bin/sh, just like./bootstrap?
With sh the script is not working, so I took bash as in doc/bootstrap. If that needs to be changed, I need help as its beyond my basic shell script knowledge.
Replying to @dimpase:
Otherwise, I'd like to see
./bootstrapgetting an optional argument,conda,
to do the calling ofbootstrap-conda+conda createand then proceeding.
I like the idea to further simplify the workflow, but lets keep this ticket small and do further streamlining efforts in #33765.
Replying to @mkoeppe:
Still need to invoke this so that the generated files end up in the sdist.
Okay, added.
Branch pushed to git repo; I updated commit sha1. New commits:
42fc241 | Merge branch 'develop' into public/build/conda-dev-env |
Description changed:
---
+++
@@ -1,8 +1,9 @@
We add a conda enviroment that is meant for development, i.e. includes tools that one probably needs while working on sage development such as linters, ssh (for contributing to trac) etc.
-Moreover, we commit the auto-generated file in order to simplify the conda workflow as follows:
+Moreover, we extract the conda-related things from `docs/bootstrap` to a new `bootstrap-conda` script in order to simplify the conda workflow as follows:
- install mamba/conda
- git checkout
+- bootstrap-conda
- mamba create -n sage-dev -f src/environment-dev.yml
- conda activate sage-dev
- ./bootstrapReplying to @tobiasdiez:
Replying to @dimpase:
shouldn't
./bootstrap-condarun on#!/bin/sh, just like./bootstrap?With
shthe script is not working, so I tookbashas indoc/bootstrap. If that needs to be changed, I need help as its beyond my basic shell script knowledge.
did you try http://manpages.ubuntu.com/manpages/bionic/man1/checkbashisms.1.html ?
In this change:
- $ conda create -n sage-build
+ $ conda env create --file environment.yml
$ conda activate sage-build
I would suggest to keep the -n so that it's easy to see what needs to be changed if the user wants to use a different name
The instructions in "Using conda to provide all dependencies for the Sage library (experimental)" are missing ./bootstrap
Also ci-conda.yml should be updated so it tests the updated instructions
Replying to @dimpase:
Replying to @tobiasdiez:
Replying to @dimpase:
shouldn't
./bootstrap-condarun on#!/bin/sh, just like./bootstrap?With
shthe script is not working, so I tookbashas indoc/bootstrap. If that needs to be changed, I need help as its beyond my basic shell script knowledge.did you try http://manpages.ubuntu.com/manpages/bionic/man1/checkbashisms.1.html ?
It finds things of the form
possible bashism in ./bootstrap-conda line 25 (should be VAR="${VAR}foo"):
SYSTEM_PACKAGES+=" $PKG_SYSTEM_PACKAGES"
I would say the += construction is more readable.
Replying to @mkoeppe:
Also
ci-conda.ymlshould be updated so it tests the updated instructions
Good point, done.
https://github.com/sagemath/sagetrac-mirror/actions/runs/2258733820
Replying to @mkoeppe:
The instructions in "Using conda to provide all dependencies for the Sage library (experimental)" are missing
./bootstrap
Done.
Replying to @tobiasdiez:
Replying to @dimpase:
Replying to @tobiasdiez:
Replying to @dimpase:
shouldn't
./bootstrap-condarun on#!/bin/sh, just like./bootstrap?With
shthe script is not working, so I tookbashas indoc/bootstrap. If that needs to be changed, I need help as its beyond my basic shell script knowledge.did you try http://manpages.ubuntu.com/manpages/bionic/man1/checkbashisms.1.html ?
It finds things of the form
possible bashism in ./bootstrap-conda line 25 (should be VAR="${VAR}foo"): SYSTEM_PACKAGES+=" $PKG_SYSTEM_PACKAGES"I would say the
+=construction is more readable.
but why do you think that /bin/bash is available everwhere? E.g. on macOS it is probably not the case.
I took the bash thing from the original bootstrap script https://github.com/sagemath/sagetrac-mirror/blob/develop/src/doc/bootstrap. But to be honest, I didn't have an idea what I was doing and just mixed different things from the bootstrap scripts until it was working. So I'm totally fine to change it to a more suitable version.
git grep '#!.*bash' will show the variant that is in general use
Replying to @tobiasdiez:
Replying to @mkoeppe:
Also
ci-conda.ymlshould be updated so it tests the updated instructionsGood point, done.
https://github.com/sagemath/sagetrac-mirror/actions/runs/2258733820
It fails. I don't think the two pip command lines can be combined.
Because of --no-build-isolation, the Sage library (src) cannot advertise its build dependency, so pip cannot possibly know in which order it has to install stuff.
Author: Tobias Diez
Branch pushed to git repo; I updated commit sha1. New commits:
0cf7334 | Add sage-setup as install-requires dep |
Absolutely no, this is not an install-requires.
Branch pushed to git repo; I updated commit sha1. New commits:
c1b2fbf | Revert install_requires change |
Branch pushed to git repo; I updated commit sha1. New commits:
ad56ba3 | Go back to two pip installs for now |
Replying to @mkoeppe:
Replying to @tobiasdiez:
Replying to @mkoeppe:
Also
ci-conda.ymlshould be updated so it tests the updated instructionsGood point, done.
https://github.com/sagemath/sagetrac-mirror/actions/runs/2258733820It fails. I don't think the two pip command lines can be combined.
Because of
--no-build-isolation, the Sage library (src) cannot advertise its build dependency, sopipcannot possibly know in which order it has to install stuff.
Build isolation shouldn't be an issue since this only decides if pip tries to create a temporary venv or reuses the existing one for the build env, but not what build dependencies to be installed. Could it be that there is some misconfig of the build dependencies?
Anyway, I've reverted this part and would leave it to a new ticket.
Description changed:
---
+++
@@ -9,3 +9,5 @@
- ./bootstrap
- ./configure -withsomething
- pip install --no-build-isolation -v -v --editable pkgs/sage-conf pkgs/sage-setup src
+
+Github workflow run: https://github.com/sagemath/sagetrac-mirror/actions/runs/2268985510Replying to @tobiasdiez:
Build isolation shouldn't be an issue
https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-no-build-isolation
"Disable isolation when building a modern source distribution. Build dependencies specified by PEP 518 must be already installed if this option is used."
comment:29
Also, in the edits of the documentation, the information on how to select a Python version have been lost
Replying to @mkoeppe:
Replying to @tobiasdiez:
Build isolation shouldn't be an issue
https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-no-build-isolation
"Disable isolation when building a modern source distribution. Build dependencies specified by PEP 518 must be already installed if this option is used."
Ahh good to know, then I've remembered it wrong. Thanks for the clarification.
Replying to @mkoeppe:
In this change:
- $ conda create -n sage-build + $ conda env create --file environment.yml $ conda activate sage-buildI would suggest to keep the
-nso that it's easy to see what needs to be changed if the user wants to use a different name
Normally the user doesn't need to change the name, and if he does its easy to find on the conda docs how to do this. Thus I don't really see a point in adding an arg that is the same as the default value.
Branch pushed to git repo; I updated commit sha1. New commits:
b42f5f5 | Readd how to use specific python version |
Replying to @mkoeppe:
Also, in the edits of the documentation, the information on how to select a Python version have been lost
Added again.
Replying to @tobiasdiez:
Replying to @mkoeppe:
In this change:
- $ conda create -n sage-build + $ conda env create --file environment.yml $ conda activate sage-buildI would suggest to keep the
-nso that it's easy to see what needs to be changed if the user wants to use a different nameNormally the user doesn't need to change the name
It's as normal as having a second installation in a separate directory
Replying to @mkoeppe:
Replying to @tobiasdiez:
Replying to @mkoeppe:
In this change:
- $ conda create -n sage-build + $ conda env create --file environment.yml $ conda activate sage-buildI would suggest to keep the
-nso that it's easy to see what needs to be changed if the user wants to use a different nameNormally the user doesn't need to change the name
It's as normal as having a second installation in a separate directory
If you want to install two sages in parallel, you should also be able to find the --name arg of conda. But if you insist I can readd the name.
Yes please. I added it on purpose in the original version.
Branch pushed to git repo; I updated commit sha1. New commits:
87f360c | Readd name arg |
Replying to @mkoeppe:
Yes please. I added it on purpose in the original version.
okay done
Broken: https://github.com/sagemath/sagetrac-mirror/runs/6356314150
conda's pari is not being accepted.
This is not specific to this branch and also occurs for develop https://github.com/sagemath/sage/runs/6236167749?check_suite_focus=true
I've opened #33828 for this.
Reviewer: Matthias Koeppe
Thanks for checking, I agree. Then it's good to go
Thanks!
Description changed:
---
+++
@@ -1,13 +1,5 @@
We add a conda enviroment that is meant for development, i.e. includes tools that one probably needs while working on sage development such as linters, ssh (for contributing to trac) etc.
-Moreover, we extract the conda-related things from `docs/bootstrap` to a new `bootstrap-conda` script in order to simplify the conda workflow as follows:
-- install mamba/conda
-- git checkout
-- bootstrap-conda
-- mamba create -n sage-dev -f src/environment-dev.yml
-- conda activate sage-dev
-- ./bootstrap
-- ./configure -withsomething
-- pip install --no-build-isolation -v -v --editable pkgs/sage-conf pkgs/sage-setup src
+Moreover, we extract the conda-related things from `docs/bootstrap` to a new `bootstrap-conda` script in order to simplify the conda workflow (see updated documentation).
Github workflow run: https://github.com/sagemath/sagetrac-mirror/actions/runs/2268985510Changed branch from public/build/conda-dev-env to 87f360c