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

Skip backport packages for newer python #36776

Merged
merged 6 commits into from
Jan 14, 2024

Conversation

orlitzky
Copy link
Contributor

Three of our SPKGs are backports that provide python-3.11 features:

  • importlib_metadata
  • importlib_resources
  • typing_extensions

As a result, they are not needed when sage's python is v3.11 or newer. We un-require them in spkg-configure.m4

@orlitzky
Copy link
Contributor Author

I ran ./configure with python-3.10 and python-3.11 to ensure that the checks work but I'm entrusting the build itself to the CI.

@mkoeppe
Copy link
Member

mkoeppe commented Nov 26, 2023

Need to change install-requires.txt for these packages then to conditionalize on Python version.

@orlitzky
Copy link
Contributor Author

@dimpase
Copy link
Member

dimpase commented Nov 28, 2023

you need to check that you're not using Python from the system (currently needing this package). Just wrap everything in
SAGE_SPKG_DEPCHECK([python3],...)

@orlitzky
Copy link
Contributor Author

The new version checks $PYTHON_MINOR, which should be set regardless of where python comes from, instead of executing python code. I also added an spkg-configure.m4 for the new exceptiongroup package we'll be getting.

@tornaria
Copy link
Contributor

tornaria commented Dec 9, 2023

@tornaria
Copy link
Contributor

tornaria commented Dec 9, 2023

Also: nothing in sagelib uses either package, and afaiu python modules are pip-installed. Seems highly redundant to document dependencies of dependencies.

It seems to me only dependencies of packages with custom install should be kept. E.g. typing_extensions is only a dependency of setuptools_scm, so it's not really a dependency of sagemath at all, and it shouldn't be listed in requirements of sagemath-standard, etc. BTW, there are tons of packages where spkg-install.in is nothing more than sdh_pip_install ., can those be removed?

BTW six seems to be completely unnecessary (as in: not even dependencies need it anymore).

@mkoeppe
Copy link
Member

mkoeppe commented Dec 9, 2023

Clearer dependencies will be possible with #36740, please review

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

Yes that's when the libraries were added, but newer Python versions provide new functions and, important for us, better support for PEP 420 namespace packages.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

afaiu python modules are pip-installed.

Normal packages (with checksums.ini) do not have access to PyPI. So we are installing all dependencies; and when installed from source, also build dependencies.

@tornaria
Copy link
Contributor

Clearer dependencies will be possible with #36740, please review

I'm not interested in build/* at all. In fact, I want to be able to build sagelib without it. There's nothing in there that is really necessary at all but it's all tangled up.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

OK why talk about spkg-* anything then?

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

src/pyproject.toml.m4 and src/setup.cfg.m4 is all that will matter to you then.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

It's true that currently these backport packages are not used in sagelib.
We have declared them as dependencies so that sagelib developers can rely on the new library features even though we still support Python 3.9. See https://doc.sagemath.org/html/en/developer/coding_in_python.html#python-language-standard

@tornaria
Copy link
Contributor

OK why talk about spkg-* anything then?

I just came by this PR since src/setup.cfg lists as required some packages that I don't think belong there. The most obvious ones being six, importlib_metadata, importlib_resources, my search led here, etc.

I made a comment in good faith. My mistake, sorry. I'll retract back to my corner of the world.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

You are right that six should be removed from src/setup.cfg.m4. PR please?

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

BTW, there are tons of packages where spkg-install.in is nothing more than sdh_pip_install ., can those be removed?

Yes, I think that could be a nice simplification.

@tornaria
Copy link
Contributor

You are right that six should be removed from src/setup.cfg.m4. PR please?

Sure. What about importlib_resources, importlib_metadata, typing_extensions ? They don't seem to be required by sagemath-standard (eventually as a transitive dependency).

Also: sphinx, not really necessary for running sage, afaict.

Another thing is sage_conf: I do fine with a two-line sage_conf.py (only one line after #36792) although I'd rather have this as a configuration file inside sage package.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

Sure. What about importlib_resources, importlib_metadata, typing_extensions ? They don't seem to be required by sagemath-standard

As I said in #36776 (comment), these are there deliberately

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

I never proposed that sagemath moves to python 3.12, only that requirements.txt stops claiming that it won't work with python 3.12, because it does work.

@tornaria Given your report of this, I would support making this change. PR please?

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

Also: sphinx, not really necessary for running sage, afaict.

This one comes in through sage.misc.sagedoc / sage.misc.sphinxify

Nice to have: maybe. Required: nope.

@tornaria I know. I made that work in #32759.

But as you observed elsewhere, it provides the formatting of the docstrings in the interactive help.

That's a standard functionality, so it's a dependency of sagemath-standard.

@orlitzky
Copy link
Contributor Author

System gap is usable. The system data packages I place in /usr/share/sagemath and they are usable (although SAGE_SHARE needs to be set to that, which in turns means I have to set GAP_SHARE_DIR and JMOL_DIR). sagelib (i.e. sagemath-standard) can be built using setuptools but one has to do some acrobacies. It would be nice if one could just cd sagemath-standard && python -m build --no-isolation --wheel and be done with it.

Yeah that's exactly what I mean. You shouldn't have to run your own distro to be able to build sagelib. Sage should be able to detect GAP and find those databases on its own rather than requiring you to place them exactly where it wants them and/or hand-edit conf.py.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 12, 2023

I never proposed that sagemath moves to python 3.12, only that requirements.txt stops claiming that it won't work with python 3.12, because it does work.

@tornaria Given your report of this, I would support making this change. PR please?

That's now #36869

# These packages are therefore not needed with >=python-3.11. Here
# we test for a python minor version component greater than or equal
# to 11, and mark this package as "not required" if we succeed.
AC_MSG_CHECKING([for >=python-3.11])
Copy link
Member

Choose a reason for hiding this comment

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

Why does the Python version need a separate check in spkg-configure.m4?
Does the conditionalization in install-requires.txt not do the job already? Checking whether importlib_metadata >=4.13; python_version<'3.11' is available will be trivially true for Python >= 3.11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. I wrote that check before you pointed out that we needed to change install-requires.txt. It's probably redundant now but I have to get flint-3.x packaged before I can run ./configure now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change the quoting in the macro but it basically works, if you don't care that it says "will use the system foo" rather than "foo is not required" when it skips the package.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine

Some version specifiers in install-requires.txt will contain single
quotes; for example, python_version<'3.11'. (This is according to the
setuptools "Platform Specific Dependencies" documentation.) To support
that, we double-quote the spec string rather than single quote it.
@@ -75,10 +75,13 @@ AC_DEFUN([SAGE_PYTHON_PACKAGE_CHECK], [
PYTHONUSERBASE="${HOME}/.sage/local"
])

dnl double-quote SAGE_PKG_VERSPEC because platform-specific
dnl dependencies like python_version<'3.11' will have single
dnl quotes in them.
Copy link
Member

Choose a reason for hiding this comment

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

Actually, you can use double quotes in install-requires.txt.

And in fact double quotes are better because then m4/sage_spkg_versions_toml.m4 will be generated correctly!

Copy link
Member

Choose a reason for hiding this comment

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

(relevant for #36951)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, double quotes it is. I changed the package-check macro to normalize them (back to single quotes) so that both are supported, at least in that one macro.

For the moment, this is an easy fix for the ./bootstrap generation
of sage_spkg_versions_toml.m4, which single-quotes the contents
of install-requires.
Platform version constraints in install-requires can be either single-
or double-quoted. To keep things simple, we'd like to assume in the
macro that they are single-quoted, following the examples in the
documentation. We now use sed to normalize them.
Copy link

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

Copy link
Member

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 27, 2023
    
Three of our SPKGs are backports that provide python-3.11 features:

* importlib_metadata
* importlib_resources
* typing_extensions

As a result, they are not needed when sage's python is v3.11 or newer.
We un-require them in `spkg-configure.m4`
    
URL: sagemath#36776
Reported by: Michael Orlitzky
Reviewer(s): Matthias Köppe, Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 2, 2024
    
Three of our SPKGs are backports that provide python-3.11 features:

* importlib_metadata
* importlib_resources
* typing_extensions

As a result, they are not needed when sage's python is v3.11 or newer.
We un-require them in `spkg-configure.m4`
    
URL: sagemath#36776
Reported by: Michael Orlitzky
Reviewer(s): Matthias Köppe, Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 5, 2024
    
Three of our SPKGs are backports that provide python-3.11 features:

* importlib_metadata
* importlib_resources
* typing_extensions

As a result, they are not needed when sage's python is v3.11 or newer.
We un-require them in `spkg-configure.m4`
    
URL: sagemath#36776
Reported by: Michael Orlitzky
Reviewer(s): Matthias Köppe, Michael Orlitzky
@vbraun vbraun merged commit f7ae5bf into sagemath:develop Jan 14, 2024
27 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Jan 14, 2024
@orlitzky orlitzky deleted the skip-backport-packages branch January 24, 2024 14:42
vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 24, 2024
    
After sagemath#36129 and sagemath#36776 we are still left with a bunch of external (non-
Sage) Python packages
which need `spkg-configure.m4` and distros info.

Here we add these.

### 📝 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: ...
-->

- sagemath#36129
- sagemath#36776

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36777
Reported by: Dima Pasechnik
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2024
…kages

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

- sagemath#35203 added these packages as unconditional dependencies
- prompted by a sporadic use of typing_extensions in the Sage library
(see sagemath#34831)
- motivated further by the demand to immediately drop support for Python
3.8 so that newer typing features can be used in the Sage library
(sagemath#35404)
- sagemath#36776 reduced the packages to
conditional dependencies

Here we improve the documentation in the section "Language standard" of
the developer's guide so that it aligns with how the conditional
dependencies are declared.

Based on changes split out from sagemath#37399.

### 📝 Checklist

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

- [ ] The title is concise and informative.
- [ ] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37654
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 24, 2024
…gemath#36776

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

As reported in https://groups.google.com/g/sage-devel/c/8ZsBcHGmn6s
@culler

### 📝 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#37959
Reported by: Matthias Köppe
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants