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

Add build/pkgs/SPKG/install-requires.txt for all Python packages, remove some unneeded packages #30719

Closed
mkoeppe opened this issue Oct 4, 2020 · 42 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 4, 2020

These files:

CC: @tobiasdiez @isuruf @kiwifb @antonio-rojas @dimpase

Component: build

Author: Matthias Koeppe

Branch/Commit: 6ec00dd

Reviewer: Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-9.2 milestone Oct 4, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2020

comment:2

Tobias, before I add more of these, do you think this format is useful for generating other files?


New commits:

a754e8cAdd some build/pkgs/*/install-requires.txt

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2020

Commit: a754e8c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2020

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

11ce883Add more build/pkgs/*/install-requires.txt

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2020

Changed commit from a754e8c to 11ce883

@tobiasdiez
Copy link
Contributor

comment:4

So the idea would be to concatenate all these install-requires.txt files to obtain the install_requires section of src/setup.cfg? That should work. Except that cython is a more a compile time dependency ([build-system] requires = ["cython"] in pyproject.toml), right?

I have to admit though that I don't see the benefit of having all these text files over having a single src/setup.cfg (or pyproject for that matter). It feels like it's the same information, just in a different location.
In fact, it feels a bit like you are reinventing the wheel. pyproject etc also have support for different kinds of dependencies, e.g. optional, compile time (cython), test (pytest) and build dependencies (tox).

Maybe that's a bit too naïve, but I would approach this as follows: sagelib (everything in /src) is a python library and as such only declares minimum dependencies and their requirements, trying not to pin versions (except if it's really necessary). I think, the most modern way would be to do this via a pyproject.toml file.
Then sage-the-application (the actual thing used by the enduser) is a python application and thus includes sagelib as a library and needs to satisfy its dependencies (this is the point where you have to specify in some way the exact version you want to use).
sage-the library should be completely independent of sage-the-application (i.e. it should work to have them in different git repositories).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2020

comment:5

Replying to @tobiasdiez:

So the idea would be to concatenate all these install-requires.txt files to obtain the install_requires section of src/setup.cfg?

Yes.

That should work. Except that cython is a more a compile time dependency ([build-system] requires = ["cython"] in pyproject.toml), right?

Right; but sagelib also exposes cython at runtime for interactive use.

I have to admit though that I don't see the benefit of having all these text files over having a single src/setup.cfg (or pyproject for that matter). It feels like it's the same information, just in a different location.

With modularization, I expect to see dozens of setup.cfg files for the various subset distributions. Basically, I don't want to duplicate version ranges between these.

In fact, it feels a bit like you are reinventing the wheel. pyproject etc also have support for different kinds of dependencies, e.g. optional, compile time (cython), test (pytest) and build dependencies (tox).

What I want to do is define version ranges in one place for all of the Sage project.

Distinguishing the kinds of dependencies (build, test, ...) should be orthogonal -- and the mechanism in #29041 should be able to put these version ranges into whatever file/format is needed.

(I don't think it would add sufficient value to Sage to have different version ranges for a given package in different roles, like "we need Cython >= 0.29.21 for build, but any Cython >= 0.27 is fine for runtime.)

sage-the library should be completely independent of sage-the-application (i.e. it should work to have them in different git repositories).

I don't like to go into this direction because it would add a big hurdle to our developer community. Having one git repository is key.

@tobiasdiez
Copy link
Contributor

comment:6

Replying to @mkoeppe:

I have to admit though that I don't see the benefit of having all these text files over having a single src/setup.cfg (or pyproject for that matter). It feels like it's the same information, just in a different location.

With modularization, I expect to see dozens of setup.cfg files for the various subset distributions. Basically, I don't want to duplicate version ranges between these.

Sounds like a valid point.

Having one git repository is key.

I agree! What I meant is that it might be helpful to approach it like this, i.e. how would you solve this problem if sage-library would be sitting in a different git repo? I think, this will be helpful e.g for distribution as pipy-installable package where it would be helpful if src/ works independently of the make and build scripts.

One more remark: I would try to make the version range specifications as general as possible, i.e. only add restrictions if its known that that this particular version doesn't work with sage (this in particular applies to the upper limit). Otherwise the sage-library (and its subdistributions) would be harder to use (e.g. if one cannot upgrade/downgrade a dependency because other code relies on it). In this context, it may actually turn out to be helpful if you could could control the dependency versions for each submodule sepereatly, e.g. if ModuleA breaks with a newer version but ModuleB doesn't have these problems.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2020

comment:7

Replying to @tobiasdiez:

What I meant is that it might be helpful to approach it like this, i.e. how would you solve this problem if sage-library would be sitting in a different git repo? I think, this will be helpful e.g for distribution as pipy-installable package where it would be helpful if src/ works independently of the make and build scripts.

Sounds like we are basically in agreement here. One detail, however, is the bootstrapping phase. The way I see it, after running ./bootstrap, we would have standard Python package source trees (which will not depend on make and build) -- right now, src or build/pkgs/sagelib/src/, but many more after modularization (such as build/pkgs/sage-ntl/src/). By using scripts that are executed during bootstrapping, we can avoid duplicating a lot of information and thus reduce the maintenance burden.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2020

comment:8

Replying to @tobiasdiez:

I would try to make the version range specifications as general as possible, i.e. only add restrictions if its known that that this particular version doesn't work with sage (this in particular applies to the upper limit). Otherwise the sage-library (and its subdistributions) would be harder to use (e.g. if one cannot upgrade/downgrade a dependency because other code relies on it).

Yes, this is tricky to get right and will probably need some detailed discussion for individual packages. But I do think that some upper bounds on the version are always needed because we do not want our package to stop working if a new major version of some package is released on pypi. For example, Cython has an upcoming 3.x version which we have not tested with at all.

In this context, it may actually turn out to be helpful if you could could control the dependency versions for each submodule separately, e.g. if ModuleA breaks with a newer version but ModuleB doesn't have these problems.

Yes, this is conceivable but I would hope that if this ever becomes necessary, it can be implemented by overriding the default rather than defaulting to duplicate maintenance of the version ranges.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2020

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

85b2dcffor a in *; do if [ -d $a -a ! -r $a/requirements.txt -a ! -r $a/install-requires.txt ]; then if grep -q -i sdh_pip $a/spkg-install.in >&/dev/null ; then ( if [ -r $a/package-version.txt ]; then echo "$a >=$(sed s/[.]p[0-9]$// $a/package-version.txt)"; else echo "$a"; fi) > $a/install-requires.txt && git add $a/install-requires.txt; fi; fi; done
f9ac70eAdd install-requires.txt for setuptools, pip

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2020

Changed commit from 11ce883 to f9ac70e

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2020

comment:10

Autogenerated for now, setting current version as lower bound

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

comment:12

I'm hoping to gather version information for our Python packages here.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

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

f6a2c56Merge tag '9.3.beta1' into t/30719/add_build_pkgs_spkg_install_requires_txt_for_all_python_packages

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

Changed commit from f9ac70e to f6a2c56

@kiwifb
Copy link
Member

kiwifb commented Nov 11, 2020

comment:17

That's a lot of packages to parse :(

Some stuff I can notice from a cursory inspection. I use sphinx 3.2.1 so the restriction <3.2 seem useless. Similarly I currently use networkx 2.5. python_openid was a sagenb dependency, I don't think we need it anymore. I certainly do not have it anymore on my system. Same with itsdangerous.

@tobiasdiez
Copy link
Contributor

comment:18

The pipenv.lock file added in #30371 is a good indication, too.

The <1.0 constraint for cython seems to be not necessary (at least they are very far off from version 1.0).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

Changed commit from f6a2c56 to ba1d913

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

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

e61929dbuild/pkgs/sphinx/install-requires.txt: Update from gentoo
8fe0e35build/pkgs/python_openid: Unused, remove
ba1d913build/pkgs/itsdangerous: Unused, remove

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 11, 2020

comment:20

Replying to @tobiasdiez:

The <1.0 constraint for cython seems to be not necessary (at least they are very far off from version 1.0).

From comment 8:
For example, Cython has an upcoming 3.x version which we have not tested with at all.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

Changed commit from ba1d913 to bb6c4ae

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

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

bb6c4aebuild/pkgs/tox/install-requires.txt: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

Changed commit from bb6c4ae to d507501

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

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

d507501build/pkgs/networkx/install-requires.txt: Update from gentoo

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

comment:23

See also: Upgrade ticket #30611

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

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

82d0df3build/pkgs/p_group_cohomology/install-requires.txt: Not on PyPI, remove
d5e9840build/pkgs/pathpy/install-requires.txt: Package removed in #30611, remove

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

Changed commit from d507501 to d5e9840

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c3a9352build/pkgs/pathpy/install-requires.txt: Package removed in #30611, remove

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

Changed commit from d5e9840 to c3a9352

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

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

ebd4610build/pkgs/pynac/install-requires.txt: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

Changed commit from c3a9352 to ebd4610

@mkoeppe mkoeppe changed the title Add build/pkgs/SPKG/install-requires.txt for all Python packages Add build/pkgs/SPKG/install-requires.txt for all Python packages, remove some unneeded packages Nov 12, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

comment:28

Good enough as a first approximation?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 12, 2020

comment:29

Missing: numpy

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

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

6ec00ddbuild/pkgs/{numpy,pillow}/install-requires.txt: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 12, 2020

Changed commit from ebd4610 to 6ec00dd

@dimpase
Copy link
Member

dimpase commented Nov 13, 2020

comment:31

OK, good. (all tests of this branch pass on Fedora 30, too)

@dimpase
Copy link
Member

dimpase commented Nov 13, 2020

Reviewer: Dima Pasechnik

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 14, 2020

comment:32

Thanks!

@vbraun
Copy link
Member

vbraun commented Nov 20, 2020

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

5 participants