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

pkgs/sagemath-standard: Move metadata from setup.cfg to pyproject.toml #36951

Merged
merged 12 commits into from
Apr 12, 2024

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Dec 23, 2023

Modernize the metadata.

This is a trivial "chore" PR. It updates Python metadata to the latest format. No controversies about the current format are known about in the Python community. In a typical open source project, someone in a Maintainer role would open a PR and then immediately merge it, or when receiving such a PR from the outside, quickly review and merge it (examples: my PRs pytest-dev/pytest-mock#410 (merged in within 1 day), pyodide/pyodide#4472, pytest-dev/pytest-xdist#1020,
sagemath/cypari2#158, fplll/fpylll#258, polymake/JuPyMake#2, cvxpy/cvxpy#2276, sagemath/cysignals#177).

Also fixes the pyproject.toml build requirements of sagemath-standard, broken since 10.2, hence "critical".

📝 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

name = "sagemath-standard"
description = "Sage: Open Source Mathematics Software: Standard Python Library"
dependencies = [
SPKG_INSTALL_REQUIRES_sage_conf
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can simply use dynamic metadata and

dependencies = {file = ["build/pkgs/whatever/install-requires.txt", "build/pkgs/whatever2/install-requires.txt", ...]}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, but that would not be an improvement.
It would make the sdist of this distribution depend on the monorepo / the Sage distribution.

Copy link
Contributor

Choose a reason for hiding this comment

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

setuptools automatically includes the linked files (and only these) in the sdist.

Copy link
Member Author

Choose a reason for hiding this comment

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

These file paths build/pkgs/... do not exist in this directory.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 30, 2023

Let's get it in.

@orlitzky
Copy link
Contributor

Rebase onto develop please?

@mkoeppe mkoeppe force-pushed the sagemath_standard_pyproject_toml branch from 03f8e9c to 1a71556 Compare December 31, 2023 00:13
@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 31, 2023

Rebased.

@orlitzky
Copy link
Contributor

Both choices for the metadata (./bootstrap or file=[...]) reference the build tree from src and therefore look comparably bad to me w.r.t. maintaining library/distro separation. But we also don't need to redesign it right now. Does this cause problems for anyone? If not, this is standard python packaging churn and is fine with me.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 31, 2023

Both choices for the metadata (./bootstrap or file=[...]) reference the build tree from src

Yes, but bootstrap only runs at ... bootstrapping time, so it is a "repository concern" only and not a "distribution concern" if that makes sense. After running bootstrap, each subdirectory in pkgs is a self-contained source directory of a Python distribution.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 31, 2023

Also, in PRs such as the following, I make less code dependent on the file layout within build/pkgs. So poking into there directly from the pyproject.toml would run counter to this work.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 31, 2023

Does this cause problems for anyone?

No, I wouldn't know how it could.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 1, 2024

Let's get this in please.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

It works well. LGTM.

@tobiasdiez
Copy link
Contributor

Both choices for the metadata (./bootstrap or file=[...]) reference the build tree from src and therefore look comparably bad to me w.r.t. maintaining library/distro separation. But we also don't need to redesign it right now.

@kwankyu Sorry, forgot to mention that this suggestion was implemented in #36982.

To not hold up this PR here until #36982 is merged, I would be fine for now with putting the dynamically created information in file that is generated during bootstrap and included in pyproject.toml via the file tag.

@orlitzky
Copy link
Contributor

orlitzky commented Jan 2, 2024

It works well. LGTM.

There's now a conflicting proposal in #36982, both are probably headed to the dispute pile.

Copy link

github-actions bot commented Apr 9, 2024

Documentation preview for this PR (built with commit 18cb712; changes) is ready! 🎉

@mkoeppe mkoeppe requested a review from jhpalmieri April 11, 2024 20:43
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 11, 2024
…files, remove remaining `# coding: utf-8`

    
<!-- ^^^^^
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 -->

These directives at the top of the file inform developers about the
intended assignment of modules to pip-installable distributions.

As of this PR, there should be no change to the existing distributions
(sagemath-categories...) nor the monolithic build of the Sage library.

<!-- Why is this change required? What problem does it solve? -->
- Cherry-picked from sagemath#35095
<!-- 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! -->
- Depends on sagemath#36951 (merged here)
- Depends on sagemath#36676 (merged here)
- Depends on sagemath#37667 (merged here)
    
URL: sagemath#36964
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe, Tobias Diez
@vbraun vbraun merged commit 5ae0bc7 into sagemath:develop Apr 12, 2024
13 of 33 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Apr 12, 2024
@mkoeppe mkoeppe deleted the sagemath_standard_pyproject_toml branch April 12, 2024 23:42
@jhpalmieri
Copy link
Member

+1 from me.

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 13, 2024

Thanks, John.

@mkoeppe mkoeppe mentioned this pull request Apr 15, 2024
2 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 17, 2024
    
sagemath#36964 was inappropriately merged, since two dependencies were still
disputed.  This PR attempts to revert the merge.  It was created by

```
git revert -m 1 6ecb1d8
```

### 📝 Checklist

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

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

### ⌛ Dependencies

None, though it will also revert sagemath#36676 and sagemath#36951.
    
URL: sagemath#37796
Reported by: roed314
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 18, 2024
…arate jobs for pyright, build, modularized tests, long tests

    
<!-- ^^^^^
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 -->

<!-- Why is this change required? What problem does it solve? -->
Running the containers explicitly, instead of using the declarative
`container:` feature of GH Actions gives us more control:
- we can create more space on the host if necessary; we just scraped by
an out of space condition in sagemath#36473 /
sagemath#36469 (comment)
- we can run some operations outside of the container but in the same
job; this will make the separate "Get CI fixes" jobs unnecessary,
addressing the cosmetic concerns from
sagemath#36338 (comment),
sagemath#36349
- it enables caching between the various workflows (as first discussed
in sagemath#36446).

We split out static type checking with pyright into its separate
workflow:
- **pyright.yml**: As static checking does not need  a build of the Sage
library, for PRs that do not make any changes to packages, there's
nothing to build, and the workflow gives a fast turnaround just after 10
minutes. For applying the CI fixes from blocker tickets, this workflow
uses the technique favored in sagemath#36349.

The workflow **build.yml** first launches a job:
- **test-new:** It builds incrementally (using a tox-generated
`Dockerfile` and https://github.com/marketplace/actions/build-and-push-
docker-images) and does the quick incremental test. This completes
within 10 to 20 minutes when there's no change.

After this is completed, more jobs are launched:
- **test-mod:** It again builds incrementally and tests a modularized
distribution. Later (with more from sagemath#35095), more jobs will be added to
this matrix job for other distributions.
- **test-long:** It again builds incrementally and runs the long test.

The workflows **doc-build.yml** and **doc-build-pdf.yml** again build
incrementally and then build the documentation. The diffing code for the
HTML documentation now runs in the host, not the container, which
simplifies things. (To show that diffing still works, we include a small
change to the Sage library.)

Splitting the workflow jobs implements @kwankyu's  suggestion from:
- sagemath#35652 (comment)
(Fixes sagemath#35814)

The steps "again builds incrementally" actually use the GH Actions
cache, https://docs.docker.com/build/ci/github-actions/cache/#cache-
backend-api. When nothing is cached and the 3 workflows are launched in
parallel, they may each build the same thing. But when there's
congestion that leads to the workflows to be run serially, the 2nd and
3rd workflow will be able to use the cache from the 1st workflow. This
elasticity may be helpful in reducing congestion.

There is a rather small per-project limit of 10 GB for this cache, so
we'll have to see how effectively caching works in practice. See
https://github.com/sagemath/sage/actions/caches


<!-- 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.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36938 (merged here)
- Depends on sagemath#36951 (merged here)
- Depends on sagemath#37351 (merged here)
- Depends on sagemath#37750 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36498
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2024
…arate jobs for pyright, build, modularized tests, long tests

    
<!-- ^^^^^
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 -->

<!-- Why is this change required? What problem does it solve? -->
Running the containers explicitly, instead of using the declarative
`container:` feature of GH Actions gives us more control:
- we can create more space on the host if necessary; we just scraped by
an out of space condition in sagemath#36473 /
sagemath#36469 (comment)
- we can run some operations outside of the container but in the same
job; this will make the separate "Get CI fixes" jobs unnecessary,
addressing the cosmetic concerns from
sagemath#36338 (comment),
sagemath#36349
- it enables caching between the various workflows (as first discussed
in sagemath#36446).

We split out static type checking with pyright into its separate
workflow:
- **pyright.yml**: As static checking does not need  a build of the Sage
library, for PRs that do not make any changes to packages, there's
nothing to build, and the workflow gives a fast turnaround just after 10
minutes. For applying the CI fixes from blocker tickets, this workflow
uses the technique favored in sagemath#36349.

The workflow **build.yml** first launches a job:
- **test-new:** It builds incrementally (using a tox-generated
`Dockerfile` and https://github.com/marketplace/actions/build-and-push-
docker-images) and does the quick incremental test. This completes
within 10 to 20 minutes when there's no change.

After this is completed, more jobs are launched:
- **test-mod:** It again builds incrementally and tests a modularized
distribution. Later (with more from sagemath#35095), more jobs will be added to
this matrix job for other distributions.
- **test-long:** It again builds incrementally and runs the long test.

The workflows **doc-build.yml** and **doc-build-pdf.yml** again build
incrementally and then build the documentation. The diffing code for the
HTML documentation now runs in the host, not the container, which
simplifies things. (To show that diffing still works, we include a small
change to the Sage library.)

Splitting the workflow jobs implements @kwankyu's  suggestion from:
- sagemath#35652 (comment)
(Fixes sagemath#35814)

The steps "again builds incrementally" actually use the GH Actions
cache, https://docs.docker.com/build/ci/github-actions/cache/#cache-
backend-api. When nothing is cached and the 3 workflows are launched in
parallel, they may each build the same thing. But when there's
congestion that leads to the workflows to be run serially, the 2nd and
3rd workflow will be able to use the cache from the 1st workflow. This
elasticity may be helpful in reducing congestion.

There is a rather small per-project limit of 10 GB for this cache, so
we'll have to see how effectively caching works in practice. See
https://github.com/sagemath/sage/actions/caches


<!-- 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.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36938 (merged here)
- Depends on sagemath#36951 (merged here)
- Depends on sagemath#37351 (merged here)
- Depends on sagemath#37750 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36498
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 25, 2024
    
sagemath#36964 was inappropriately merged, since two dependencies were still
disputed.  This PR attempts to revert the merge.  It was created by

```
git revert -m 1 6ecb1d8
```

### 📝 Checklist

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

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

### ⌛ Dependencies

None, though it will also revert sagemath#36676 and sagemath#36951.
    
URL: sagemath#37796
Reported by: roed314
Reviewer(s): Dima Pasechnik
@saraedum
Copy link
Member

The changes from this PR are now at #37902 which reverts the revert from #37796.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 2, 2024
…arate jobs for pyright, build, modularized tests, long tests

    
<!-- ^^^^^
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 -->

<!-- Why is this change required? What problem does it solve? -->
Running the containers explicitly, instead of using the declarative
`container:` feature of GH Actions gives us more control:
- we can create more space on the host if necessary; we just scraped by
an out of space condition in sagemath#36473 /
sagemath#36469 (comment)
- we can run some operations outside of the container but in the same
job; this will make the separate "Get CI fixes" jobs unnecessary,
addressing the cosmetic concerns from
sagemath#36338 (comment),
sagemath#36349
- it enables caching between the various workflows (as first discussed
in sagemath#36446).

We split out static type checking with pyright into its separate
workflow:
- **pyright.yml**: As static checking does not need  a build of the Sage
library, for PRs that do not make any changes to packages, there's
nothing to build, and the workflow gives a fast turnaround just after 10
minutes. For applying the CI fixes from blocker tickets, this workflow
uses the technique favored in sagemath#36349.

The workflow **build.yml** first launches a job:
- **test-new:** It builds incrementally (using a tox-generated
`Dockerfile` and https://github.com/marketplace/actions/build-and-push-
docker-images) and does the quick incremental test. This completes
within 10 to 20 minutes when there's no change.

After this is completed, more jobs are launched:
- **test-mod:** It again builds incrementally and tests a modularized
distribution. Later (with more from sagemath#35095), more jobs will be added to
this matrix job for other distributions.
- **test-long:** It again builds incrementally and runs the long test.

The workflows **doc-build.yml** and **doc-build-pdf.yml** again build
incrementally and then build the documentation. The diffing code for the
HTML documentation now runs in the host, not the container, which
simplifies things. (To show that diffing still works, we include a small
change to the Sage library.)

Splitting the workflow jobs implements @kwankyu's  suggestion from:
- sagemath#35652 (comment)
(Fixes sagemath#35814)

The steps "again builds incrementally" actually use the GH Actions
cache, https://docs.docker.com/build/ci/github-actions/cache/#cache-
backend-api. When nothing is cached and the 3 workflows are launched in
parallel, they may each build the same thing. But when there's
congestion that leads to the workflows to be run serially, the 2nd and
3rd workflow will be able to use the cache from the 1st workflow. This
elasticity may be helpful in reducing congestion.

There is a rather small per-project limit of 10 GB for this cache, so
we'll have to see how effectively caching works in practice. See
https://github.com/sagemath/sage/actions/caches


<!-- 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.
- [x] 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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36938 (merged here)
- Depends on sagemath#36951 (merged here)
- Depends on sagemath#37351 (merged here)
- Depends on sagemath#37750 (merged here)
- Depends on sagemath#37277 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36498
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request May 2, 2024
    
<!-- ^^^^^
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 -->

As noted in
sagemath#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!

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

- [ ] 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

- sagemath#37796
<!-- 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#36982
Reported by: Tobias Diez
Reviewer(s): Dima Pasechnik, Gonzalo Tornaría, Matthias Köppe, Tobias Diez
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: distribution disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ p: critical / 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants