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

Make pyproject.toml the source for build dependencies #36982

Merged

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Dec 31, 2023

As noted in #36951 (comment), the fact that the dependency declaration of sage-the-library depends on information contain in sage-the-distro's build folder is problematic for the separation of concerns. Sage-the-distro should rely on information of sage-the-library, not the other way around. Here we fix this for the build requirements by making src/pyproject.toml the single source of thruth. The corresponding install-requires.txt are demoted to mere indicator files (to be removed later). Moreover, this change allows us to make pyproject.toml a static file (no longer generated by configure), which should help with downstream packaging.


Happy new year!

📝 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
Member

mkoeppe commented Dec 31, 2023

pyproject.toml is not generated by configure. It's generated by bootstrap. The best way to think about bootstrap is as a script of the monorepo, not of the Sage distribution.

I expect src/pyproject.toml to go away with the modularization. And the modularized distributions still have to go through bootstrap, so I don't think this approach helps with anything.

Anyway, some change away from the install-requires.txt files should probably happen at some point, as this name has become outdated with the transition to the new names set by the pyproject.toml format. But this clearly needs more thought.

@tornaria tornaria removed their request for review December 31, 2023 13:17
@orlitzky
Copy link
Contributor

Well, this is pretty much what I had in mind when I said "but we also don't need to redesign it right now." Distro-from-library has a few benefits over library-from-distro:

  1. Having the dependencies in pyproject.toml is the standard thing and is what people expect
  2. While the library (or modular packages) may still need ./bootstrap, this reduces how much they need it. Some sub-packages could potentially become git submodules that can be checked out and built on their own without ./bootstrap; it's one fewer thing tying us to the monorepo design.

We do however still need the install-requires.txt with the versions in them (the python package ./configure check uses them). To keep everything working, we'd have to generate the build/.../install-requires.txt from the toml file(s). The sage-get-system-packages script is already parsing the toml file though so maybe that's not too hard.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 31, 2023

2. Some sub-packages could potentially become git submodules that can be checked out and built on their own without ./bootstrap; it's one fewer thing tying us to the monorepo design.

My solution for this is #31662, packaging sage-bootstrap with the metadata as a pip-installable distribution

@orlitzky
Copy link
Contributor

  1. Some sub-packages could potentially become git submodules that can be checked out and built on their own without ./bootstrap; it's one fewer thing tying us to the monorepo design.

My solution for this is #31662, packaging sage-bootstrap with the metadata as a pip-installable distribution

The example I have in mind (if sage-foo is pure python) is,

$ git clone https://github.com/sagemath/sage-foo.git
$ cd sage-foo
$ emacs src/sage/example.py
$ pip install .

That's just how people expect to work on a python project. If cython is involved, there would also be a meson setup/compile in there somewhere, but the process is pretty familiar to people. You can declare sage-bootstrap to be a dependency of sage-foo but that still adds an extra step, and it's likely that the sage-bootstrap version on pypi will be outdated relative to your git checkout of the subproject.

Anyway, details aside, it's just nicer to not have to solve this extra problem.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 31, 2023

The syncing of versions has to happen one way or the other. I think we all know the various possible designs, which differ by the time that the syncing happens and the actor that does the syncing.

In the end, it will depend on the actual developer & user community of a "subpackage" what is the best way to maintain it.

Right now we have the luxury of the monorepo, where all version info is central and syncing is done by developers, not users, by running the bootstrap script; [Edit:] and the release manager by running the sage-update-version script.

I'm all for making our system agnostic with respect to the syncing mechanism so that when it is useful for a community that takes care of a "subpackage" finds it convenient or more productive to develop in a separate repo, we are ready for it.

But at the moment there is no evidence that we have a viable community for any subpackage that would need this.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 31, 2023

@mkoeppe
Copy link
Member

mkoeppe commented Dec 31, 2023

The built-in stuff in GitHub (obviously) does not know about out install-requires.txt files. The table https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/about-the-dependency-graph#supported-package-ecosystems shows for Python:

Package manager Languages Recommended formats All supported formats
pip Python requirements.txt, pipfile.lock requirements.txt, pipfile, pipfile.lock, setup.py
Python Poetry Python poetry.lock poetry.lock, pyproject.toml

I doesn't say in the documentation what actual filenames it scans. But https://github.com/sagemath/sage/network/dependencies shows that it finds files named pyproject.toml and requirements.txt.

So a nice simple improvement for the dependency graphs / SBOM business would be:

  • to rework our install-requires.txt files as pyproject.toml files in the same place.
  • but as the table shows, it prefers concrete versions (such as requirements.txt and lock files) over abstract version constraints (as are traditionally put in pyproject.toml etc.)
  • the concrete versions of our packages are currently in our package-version.txt; for our Python packages, we could move them to requirements.txt files instead.
  • currently though, when a requirements.txt file is present, the SPKG is treated as a "pip" package (checksums.ini is ignored). But that can be changed without much trouble.

Any such changes are easier to make after the refactoring done in:

@orlitzky
Copy link
Contributor

orlitzky commented Jan 1, 2024

The syncing of versions has to happen one way or the other. I think we all know the various possible designs, which differ by the time that the syncing happens and the actor that does the syncing.

If a subpackage can be made to not depend on sage-bootstrap, then we don't need to worry about the version of sage-bootstrap. All else being equal, that's an improvement.

In the end, it will depend on the actual developer & user community of a "subpackage" what is the best way to maintain it.

Right now we have the luxury of the monorepo, where all version info is central and syncing is done by developers, not users, by running the bootstrap script; [Edit:] and the release manager by running the sage-update-version script.

I'm all for making our system agnostic with respect to the syncing mechanism so that when it is useful for a community that takes care of a "subpackage" finds it convenient or more productive to develop in a separate repo, we are ready for it.

But at the moment there is no evidence that we have a viable community for any subpackage that would need this.

We'll never need it, but it's the simple standard solution to an easy problem. The Github dependency graph is a great example of how such a solution can save us time. Putting the dependencies in the standard location lets Github just... find them, easily, in contrast with the amount of work you've outlined under build/ to achieve the same thing.

@mkoeppe
Copy link
Member

mkoeppe commented Jan 1, 2024

Putting the dependencies in the standard location

But there is no "standard location"; we have multiple packages in pkgs/, and it's the very point that they are synchronized. (As I wrote in #36982 (comment), the monolithic src/pyproject.toml is going to go away.)

@orlitzky
Copy link
Contributor

orlitzky commented Jan 1, 2024

Putting the dependencies in the standard location

But there is no "standard location"; we have multiple packages in pkgs/, and it's the very point that they are synchronized. (As I wrote in #36982 (comment), the monolithic src/pyproject.toml is going to go away.)

Inside of a pyproject.toml file is the standard location. When the monolithic one goes away, we'll have multiple pyproject.toml files, one for each subproject, having its own list of dependencies and constraints. Github will still be able to find them. and we'll still be able to figure out a version that satisfies them all to put in package-version.txt. There's even an automated tool called a package manager than can compute a feasible set of versions for us :)

@mkoeppe
Copy link
Member

mkoeppe commented Jan 1, 2024

When the monolithic one goes away, we'll have multiple pyproject.toml files, one for each subproject, having its own list of dependencies and constraints.

That's synchronizing ... by not synchronizing?

@orlitzky
Copy link
Contributor

orlitzky commented Jan 1, 2024

When the monolithic one goes away, we'll have multiple pyproject.toml files, one for each subproject, having its own list of dependencies and constraints.

That's synchronizing ... by not synchronizing?

The best kind of synchronizing! (Why would we manually synchronize them?)

@mkoeppe
Copy link
Member

mkoeppe commented Jan 1, 2024

Hm? The synchronization of version constraints through bootstrap simplifies maintenance. I think there should be a compelling reason before we start giving different version constraints to different distribution packages in pkgs/.

@orlitzky
Copy link
Contributor

orlitzky commented Jan 1, 2024

Hm? The synchronization of version constraints through bootstrap simplifies maintenance. I think there should be a compelling reason before we start giving different version constraints to different distribution packages in pkgs/.

Well, presumably you've spent years working on the modularization so that users will be able to install only some parts of sage. Why would we require them to satisfy the constraints of the pieces that they haven't installed?

@mkoeppe
Copy link
Member

mkoeppe commented Jan 1, 2024

For the convenience of developers. When users need more flexibility, that can be a compelling reason to deviate from it.

@orlitzky
Copy link
Contributor

orlitzky commented Jan 1, 2024

For this developer at least, not having to run ./bootstrap is pretty convenient. Not having to reverse engineer the correct dependencies for our distro packages would be nice as well.

@dimpase
Copy link
Member

dimpase commented Jan 1, 2024

We already constrain users and developers --- by not using rust-dependent packages.
(Arguing that it's not feasible to ship rust - which defies the logic of not shipping C++ compilers on macOS, as we just can't.)

We already hear requests from.developers (@tornaria for instance) for not putting upper limits on package versions.

It may well be that there are platforms on which sage the distro doesn't work, but sagelib does work.

@tobiasdiez
Copy link
Contributor Author

We do however still need the install-requires.txt with the versions in them (the python package ./configure check uses them). To keep everything working, we'd have to generate the build/.../install-requires.txt from the toml file(s). The sage-get-system-packages script is already parsing the toml file though so maybe that's not too hard.

Right, I forgot about that. Was indeed very easy to fix and the necessary install_requires files are now automatically generated. In the future, we may consider extracting the version from the pyproject.toml file directly also for the configure checks.

@tobiasdiez
Copy link
Contributor Author

The syncing of versions has to happen one way or the other. I think we all know the various possible designs, which differ by the time that the syncing happens and the actor that does the syncing.

And none of this is changed in this PR.

@tobiasdiez
Copy link
Contributor Author

The failing test is not related to this PR #36992.

bootstrap Outdated Show resolved Hide resolved
src/pyproject.toml Outdated Show resolved Hide resolved
@tornaria
Copy link
Contributor

tornaria commented Jan 2, 2024

For the convenience of developers. When users need more flexibility, that can be a compelling reason to deviate from it.

I'm starting to believe that when you say "modularity" you mean something different than what I understand by "modularity".

For the convenience of developers, IMO modularity means independent modules that can be built, tested, installed, updated, independently of each other. This includes having each its own dependencies, and trying to "synchronize up" or "automate" is against this.

I'm 100% in favour of static pyproject.toml files that allow me to build sage-the-lib with standard pep-517 builder, no bootstrap, no sage-setup if possible, no mandatory sage-conf, etc. Just find stuff on standard locations. Of course this only works if I have all the necessary bits already installed, but so what? The compiler will tell me if I'm missing some bits and I can correct that.

Modularity also means separation of concerns, e.g. that my distro of choice takes care of other software needed to build sagemath.

It may well be that there are platforms on which sage the distro doesn't work, but sagelib does work.

Indeed, trying #36957 I learned that "sage the distro" doesn't work on void linux (I couldn't make tox -e docker-voidlinux-standard work after spending a whole afternoon trying -- most of the time waiting between attempts which goes to show how useless it is since building sagelib from scratch only takes a few minutes).

@mkoeppe
Copy link
Member

mkoeppe commented Apr 30, 2024

Removing the version_requirements.txt files from the repo -- bad idea because repology.org uses the presence of these files to distinguish Python from non-Python packages

repology.org has nothing to do with version_requirements.txt - or anything else - in vendored by Sage packages.

@dimpase

@mkoeppe
Copy link
Member

mkoeppe commented Apr 30, 2024

The dispute resolution process is intended to resolve disagreements in the review process.

It is not a replacement for the review process; and it's certainly not an invitation for authors to ignore review comments.

@dimpase

This comment was marked as abuse.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 30, 2024

Here's what a correct version of the idea of this PR would look like:

@mkoeppe
Copy link
Member

mkoeppe commented Apr 30, 2024

@mkoeppe - why do we care about whatever repology.org is doing in this case? It has no value, who cares about the version of jupiter-foobar you are bloating sage distro with?

@dimpase, necessary reminder: "How we talk about past, current, future contributions" (https://groups.google.com/g/sage-devel/c/OeN8o14s6Jc/m/ChnpijP3AgAJ). The denunciations that you are using, the expressions of disrespect, etc., have no place in the Sage community.

@roed314
Copy link
Contributor

roed314 commented Apr 30, 2024

@tobiasdiez is correct about our procedure for disputed tickets. @mkoeppe pointed out some changes that he thinks are necessary, but the author and reviewer disagree and the vote is currently 3-1 in favor of this ticket. Others are, of course welcome to weigh in, but for now I'm setting this back to positive review.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 30, 2024

@roed314, thanks for the fast clarification about our procedures! Reminder though that I'm waiting for your response on some related matters since January.

@mkoeppe mkoeppe requested a review from kcrisman April 30, 2024 18:49
@vbraun vbraun merged commit f19f431 into sagemath:develop May 2, 2024
22 of 41 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 2, 2024
@mkoeppe
Copy link
Member

mkoeppe commented May 3, 2024

mkoeppe added a commit to mkoeppe/sage that referenced this pull request May 3, 2024
@tobiasdiez tobiasdiez deleted the build-pyprojecttoml-install-requires branch May 3, 2024 01:53
@mkoeppe
Copy link
Member

mkoeppe commented May 3, 2024

The broken repology index can now be seen in https://repology.org/projects/?inrepo=sagemath_develop

@dimpase

This comment was marked as abuse.

mkoeppe added a commit to mkoeppe/sage that referenced this pull request May 12, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 2024
…src/pyproject.toml` after sagemath#36982

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Also some unrelated fixes:
- fix for `centos-7-devtoolset` needed to adjust for a small change in
how Docker files are parsed (https://github.com/sagemath/sage/actions/ru
ns/8931531493/job/24538795775).
- fix for `ubuntu-xenial-...` and other platforms that need special
`configure` args
- fix for current macOS Docker Desktop

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37926
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
mkoeppe added a commit to mkoeppe/sage that referenced this pull request Jun 12, 2024
mkoeppe added a commit to mkoeppe/sage that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: build disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants