Opened 7 months ago

Closed 6 months ago

#33520 closed defect (fixed)

scipy, networkx: Update install-requires.txt

Reported by: Matthias Köppe Owned by:
Priority: critical Milestone: sage-9.6
Component: build Keywords:
Cc: Antonio Rojas, Tobias Diez, François Bissey Merged in:
Authors: Matthias Koeppe Reviewers: Tobias Diez
Report Upstream: N/A Work issues:
Branch: 2abe0ad (Commits, GitHub, GitLab) Commit: 2abe0ad6527e8621490c427396a15da6cdc63a37
Dependencies: #33495 Stopgaps:

Status badges

Description

After #33336 and #33495, we should change the upper version bounds in build/pkgs/.../install-requires.txt

Change History (38)

comment:1 Changed 7 months ago by Matthias Köppe

Branch: u/mkoeppe/scipy__networkx__update_install_requires_txt

comment:2 Changed 7 months ago by Matthias Köppe

Authors: Matthias Koeppe
Commit: c572b0aa4951d34f31df49843fdfa1fe66a64a18
Status: newneeds_review

New commits:

c6dc11bUse EdgesView for matching output
66a1fe8Cast networkx.node_clique_number output to dict
2a7fa3cSpecify the input format to Graph
8452003Update tests and docs
3707210Merge #33495
73c2b45build/pkgs/networkx/install-requires.txt: Accept networkx 2.7.x
c572b0abuild/pkgs/scipy/install-requires.txt: Accept scipy 1.8.x

comment:3 Changed 7 months ago by Tobias Diez

Why are upper bounds needed for these packages? For example, networkx doesn't yet have a version 2.8 released (at least on pip) and I couldn't find a ticket that says that this version is indeed not compatible with the current version of sagelib.

comment:4 Changed 7 months ago by Matthias Köppe

It's part of best practices to set upper bounds so that incompatible releases of these third-party packages do not retroactively break our release.

comment:5 in reply to:  4 Changed 7 months ago by Tobias Diez

Status: needs_reviewneeds_work

Replying to mkoeppe:

It's part of best practices to set upper bounds so that incompatible releases of these third-party packages do not retroactively break our release.

Best practice is to only set a upper limit for versions that are known to cause incompatibilities. For example, have a look at:

Libraries/packages should be setting a floor, and if necessary excluding known buggy versions, but otherwise don't cap the maximum version as you can't predict future compatibility

(https://snarky.ca/why-i-dont-like-semver/, from one of the core members of the python team)

and https://iscinumpy.dev/post/bound-version-constraints/ for a very detailed analysis ending with the recommendation

Valid reasons to add an upper limit are:

  1. If a dependency is known to be broken, block out the broken version. ...
  2. If you know upstream is about to make a major change that is very likely to break your usage, you can cap. ... ...

comment:6 Changed 7 months ago by Matthias Köppe

For each of the updates of these packages from x.y to x.(y+1) we had tickets for fixing doctest failures that they introduced. So yes, we can predict future incompatibility. scipy and networkx are fast-moving active projects.

comment:7 Changed 7 months ago by Matthias Köppe

Note also our release cadence (https://wiki.sagemath.org/ReleaseTours) is on the order of months to half a year.

Which means when an incompatibility arises, we are not able to react quickly, and we will be months with broken dependencies.

comment:8 Changed 7 months ago by Matthias Köppe

Status: needs_workneeds_review

comment:9 Changed 7 months ago by Matthias Köppe

Cc: François Bissey added

comment:10 in reply to:  7 Changed 7 months ago by Tobias Diez

Reviewers: Tobias Diez

Replying to mkoeppe:

Note also our release cadence (https://wiki.sagemath.org/ReleaseTours) is on the order of months to half a year.

Which means when an incompatibility arises, we are not able to react quickly, and we will be months with broken dependencies.

That conversely means that people cannot upgrade networkx for a very long time although there might be no or only minor incompatibilities.

Big -1 from my side on upper limits that don't satisfy one of the criteria outlined in the linked articles.

comment:11 Changed 7 months ago by François Bissey

All those "requirements" are only indications for pip to manage dependencies on its own. If you are not using pip to manage your package (say apt, emerge, pacman...) those requirements are only indication for you as a packager, you can ignore them.

I usually pay attention to lower bounds, but the upper one is checked by running the testsuite if there is one, and comparing the result with the highest version that fits the upper limit.

I tend to agree that higher limits are bets about future development - unless you happen to have inside knowledge about upcoming breakage.

comment:12 in reply to:  11 Changed 7 months ago by Matthias Köppe

Replying to fbissey:

I usually pay attention to lower bounds, but the upper one is checked by running the testsuite if there is one, and comparing the result with the highest version that fits the upper limit.

Yes, input from you and other packagers of cutting-edge distributions can inform us about upper bounds that we need to set in install-requires.txt if we are not immediately able to adjust tests and code.

comment:13 Changed 7 months ago by Matthias Köppe

In addition to our experience with the upgrades of these packages over the years, what is relevant is the policy of the package regarding deprecations - for scipy that's https://scipy.github.io/devdocs/dev/core-dev/index.html#deprecations

scipy used the MAJOR.MINOR.MICRO versioning, and the policy https://scipy.github.io/devdocs/dev/core-dev/index.html#version-numbering advises that "Minor versions are typically released twice a year and can contain new features, deprecations and bug-fixes." (Micro version steps cannot make new deprecations.)

So such deprecation warnings are routinely introduced twice a year with the release of a new MINOR versions.

Sage users/developers traditionally value spotless doctest runs (see typical sage-devel posts); the conservative policy of excluding an increase of the MINOR version (scipy<1.9, as per this ticket - as our current version is 1.7.x and compatibility with 1.8.x has been taken care of in #33336) protects against such deprecation warnings.

Of course, it would be valid to argue that this is too conservative. A more relaxed policy would consider that within 6 months a hard incompatibility can only be introduced when the next MINOR version is released; so this would justify setting scipy<1.10.

But what is the right policy is for the Sage community to decide and cannot be resolved by a generic appeal to (selected) authority.

comment:14 Changed 7 months ago by Matthias Köppe

Guidance on upper bounds for numpy is in https://numpy.org/devdocs/user/depending_on_numpy.html#runtime-dependency-version-ranges (we currently do not set an upper bound).

comment:16 Changed 6 months ago by Matthias Köppe

comment:17 Changed 6 months ago by Matthias Köppe

Priority: majorcritical

Bumping this up so we can get this fix into 9.6.

comment:18 Changed 6 months ago by Tobias Diez

I don't see how these last comments of yours address any point that is brought forward in comment:11 above and in https://iscinumpy.dev/post/bound-version-constraints/.

Moreover, they actually are arguments for why these upper bounds are not a good idea:

Note that setting an upper bound on NumPy? may affect the ability of your library to be installed alongside other, newer packages.

And

you can set an upper bound of <MAJOR.MINOR + N with N no less than 3, and MAJOR.MINOR being the current release of NumPy?

In this ticket N=1.

comment:19 Changed 6 months ago by Matthias Köppe

So which N do you think we should use?

comment:20 Changed 6 months ago by François Bissey

From the packaging perspective in Gentoo, most packages only include lower limits. Upper limits, when they exist, are added after the fact in 99.9% of the cases (notable exceptions would be stuff like pynac which was known to break things at every single release). While I don't want to impose my views on this on you and don't feel as strongly as Tobias, upper limits are mostly just a guess and assuming them means that you should be ready to update them frequently.

Part of the reasons I don't care that much about the limits on this ticket, is that I know they exist and rarely bother reading them :)

comment:21 Changed 6 months ago by Matthias Köppe

This makes sense for Gentoo, which is a fast moving cutting edge distribution.

comment:22 Changed 6 months ago by Matthias Köppe

We are talking here about the metadata that comes with releases of Sage (= releases about 2-3 times a year, with no bugfix releases ever). The idea is to provide each release with upper bounds for its key dependencies that ensures that this version can be used within its lifespan.

comment:23 Changed 6 months ago by François Bissey

I do understand your intent, even if fundamentally living in Gentoo land for about 20 years makes me think it is misguided. It is something I associate with proprietary software. Which of course sagemath is a competition to. I just always feel I and Tobias and Antonio are just the wrong persons to help you make those kind of decisions - our investment in that aspect is low.

comment:24 in reply to:  19 ; Changed 6 months ago by Tobias Diez

Replying to mkoeppe:

So which N do you think we should use?

Infinity?

The numpy docs explicitly have a frequent release cycle as a requirement for an upper version limit. The reason for this is also explained in the the second link cited above:

This means every single major release of every dependency you cap immediately requires you to make a new release or everyone using your library can no longer use the latest version of those libraries. If “make a new release quickly” from above bothered you; well, now you have to make it on every version bump of every pinned dependency.

And sage at it current dev speed cannot guarantee such quick releases.

For example, what if scipy 1.9 contains an urgent hotfix? Then users of sage + scipy cannot upgrade their scipy for a few months just because we were guessing that scipy 1.9 might break a couple of doctests.

comment:25 in reply to:  24 ; Changed 6 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

So which N do you think we should use?

Infinity?

That's an extreme position which puts 0 emphasis on stability.

For example, what if scipy 1.9 contains an urgent hotfix?

First of all, scipy has just only started the 1.8 series and will continue making 1.8.x fixes.

Then users of sage + scipy cannot upgrade their scipy for a few months just because we were guessing that scipy 1.9 might break a couple of doctests.

That's an argument that supports setting N higher, but it does not support N=infinity.

comment:26 in reply to:  25 ; Changed 6 months ago by Tobias Diez

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Replying to mkoeppe:

So which N do you think we should use?

Infinity?

That's an extreme position which puts 0 emphasis on stability.

Not at all. It's very easy (through mildly annoying) for *users* to set upper limits for these dependencies in their installation as they see fit; potentially trading new shiny bug fixes of scipy with some issues in sage's usage of scipy. But that tradeoff depends on the needs of the particular use case (maybe she doesn't need anything new from scipy vs she doesn't care that half of sage is broken with a new version of scipy as long as the other half works). Artificial upper version limits just remove this flexibility.

Something like N=4 seems also okay, although I don't really see the benefit of this.

comment:27 in reply to:  26 Changed 6 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

It's very easy (through mildly annoying) for *users* to set upper limits for these dependencies

This asymmetry between setting limits (easy) and removing limits (hard) is, of course, an important argument in favor of looser bounds; but it still does not support N=infinity.

comment:28 Changed 6 months ago by Matthias Köppe

Status: needs_reviewneeds_work

I'll prepare a version based on the deprecation policies linked to in comment:14 to comment:16

comment:29 in reply to:  28 Changed 6 months ago by Tobias Diez

Replying to mkoeppe:

I'll prepare a version based on the deprecation policies linked to in comment:14 to comment:16

Thanks!

Note that just because something is deprecated (and later removed) doesn't mean that sage is affected by it. Thus one doesn't have to strictly follow the deprecation timeline, although it makes sense to take them as orientation.

comment:30 Changed 6 months ago by git

Commit: c572b0aa4951d34f31df49843fdfa1fe66a64a18ce77eb75158be12f565a601f56c1af590f4b5f34

Branch pushed to git repo; I updated commit sha1. New commits:

ce77eb7build/pkgs/networkx/install-requires.txt: Use <3.0

comment:31 Changed 6 months ago by git

Commit: ce77eb75158be12f565a601f56c1af590f4b5f34145a226ad8162159ee9256561e32e0686fd9d8ae

Branch pushed to git repo; I updated commit sha1. New commits:

145a226build/pkgs/scipy/install-requires.txt: Use <1.11

comment:32 Changed 6 months ago by Matthias Köppe

Status: needs_workneeds_review

comment:33 Changed 6 months ago by Tobias Diez

Thanks for the changes.

Shouldn't it be < 1.12 for scipy? Some feature might be deprecated in 1.9, then it stays deprecated for half a year (= 2 minor releases, i.e. 1.10 and 1.11) before it is potentially removed in 1.12.

Moreover, I think it would be a good idea to distill our discussion and add a remark to the dev docs that upper bounds should be used carefully and need to be documented.

comment:34 in reply to:  33 Changed 6 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

Shouldn't it be < 1.12 for scipy? Some feature might be deprecated in 1.9, then it stays deprecated for half a year (= 2 minor releases, i.e. 1.10 and 1.11) before it is potentially removed in 1.12.

no, half a year is one minor release.

comment:35 Changed 6 months ago by git

Commit: 145a226ad8162159ee9256561e32e0686fd9d8ae2abe0ad6527e8621490c427396a15da6cdc63a37

Branch pushed to git repo; I updated commit sha1. New commits:

2abe0adsrc/doc/en/developer/packaging.rst: Point to #33520

comment:36 Changed 6 months ago by Tobias Diez

Okay, looks good to me.

Feel free to set it to positive review once https://github.com/sagemath/sagetrac-mirror/runs/5710585148?check_suite_focus=true passes.

comment:37 Changed 6 months ago by Matthias Köppe

Status: needs_reviewpositive_review

Thanks!

comment:38 Changed 6 months ago by Volker Braun

Branch: u/mkoeppe/scipy__networkx__update_install_requires_txt2abe0ad6527e8621490c427396a15da6cdc63a37
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.