Skip to content

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented May 1, 2023

📚 Description

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 2, 2023

Our upper bound on setuptools conflicted with cvxpy, which for unknown reasons has setuptools in install-requires (see cvxpy/cvxpy#2118)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 2, 2023

Still lots of conflicts, involving narrowly restricted version of boost-cpp etc.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 3, 2023

@tobiasdiez do you think we can have the "s: run conda ci" label also run the workflow on PR syncs?

@tobiasdiez tobiasdiez added s: run conda ci Run the conda workflow on this PR. and removed s: run conda ci Run the conda workflow on this PR. labels May 3, 2023
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2023

@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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 27, 2023

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
@isuruf Not sure what's wrong here, or how to ask the resolver for hints what we should be doing differently.

Copy link
Contributor

@tobiasdiez tobiasdiez left a 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.


- Run the ``configure`` script::

$ ./bootstrap
$ ./configure --with-python=$CONDA_PREFIX/bin/python \
--prefix=$CONDA_PREFIX \
--enable-system-site-packages \
Copy link
Contributor

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.

Copy link
Contributor Author

@mkoeppe mkoeppe Sep 28, 2023

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.)

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 28, 2023

I propose you split this PR in multiple small ones with a single purpose to make reviewing easier.

I don't think so. The changes are needed to restore the workflow from broken to working.

@dimpase
Copy link
Member

dimpase commented Sep 28, 2023

can we only run ./bootstrap-conda on conda?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 28, 2023

No. Its output files are part of the configure tarball.

@dimpase
Copy link
Member

dimpase commented Sep 28, 2023

there should be an option to bootstrap to build this tarball. We run thus mostly useless bunch of stuff a lot, during routine rebuilds.

@orlitzky
Copy link
Contributor

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,

  1. Install all sage dependencies into a conda environment
  2. Install sage normally, but pointing ./configure to the conda environment

Where, in my head at least, that second phase still uses a venv.

@dimpase
Copy link
Member

dimpase commented Sep 29, 2023

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 --enable-system-site-packages available. But that's for another ticket.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2023

I must be out of the loop. How do we avoid the virtualenv when using conda to provide only the dependencies for sage?

By telling users not to run make. That's the experimental method in section 3 ("Using conda to provide all dependencies") of https://doc.sagemath.org/html/en/installation/conda.html

I thought the process was, at a high level,

  1. Install all sage dependencies into a conda environment
  2. Install sage normally, but pointing ./configure to the conda environment

Where, in my head at least, that second phase still uses a venv.

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2023

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 --enable-system-site-packages available.

This method is what we have been talking about here on the PR. I have added --enable-system-site-packages here (because it makes sense). It doesn't simplify anything, but it brings what ./configure says closer to what is actually going to happen. (./configure still says "... will be installed from SPKG" for a few packages that don't have spkg-configure or are not available on conda -- but we instruct the user to never type "make", so nothing will be installed from SPKG.)

@orlitzky
Copy link
Contributor

Ah, thanks. Now that I know that make is missing, I can see that it's missing.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2023

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2023

I've reduced the scope of the PR; it now no longer makes changes to the installation manual.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 30, 2023

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 --enable-system-site-packages available.

This method is what we have been talking about here on the PR. I have added --enable-system-site-packages here (because it makes sense). It doesn't simplify anything, but it brings what ./configure says closer to what is actually going to happen. (./configure still says "... will be installed from SPKG" for a few packages that don't have spkg-configure or are not available on conda -- but we instruct the user to never type "make", so nothing will be installed from SPKG.)

In #36367 I simplify the installation instructions by moving the run of configure inside the installation of the sage-conf package.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
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
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
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
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
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
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 25, 2023

Still some rough edges:

  /usr/share/miniconda3/envs/sage/bin/x86_64-conda-linux-gnu-cc -shared -Wl,--allow-shlib-undefined -Wl,-rpath,/usr/share/miniconda3/envs/sage/lib -Wl,-rpath-link,/usr/share/miniconda3/envs/sage/lib -L/usr/share/miniconda3/envs/sage/lib -Wl,--allow-shlib-undefined -Wl,-rpath,/usr/share/miniconda3/envs/sage/lib -Wl,-rpath-link,/usr/share/miniconda3/envs/sage/lib -L/usr/share/miniconda3/envs/sage/lib -Wl,-O2 -Wl,--sort-common -Wl,--as-needed -Wl,-z,relro -Wl,-z,now -Wl,--disable-new-dtags -Wl,--gc-sections -Wl,--allow-shlib-undefined -Wl,-rpath,/usr/share/miniconda3/envs/sage/lib -Wl,-rpath-link,/usr/share/miniconda3/envs/sage/lib -L/usr/share/miniconda3/envs/sage/lib -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /usr/share/miniconda3/envs/sage/include -DNDEBUG -D_FORTIFY_SOURCE=2 -O2 -isystem /usr/share/miniconda3/envs/sage/include /tmp/tmpi4cbpdyd.build-temp/sage/graphs/chrompoly.o -lgmp -o /tmp/tmpjgan9ucn.build-lib/sage/graphs/chrompoly.cpython-310-x86_64-linux-gnu.so -lpari
  sage/graphs/cliquer.c:1217:10: fatal error: cliquer/graph.h: No such file or directory
   1217 | #include "cliquer/graph.h"
        |          ^~~~~~~~~~~~~~~~~
  compilation terminated.

@@ -131,6 +169,7 @@ echo >&2 $0:$LINENO: generate conda environment files
(
echo >&4 " - pip:"
echo >&5 " - pip:"
echo >&6 " - pip:"
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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"
Copy link
Contributor

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'.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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'
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 27, 2023

I would also suggest a bit more restraint when relabeling as "needs work". None of your comments made identified a clear work item.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 22, 2023

Let's get this in.

Copy link

github-actions bot commented Apr 1, 2024

Documentation preview for this PR (built with commit a5e4c8f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimize conda environment
5 participants