-
-
Notifications
You must be signed in to change notification settings - Fork 654
Add needed pip packages to src/environment.yml
, minimize conda environments
#35593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Our upper bound on |
Still lots of conflicts, involving narrowly restricted version of |
@tobiasdiez do you think we can have the "s: run conda ci" label also run the workflow on PR syncs? |
This is now part of #35380 |
Solves and package installs take pretty long - on the CI runners about 30 mins; see https://github.com/sagemath/sage/actions/runs/6331256567?pr=35593 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose you split this PR in multiple small ones with a single purpose to make reviewing easier.
src/doc/en/installation/conda.rst
Outdated
|
||
- Run the ``configure`` script:: | ||
|
||
$ ./bootstrap | ||
$ ./configure --with-python=$CONDA_PREFIX/bin/python \ | ||
--prefix=$CONDA_PREFIX \ | ||
--enable-system-site-packages \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed / what is it doing in this context? Also since the word 'system' package might be misleading for conda, and the list of additional commands is getting rather big, I propose you introduce a new toggle "with-conda=$CONDA_PREFIX" to configure that takes care of all this config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is it doing in this context?
It's unfortunately not documented yet (@orlitzky, am I right about this?), and is as experimental as this conda installation method.
Here I am using it so that the version constraints of those Python packages with spkg-configure are checked. Extra checks = good.
How long this command is does not really matter -- it is to be copy-pasted anyway. In any case, inventing new switches for conda is beyond the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it also changing how the venv is setup/used?
Here I am using it so that the version constraints of those Python packages with spkg-configure are checked. Extra checks = good.
I don't see the point in that. We have ci checks already in place and the lock files I proposed would make it even more unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it also changing how the venv is setup/used?
Note that in these instructions, no venv is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An important point of running the configure
script with the "force" options is to check that the installed versions are suitable for Sage. It is of course just a partial check because it is only done for those package for which we have the spkg-configure.m4. Now thanks to @orlitzky's work, we can check more, so here I'm adding it. It's a natural improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration step won't go away. This has nothing to do with using or not using the Sage build system. (We aren't using it in this mode, as you know.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning 1), I'm right in the assumption that this is not relevant here as we don't create a venv when used with conda.
I can provide a better answer to this now.
Right: it's not directly relevant to the conda installation. But it does provide a sanity check on the compatibility of the versions used by conda and sage. In the long run, that will help conda.
We are currently adding spkg-configure.m4
files for all of sage's python packages, testing the version bounds in install-requires.txt
, adding compatibility patches, etc. When all that is done, and when enough people have tested it, my hope is that we can turn --enable-system-site-packages
on by default. If that happens, using conda to install dependencies for sage becomes much simpler because you don't have to circumvent make
and the venv.
For that to work smoothly, the conda versions all have to be acceptable to ./configure
; otherwise, ./configure
will either crash or want to install an SPKG. Adding --enable-system-site-packages
now, while not having any direct effect, does flag those incompatibilities to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using conda to install dependencies for sage becomes much simpler because you don't have to circumvent make
It's actually a feature and not a bug that the conda install doesn't use the make interface. One of the longterm goals is to replace sages package management and build infrastructure with conda. Right now this is already working very well for python packages. You could remove every python package (but keep the auto generated conda env and setup.cfg files) and everything is still working. That's why I think this flag here is a step backwards (for the conda install, I'm sure it has other good usages).
I would propose we remove this flag from the docs for now, but keep it in the ci workflow. Then you do get the consistency checks, just that not everyone has to run them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anyone is proposing to add the use of make
to the all-conda installation method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was also not my point...
I don't think so. The changes are needed to restore the workflow from broken to working. |
can we only run |
No. Its output files are part of the configure tarball. |
there should be an option to bootstrap to build this tarball. We run thus mostly useless bunch of stuff a lot, during routine rebuilds. |
I must be out of the loop. How do we avoid the virtualenv when using conda to provide only the dependencies for sage? I thought the process was, at a high level,
Where, in my head at least, that second phase still uses a venv. |
I presume this https://doc.sagemath.org/html/en/installation/conda.html#using-conda-to-provide-all-dependencies-for-the-sage-library-experimental can be simplified now, with |
By telling users not to run
Yes, that's the method of section 2 ("Using conda to provide system packages for the Sage distribution") of https://doc.sagemath.org/html/en/installation/conda.html |
This method is what we have been talking about here on the PR. I have added |
Ah, thanks. Now that I know that |
I'm more concerned about the long solve/install time of the environment (see current run https://github.com/sagemath/sage/actions/runs/6358819359?pr=35593). Any help there would be very welcome. |
I've reduced the scope of the PR; it now no longer makes changes to the installation manual. |
In #36367 I simplify the installation instructions by moving the run of |
sagemathgh-36514: `build/pkgs/sagenb_export`: Fix install-requires.txt <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> Just one commit split out from sagemath#35593. <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36514 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik
sagemathgh-36513: `networkx`, `scipy`, `ipywidgets`: Update version ranges in `conda.txt` Just one commit split out from sagemath#35593. <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36513 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría, Matthias Köppe, Tobias Diez
sagemathgh-36514: `build/pkgs/sagenb_export`: Fix install-requires.txt <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> Just one commit split out from sagemath#35593. <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36514 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik
Still some rough edges:
|
@@ -131,6 +169,7 @@ echo >&2 $0:$LINENO: generate conda environment files | |||
( | |||
echo >&4 " - pip:" | |||
echo >&5 " - pip:" | |||
echo >&6 " - pip:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard environment file doesn't contain a pip
section by design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if that's by design (whose?), then that's a severe bug in it, and this is the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not there to check that all standard packages are installed via conda. If anything is a bug, then it's the presence of the pip section for optional and dev environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not there to check that all standard packages are installed via conda.
No, it is not being checked by anything. sagenb_export
is a standard package, and so far it has just been omitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bigger flaw is related to what we explained to you over in #36765 (comment):
Creating conda packages is a downstream activity. If we were to take the design idea that "all standard packages need to have a conda package" seriously, it would mean that we could never add a standard package in our capacity as the upstream project.
But neither is this current practice (neither with conda nor with any other downstream package repository), nor has such a change of policy been established or even discussed anywhere. And certainly this change cannot come into existence by gaslighting PR authors.
|
||
for PKG_BASE in $(sage-package list --has-file distros/conda.txt --exclude _sagemath); do | ||
PKG_SCRIPTS=build/pkgs/$PKG_BASE | ||
SYSTEM_PACKAGES_FILE=$PKG_SCRIPTS/distros/conda.txt | ||
PKG_TYPE=$(cat $PKG_SCRIPTS/type) | ||
PKG_SYSTEM_PACKAGES=$(echo $(${STRIP_COMMENTS} $SYSTEM_PACKAGES_FILE)) | ||
if [ -n "$PKG_SYSTEM_PACKAGES" ]; then | ||
case "$PKG_SYSTEM_PACKAGES" in | ||
*([-A-Za-z0-9_ ])) # just a package name | ||
UNCONSTRAINED_PACKAGES+=" $PKG_BASE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a fragile design to use "unconstrained version" as an indicator for including or excluding a certain dependency in the environment. It would prevent us from adding a version constraint to non-direct dependencies (although I have to admit I don't even understand the purpose of having conda.txt files for such dependencies) and forces us to specify a version constraint for those packages that do should end up.
Going with Python's 'explicit is better than implicit' slogan, I think it would be better to have a certain indicator file that says 'I'm a direct dependency, so include me please'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would prevent us from adding a version constraint to non-direct dependencies
To the contrary. It does the right thing -- namely including the dependency with the version constraint in the environment file -- for the non-direct dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
design to use "unconstrained version" as an indicator for including or excluding a certain dependency in the environment.
That's not what it does. Packages are excluded by the minimizer from the environment file when they are implied by dependence. But we never exclude packages that have their own version constraints, so that the version constraint is not lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need for any of the version constraints you added in this PR (intrinsic need, as in "if you install an older version that it's known to fail"). Thus you use version constraints as a a signal to include the given package in the environment file. I think this is not a good use of version constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is an "intrinsic need" for these version constraints; they are the same version bounds that configure
checks, each with good reasons.
It's just that "extrinsically" we have so far gotten away with not setting them -- simply because there is nothing that would force an older unsuitable version. But when users mix our environment with other conda packages, we can run into trouble. (And no, it's not better to wait for the bug report.)
Yes, I reuse this mechanism here for another purpose -- so that not another mechanism needs to be invented.
@@ -29,8 +29,8 @@ def run(self): | |||
'See https://doc.sagemath.org/html/en/installation/conda.html on how to get started.') | |||
|
|||
cmd = f"cd {SAGE_ROOT} && ./configure --enable-build-as-root --with-system-python3=force --disable-notebook --disable-sagelib --disable-sage_conf --disable-doc" | |||
cmd += ' --with-python=$CONDA_PREFIX/bin/python --prefix="$CONDA_PREFIX"' | |||
cmd += ' $(for pkg in $(PATH="build/bin:$PATH" build/bin/sage-package list :standard: --exclude rpy2 --has-file spkg-configure.m4 --has-file distros/conda.txt); do echo --with-system-$pkg=force; done)' | |||
cmd += ' --with-python=$CONDA_PREFIX/bin/python --prefix="$CONDA_PREFIX" --enable-system-site-packages' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it's to early for that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just re-read the comments in the thread above from 2 months ago, I don't think there's anything that supports delaying it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still "experimental" right? Given that the conda workflow is not as stable right now as we want it to be, I don't think we should add any non-stable elements to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building the Sage distribution with --enable-system-site-packages
is experimental.
Running the configure
script with it is safe.
I would also suggest a bit more restraint when relabeling as "needs work". None of your comments made identified a clear work item. |
Let's get this in. |
Documentation preview for this PR (built with commit a5e4c8f; changes) is ready! 🎉 |
… be eliminated by the bootstrap-conda environment minimizer
📚 Description
pip
section tosrc/environment.yml
for the missing standard packagesagenb_export
(this was left out inbootstrap-conda
: Refactor, generate versioned environment files #36405)pip
section is also good for adding the new pip package in Update to new conway-polynomials python package #36765--enable-system-site-packages
(this was left out in Simplify experimental all-conda installation instructions viapkgs/sage-conf_conda
#36367 (review)) and to avoid--with-system-SPKG=force
for packages that may be omitted by the environment minimizer.src/environment-optional.yml
📝 Checklist
⌛ Dependencies
sage --package list
: Sort output, add switches--{in,ex}clude-dependencies
#36393bootstrap-conda
: Refactor, generate versioned environment files #36405setuptools
update #36411build/pkgs/setuptools_scm
: Update to 8.0.4, add fixes for version 8 #36400networkx
,scipy
,ipywidgets
: Update version ranges inconda.txt
#36513 (split out from here)build/pkgs/sagenb_export
: Fix install-requires.txt #36514 (split out from here)