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

scipy, networkx: Update install-requires.txt #33520

Closed
mkoeppe opened this issue Mar 17, 2022 · 41 comments
Closed

scipy, networkx: Update install-requires.txt #33520

mkoeppe opened this issue Mar 17, 2022 · 41 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Mar 17, 2022

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

Depends on #33495

CC: @antonio-rojas @tobiasdiez @kiwifb

Component: build

Author: Matthias Koeppe

Branch/Commit: 2abe0ad

Reviewer: Tobias Diez

Issue created by migration from https://trac.sagemath.org/ticket/33520

@mkoeppe mkoeppe added this to the sage-9.6 milestone Mar 17, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2022

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2022

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2022

Commit: c572b0a

@tobiasdiez
Copy link
Contributor

comment:3

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2022

comment:4

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.

@tobiasdiez
Copy link
Contributor

comment:5

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2022

comment:6

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 17, 2022

comment:7

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.

@tobiasdiez
Copy link
Contributor

comment:10

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.

@tobiasdiez
Copy link
Contributor

Reviewer: Tobias Diez

@kiwifb
Copy link
Member

kiwifb commented Mar 18, 2022

comment:11

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2022

comment:12

Replying to @kiwifb:

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2022

comment:13

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 19, 2022

comment:14

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 19, 2022

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 22, 2022

comment:16

And here is the networkx deprecation policy.
https://networkx.org/documentation/stable/developer/deprecations.html#policy

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 22, 2022

comment:17

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

@tobiasdiez
Copy link
Contributor

comment:18

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.

@kiwifb
Copy link
Member

kiwifb commented Mar 22, 2022

comment:20

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 22, 2022

comment:21

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 22, 2022

comment:22

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.

@kiwifb
Copy link
Member

kiwifb commented Mar 22, 2022

comment:23

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.

@tobiasdiez
Copy link
Contributor

comment:24

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 22, 2022

comment:25

Replying to @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.

@tobiasdiez
Copy link
Contributor

comment:26

Replying to @mkoeppe:

Replying to @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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 22, 2022

comment:27

Replying to @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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 22, 2022

comment:28

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

@tobiasdiez
Copy link
Contributor

comment:29

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2022

Changed commit from c572b0a to ce77eb7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2022

Changed commit from ce77eb7 to 145a226

@tobiasdiez
Copy link
Contributor

comment:33

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 27, 2022

comment:34

Replying to @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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

Changed commit from 145a226 to 2abe0ad

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

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

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

@tobiasdiez
Copy link
Contributor

comment:36

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 30, 2022

comment:37

Thanks!

@vbraun
Copy link
Member

vbraun commented Apr 2, 2022

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

No branches or pull requests

4 participants