Skip to content
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

Fix conda install instructions after setuptools update #36411

Merged
merged 5 commits into from
Oct 21, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Oct 5, 2023

📝 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

@tobiasdiez
Copy link
Contributor

There are now build errors.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 6, 2023

possibly from Cython 3.0.3, which was installed in the conda environment. I'll test that version separately in #36416

@mkoeppe mkoeppe changed the title Fix conda install instructions after setuptools update Fix conda install instructions after setuptools update, pin cython to 3.0.2 Oct 6, 2023
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 6, 2023

Yes, that did the trick.

Remaining error in pip check: "sagemath-standard 10.2b5 has requirement ipython<8.9.0,>=7.13.0, but you have ipython 8.16.1."

@@ -1 +1 @@
cython
cython==3.0.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to exclude the broken 3.0.3 instead of fixing the version (here and in install-requires)? From what I read, the bug should be fixed in 3.0.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we have to expect further breakage in the 3.0.x series

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any basis for this expectation? Looking at #36110, there was no problem upgrading from 3.0.0 to 0.1 and 0.2.

I think we can always exclude newer versions once they are known to break something...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can always exclude newer versions once they are known to break something...

I think I have explained this to you several times in the past already. This type of breakage affects releases that we have already made retroactively. As our project does not make bug fix releases, we cannot fix such breakage after it happens.

So when our experience indicates that we have to expect breakage from updates, it is prudent to put in suitable version constraints.

The breakage with 3.0.3 affected a major project (scipy), so this is an indication that the Cython project is currently struggling with release management. This is understandable because after the rollout of Cython 3 in distributions, there is likely very high demand for developer attention as lots of projects are forced to add support for Cython 3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when our experience indicates that we have to expect breakage from updates, it is prudent to put in suitable version constraints.

Yes, that's why you put in the major version constraint for cython. Bugs in other projects can always happen, but that's not a valid reason to fix the version. (A better solution in the form of a lock file is waiting for review: #35986)

For example, if we release sage with this version constraint, then it is no longer possible to use it in a conda environment with a newer version of cython. But someone might need 3.0.4 because earlier versions break of them.

So please loosen the constraint again. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not a valid reason to fix the version.

Sorry, Tobias, you don't get to declare that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By that logic you would need to introduce a lot more fixed version constraints for a lot more packages. A lot of our dependencies have weaker ci than cython and thus may introduce new bugs.

For me these version constraints say "we expect that sage is compatible with these versions". Since that range usual contains future releases, it implicitly has the addition "of course, only given that the dependency doesn't introduce breaking changes/bugs".

From what I can see the cython issue that affects us is likely fixed in 3.0.4 (it already has patches discussed etc). So until you have evidence that there might be more bugs that affect us, I have the expectation that 3.0.4 works.

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 helpful to repeat the same points every time that a version constraint has to be set. I'm fully aware of any and all of these points, and they are already factored in.

Copy link
Contributor

@tobiasdiez tobiasdiez Oct 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fully aware of any and all of these points, and they are already factored in.

Perfect! In that case you might want to point out where our evaluation differs, since apparently we come to a different end result. For example, you might want to start with discussing why we didn't needed a fixed version before but now we do. For all I can see the cython 3 gets more stable and stable, so there are less and less bugs expected. By this logic, we would need a fixed version less and less.

Would the following compromise work for you: we simply exclude 3.0.3 for now, and if 3.0.4 breaks things again, we introduce an upper version constraint <= 3.0.2?

@tobiasdiez

This comment was marked as off-topic.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 9, 2023

Tobias, you misunderstand. I've marked the thread as resolved because there were no useful points in it, nor do I expect to learn anything new from it if were to continue. It's all empty calories.

@tobiasdiez
Copy link
Contributor

An alternative is suggested in #35986 (ready for review), where the cython lock is present in the environment files that users/devs will us to generate the conda env, but not in the conda.txt. I think this is a nice compromise.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 10, 2023

I don't see the conda lock as a solution for this problem. It can be nice for creating stability for deployments on the application level, such as our gitpod or devcontainers.
But it's hardly a replacement for the thoughtful use of version ranges.

@mkoeppe mkoeppe requested a review from isuruf October 10, 2023 05:18
@dimpase
Copy link
Member

dimpase commented Oct 11, 2023

I'm trying to run "Using conda to provide all dependencies for the Sage library (experimental)" on this branch, and get

$ pip install --no-build-isolation --config-settings editable_mode=compat -v -v --editable ./src
...
  adding 'sage_conf-9.8.dist-info/top_level.txt'
  adding 'sage_conf-9.8.dist-info/RECORD'
  removing build/bdist.linux-x86_64/wheel
  Building wheel for sage-conf (pyproject.toml) ... done
  Created wheel for sage-conf: filename=sage_conf-9.8-py3-none-any.whl size=5929 sha256=2452cdf180ceebae3ac425dc4be670ed835ba9e14dabadcca092846ec7d3003b
  Stored in directory: /home/scratch/dimpase/.cache/pip/wheels/42/8d/24/ee2834b08081c68f0473dced1cfa0c801641dffdf6648cb811
Successfully built sage-conf
Failed to build sagemath-standard
ERROR: Could not build wheels for sagemath-standard, which is required to install pyproject.toml-based projects
Exception information:
Traceback (most recent call last):
  File "/home/scratch/scratch2/dimpase/mambaforge/envs/sage-dev/lib/python3.11/site-packages/pip/_internal/cli/base_command.py", line 180, in exc_logging_wrapper
    status = run_func(*args)
             ^^^^^^^^^^^^^^^
  File "/home/scratch/scratch2/dimpase/mambaforge/envs/sage-dev/lib/python3.11/site-packages/pip/_internal/cli/req_command.py", line 248, in wrapper
    return func(self, options, args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/scratch/scratch2/dimpase/mambaforge/envs/sage-dev/lib/python3.11/site-packages/pip/_internal/commands/install.py", line 429, in run
    raise InstallationError(
pip._internal.exceptions.InstallationError: Could not build wheels for sagemath-standard, which is required to install pyproject.toml-based projects
Remote version of pip: 23.2.1
Local version of pip:  23.2.1
Was pip installed by pip? False

Any idea what it could be?

I don't know whether it's a regression or not, though.

@dimpase
Copy link
Member

dimpase commented Oct 11, 2023

it appears that one needs to run ./bootstrap, not just ./bootstrap-conda, to set everything up properly (aftre which the error above went away)

So this part of the doc should be fixed.

@tobiasdiez
Copy link
Contributor

Did you run pip install --no-build-isolation -v -v --editable ./pkgs/sage-conf_conda ./pkgs/sage-setup before this? This should call bootstrap internally (#36367)

@dimpase
Copy link
Member

dimpase commented Oct 11, 2023

Did you run pip install --no-build-isolation -v -v --editable ./pkgs/sage-conf_conda ./pkgs/sage-setup before this? This should call bootstrap internally (#36367)

I did run

$ ./bootstrap-conda
$ mamba activate foo_env
(foo_env) $ pip install --no-build-isolation -v -v --editable ./pkgs/sage-conf_conda ./pkgs/sage-setup
(foo_env) $ pip install --no-build-isolation --config-settings editable_mode=compat -v -v --editable ./src

I don't see where ./bootstrap is called in sage/pkgs/sage-conf_conda/setup.py

I see it calls ./configure.

But honestly I think it's insane that pip would call ./bootstrap.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 11, 2023

I don't see where ./bootstrap is called in sage/pkgs/sage-conf_conda/setup.py

You are right, that was a mistake in #36367. I'll fix it here.

@dimpase
Copy link
Member

dimpase commented Oct 11, 2023

There seems to be one more bug/feature in setup.py: configure is not run if config.status is present.

And if you have a stale config.status lying around, then there is a good chance you get a misconfiguration. Not sure how to resolve this in general, but here removing it first of all is a workaround..

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 11, 2023

You are right, it should probably at least run ./config.status --recheck && ./config.status in this case, and maybe check that prefix is configured to be the same as the conda prefix

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2023

This PR is introducing an additional test failure in interpreter.py.

Link to the failure please. There is no such failure in the Build & Test of this very PR.

@tobiasdiez
Copy link
Contributor

tobiasdiez commented Oct 12, 2023

e.g. https://github.com/sagemath/sage/actions/runs/6451909843/job/17624459499?pr=36431. I've opened #36453 for the cython version fix, so we can see if it's really related to the other changes of this PR here, or if its some updated dependency causing it.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2023

You mean this?

sage -t --random-seed=219809728447624716937891191463986234770 src/sage/repl/interpreter.py
**********************************************************************
File "src/sage/repl/interpreter.py", line 77, in sage.repl.interpreter
Failed example:
    print("dummy line"); shell.run_cell('1/0') # see #25320 for the reason of the `...` and the dummy line in this test
Expected:
    dummy line
    ...
    ZeroDivisionError...Traceback (most recent call last)
    ...
    ----> 1 Integer(1)/Integer(0)
    .../sage/rings/integer.pyx... in sage.rings.integer.Integer...div...
    ...
    -> ...                  raise ZeroDivisionError("rational division by zero")
       ...            x = <Rational> Rational.__new__(Rational)
       ...            mpq_div_zz(x.value, ....value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
Got:
    dummy line
    ---------------------------------------------------------------------------
    ZeroDivisionError                         Traceback (most recent call last)
    Cell In[1], line 1
    ----> 1 Integer(1)/Integer(0)
    <BLANKLINE>
    File sage/rings/integer.pyx:2016, in sage.rings.integer.Integer.__truediv__()
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
*********************************************************************

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2023

The ipython version bound set here on this PR branch is the only thing that could be relevant for this doctest

@tobiasdiez
Copy link
Contributor

Yes, that's the one. #36453 is working.

@dimpase
Copy link
Member

dimpase commented Oct 12, 2023

It's a warning of sorts ("haha, just overwrote your config!")

please write warnings as warnings, not in this kind of haha-speak

@dimpase
Copy link
Member

dimpase commented Oct 12, 2023

"re-using" might mean different things, e.g. not ignoring rather than ignoring the existing settings

@dimpase
Copy link
Member

dimpase commented Oct 12, 2023

how about using the approach of #36453, i.e. not pinning a concrete version of Cython, but restricting the range and excluding 3.0.2, here?

@mkoeppe mkoeppe force-pushed the conda_legacy_editable branch from 6b33bd7 to d0cae80 Compare October 12, 2023 16:13
@mkoeppe mkoeppe changed the title Fix conda install instructions after setuptools update, pin cython to 3.0.2 Fix conda install instructions after setuptools update Oct 12, 2023
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2023

I've narrowed the scope of the PR, removing the change to the ipython (#36411 (comment)) and cython version constraints, and have removed the message.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2023

how about using the approach of #36453

See previous discussion here on this PR on this topic. To be continued on #36453.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2023

Thanks

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 15, 2023
…pdate

    
<!-- ^^^^^
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 -->
- Updates install instructions for use with modern `setuptools`
versions.
- Restore `bootstrap` step in instructions, lost in sagemath#36367
- Unpin `setuptools`.
<!-- 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.
- [ ] 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#36411
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Matthias Köppe, Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 17, 2023
…pdate

    
<!-- ^^^^^
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 -->
- Updates install instructions for use with modern `setuptools`
versions.
- Restore `bootstrap` step in instructions, lost in sagemath#36367
- Unpin `setuptools`.
<!-- 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.
- [ ] 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#36411
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Matthias Köppe, Tobias Diez
@vbraun vbraun merged commit 1071779 into sagemath:develop Oct 21, 2023
21 of 31 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 21, 2023
@mkoeppe mkoeppe deleted the conda_legacy_editable branch November 26, 2023 00:53
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.

4 participants