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

m4/pyproject_toml_metadata.m4: Allow Python 3.12 #36869

Merged
merged 2 commits into from
Apr 27, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Dec 12, 2023

We allow Python 3.12 in the distribution packages of the Sage library, based on the downstream reports by @tornaria and @kiwifb

After parts of this was done in other PRs in the meantime, this amounts to adding the missing classifier
Programming Language :: Python :: 3.12 in a bunch of files, including m4/pyproject_toml_metadata.m4.

📝 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 mkoeppe changed the title m4/pyproject_toml_metadata.m4: Allow Python 3.12 m4/pyproject_toml_metadata.m4, src/setup.cfg.m4: Allow Python 3.12 Dec 12, 2023
@mkoeppe mkoeppe self-assigned this Dec 12, 2023
@mkoeppe mkoeppe changed the title m4/pyproject_toml_metadata.m4, src/setup.cfg.m4: Allow Python 3.12 m4/pyproject_toml_metadata.m4, src/setup.cfg.m4: Allow Python 3.12 Dec 12, 2023
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 12, 2023

Someone please set #36561 back to "positive review"

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

This is a nightmare. We have 10 places where you by hand have to fill 3.12 (or 3.13).

This is m4. m4 I suppose is capable of getting LT_VERSION or LE_VERSION or whatever from some central place,
like m4/version.m4 or something.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 13, 2023

This is a nightmare. We have 10 places where you [...] have to fill [...]

That's right, Dima! But you are phrasing this rather ambiguously.

Every Python distribution package has standard metadata, which includes the python_requires information and version constraints of dependencies.

In a multirepo layout, as discussed at length in #36777, each distribution package in its separate GitHub repository would require a separate pull request to update their metadata.

But thanks to our advanced monorepo tooling (./bootstrap + m4/ + the package information organized in builds/pkgs), we get away with making the change only in m4/pyproject_toml_metadata.m4 --- except for the packages that have not been converted yet from setup.cfg to the modern metadata format pyproject.toml; see #36561.

@tornaria
Copy link
Contributor

I am having a couple of issues here:
a. sage-setup is pinned to 10.3.b1, but I have 10.2 installed in my system. Let me tell you first that packaging sage-setup from pypi as pep517 was a breeze. But if you want me to package and use sage-setup, you need to be more lenient here.
b. sage-conf is required. No, it's not required. It's not required. I tell you it's not required. I need a couple of lines which I handle with a simple sage_conf.py file, but I'm close to not requiring anything there. IMO we should try be reasonable on what we expect from distros and work without sage_conf at all.

Under some circumstances I ended up with a distribution (wheel) named UNKNOWN. I'm thinking this may be be because I had not run bootstrap or something (I sure did run bootstrap, but maybe I cleaned up without realizing). I don't know what can be done about this, but at least pyproject.toml file seems like it should be a static file, not m4. If a particular version of sage-setup is necessary (because a new feature, say) this should be tuned by hand.

In an ideal world, sage-setup has an independent version number with independent releases, and sagemath-standard doesn't require a particular sage-setup version that is not released (this means: not even in beta, because if I'm going down the road of packaging sage-setup I need to be sure I'll be able to build betas without having to muck with the sage-setup that comes with the distro).

My workflow here is something like

  1. ./bootstrap sagelib
  2. cd pkgs/sagemath-standard
  3. python -m build --no-isolation --wheel

I'm still unsure how this will work out in the end... I'm also trying cd src in step 2 to compare. This is taking long because, for some reason, building in this way seems to be giving all misses in ccache, even if I should be compiling the same files again and again. Do you have any clue about this?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2023

I don't know what can be done about this, but at least pyproject.toml file seems like it should be a static file, not m4.

It's m4 so that we can have central synchronization of version info and shared boilerplate.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2023

In an ideal world, sage-setup has an independent version number with independent releases, and sagemath-standard doesn't require a particular sage-setup version that is not released

Yes, in an ideal world, but I'm not going to do this while the modularization work is still ongoing.

Right now, all versions are synchronized because otherwise we would have to test compatibility with other published versions. I don't have the bandwidth for this.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2023

This is taking long because, for some reason, building in this way seems to be giving all misses in ccache, even if I should be compiling the same files again and again. Do you have any clue about this?

python -m build uses a temporary directory for the build, and there is no option to change that. That's

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2023

Creating a ccache-friendly version of pypa/build would be worthwhile

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2023

I do work from git checkout for develop and the tip of Volker's merging tree. For releases, I just use pypi sdist.
When using checkout, and I do this for

  • sage_conf_pypi (patched with my own values and to not run configure)
  • sage_setup
  • sage_docbuild
  • sage [soon to be renamed sagemath-standard in sage-on-gentoo because that is really what it is now]
  • sagemath-bliss
  • sagemath-meataxe
  • sagemath-sirocco
    I have high hopes of using sagemath-doc-html and sagemath-doc-pdf in the near future.
    I have boiler plate that does
  • git checkout of the sage tree
  • run bootstrap
  • run python -m build -n -x -s pkg/${pkgname}
  • untar the resulting sdist
    Next comes any distro patching I carry, then I do a regular wheel build and install with gpep517.
    My main dev machine runs a sage from Volker's merging tree and I have individual packages that do install in sync.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2023

  • sage_conf_pypi (patched with my own values and to not run configure)

Would something like sage-conf_static be useful to reduce the amount of patching?

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2023

  • sage_conf_pypi (patched with my own values and to not run configure)

Would something like sage-conf_static be useful to reduce the amount of patching?

Have to look it up. I have not seen it yet, but one of my requirement is for it to be on pypi at release time. It is annoying if it isn't.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2023

one of my requirement is for it to be on pypi at release time.

OK, no, it wouldn't be on PyPI. There can only be one version of sage-conf on PyPI -- the one for installing from PyPI

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2023

I misunderstood your sentence. I thought you meant something existing. Yes, It could be interesting, so long I can get a sdist from pypi at release time.

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2023

one of my requirement is for it to be on pypi at release time.

OK, no, it wouldn't be on PyPI. There can only be one version of sage-conf on PyPI -- the one for installing from PyPI

That's it for that. Nevermind, it is not that onerous. Possibly a build option?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2023

Possibly a build option?

Yes, we can look into doing that.

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2023

Possibly a build option?

Yes, we can look into doing that.

Mark as future work for when I have time (which is technically never :) )

@tornaria
Copy link
Contributor

In an ideal world, sage-setup has an independent version number with independent releases, and sagemath-standard doesn't require a particular sage-setup version that is not released

Yes, in an ideal world, but I'm not going to do this while the modularization work is still ongoing.

Right now, all versions are synchronized because otherwise we would have to test compatibility with other published versions. I don't have the bandwidth for this.

Does sage-setup change that much? What about having a slow release cycle? Even if sage-setup stays in the monorepo, it could have its own release version, and sage-the-distro could install sage-setup from the released version in pypi instead of the version in the monorepo.

Again: if sage-the-distro aims to be a "reference distribution" it should eat its own dogfood, not have "shortcuts".

Also as you explained in #36754 (comment), building sagemath-standard is not tested at all, so... But again, the answer IMO is to always build sage-the-lib using a released version of sage-setup (afaict, the last relevant change to sage_setup was the move to cython 3).

@tornaria
Copy link
Contributor

This is taking long because, for some reason, building in this way seems to be giving all misses in ccache, even if I should be compiling the same files again and again. Do you have any clue about this?

python -m build uses a temporary directory for the build, and there is no option to change that. That's

* [ccache for Python packages built in temporary directories (help wanted) #35874](https://github.com/sagemath/sage/issues/35874)

This would be a blocker for me. Rebuilding sagelib from clean slate takes 1:20 (most of the time cythonizing 562 files, as cython doesn't support ccache afaik). Building gives 0 ccache misses, with 563 direct hits (before cpp) and 63 preprocessed hits.

To be fair, I rerun the pep517 build with the exact same options as the setup.py build, and it took 2:08. The time difference may be in creating the wheel (i.e. zipping, which seems to be single thread). My options (via environment): SAGE_DEBUG=no SAGE_NUM_THREADS=8 PYTHONDONTWRITEBYTECODE=yes. This had 1 miss, 549 direct hits, 76 preprocessed hits. The total is the same 626 compilations.

I wonder if the SAGE_DEBUG=no makes a difference in terms of cacheability (sometimes debug information adds full paths which affect reproducibility).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 16, 2023

Under some circumstances I ended up with a distribution (wheel) named UNKNOWN. I'm thinking this may be be because I had not run bootstrap or something (I sure did run bootstrap, but maybe I cleaned up without realizing).

This could be

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 16, 2023

Does sage-setup change that much?

Yes, at least for a little while until I have consolidated the setup.py's of the modularized distributions in #35095; to be upstreamed in #36649, #36566. Also #36449 just added new stuff to it.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 19, 2023

Let's get this in?

@mkoeppe mkoeppe changed the title m4/pyproject_toml_metadata.m4, src/setup.cfg.m4: Allow Python 3.12 m4/pyproject_toml_metadata.m4: Allow Python 3.12 Dec 23, 2023
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 23, 2023

Now the last setup.cfg is gone.

@mkoeppe mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Feb 25, 2024
@jhpalmieri
Copy link
Member

I'm posting to record a vote of -1 on behalf of Tobias Diez.

@kiwifb
Copy link
Member

kiwifb commented Mar 14, 2024

+1 for me.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 7, 2024

I'm counting:

Hence I am setting it to "positive review".

@tornaria
Copy link
Contributor

tornaria commented Apr 8, 2024

Similar to #36561 and #36951.

-1 from me

Copy link

github-actions bot commented Apr 9, 2024

Documentation preview for this PR (built with commit 8fdedff; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2024

I've rebased it away from its open dependency.

@tornaria
Copy link
Contributor

LGTM

I wonder, since there already is a line requires-python = ">=3.9, <3.13" what is really the role of these "classifiers" strings?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2024

Thanks @tornaria. I don't have a better answer than "this is traditional decoration".

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2024

@tornaria Since this PR is "disputed", we have to include the vote tally. I'm assuming that your LGTM means that your -1 vote from #36869 (comment) is replaced by a +1 vote, which brings us to:

Hence the label "positive review" is correct.

@tornaria
Copy link
Contributor

Thanks @tornaria. I don't have a better answer than "this is traditional decoration".

Ok, so it's the metadata that shows up e.g. in pypi (see: https://pypi.org/project/sagemath-standard/). I'm guessing that the requires-python is the "formal" requirement (to be used by tools) and the classifiers are just for human consumption (maybe for search tools).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2024

Well, I think the classifiers are intended to be machine-readable. https://pypi.org/project/trove-classifiers/ is the canonical reference. The design is from over 20 years ago, PEP 301.

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 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 -->

We allow Python 3.12 in the distribution packages of the Sage library,
based on the downstream reports by @tornaria and @kiwifb

After parts of this was done in other PRs in the meantime, this amounts
to adding the missing classifier
`Programming Language :: Python :: 3.12` in a bunch of files, including
`m4/pyproject_toml_metadata.m4`.


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

- [x] The title is concise, informative, and self-explanatory.
- [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 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! -->
    
URL: sagemath#36869
Reported by: Matthias Köppe
Reviewer(s):
@vbraun vbraun merged commit 0db471f into sagemath:develop Apr 27, 2024
15 of 33 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Apr 27, 2024
@mkoeppe mkoeppe deleted the python_requires_allow_3.12 branch May 5, 2024 03:39
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants