Developer guide: Improve discussion of backport packages #37654
Conversation
mkoeppe
force-pushed
the
backport_packages_clarification
branch
2 times, most recently
from
f3f165f to
8a0812d
Compare
|
Documentation preview for this PR (built with commit 7c52361; changes) is ready! 🎉 |
mkoeppe
force-pushed
the
backport_packages_clarification
branch
from
8a0812d to
52001da
Compare
|
Can you make a list of the features from backport packages not available on python 3.9 that could be useful? I think right now nothing in sagelib uses the backport pkgs, seems better to keep it that way. |
|
That's out of scope of this PR. |
|
Thanks for the review! |
…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
|
|
||
| if sys.version_info < (3, 11): | ||
| # Use backport package providing Python 3.11 features | ||
| from importlib_resources import files | ||
| else: | ||
| from importlib.resources import files |
There was a problem hiding this comment.
This gives the impression that we recommend using the backport packages. IMO, we should discourage the backport packages. Right now, afaict, nothing in the sage library requires the backport packages and we should keep it that way.
The risk is that if someone uses the backport packages it might use a newer feature not available on current python. We've already pointed out that distros are not shipping some of the backport packages.
Please remove this suggestion.
There was a problem hiding this comment.
It's documenting the status quo, which was established by positively reviewed PRs in the past.
My point here is: if there is a concrete feature that justifies using the backport packages for python <= 3.11, it would be reasonable to compromise. But what is that feature? I know |
|
@tornaria Once more, this is outside of the scope of this PR. It is improving the documentation of what is the current policy. You are asking for a justification of the status quo and/or to change the policy. That's a separate issue -- and, most importantly, the burden of justifying the change is on you there. |
What you document here is not the status quo: nothing in sagelib uses any backport package afaict. It's already agreed that the backport packages should not be used beyond python 3.11, but I don't think we really need them even for 3.9. If there's a concrete feature that is available on 3.12 but not on 3.9 that we might want to use, it would be fine to refer to the specific feature. |
The status quo is that nothing currently uses it and that developers can use it because it's a declared dependency. |
I don't understand your position. If you need some feature for the modularization, just say so, and we can mention the particular feature. Otherwise, we all benefit from reducing dependencies, modularization as well. At the very least add a warning that features not available on python 3.12 should not be used. But really: since we are not using it, just dropping it seems the best approach. |
What "position"? You disagree that it is a correct description of the status quo? |
Yes, I disagree. These packages were added as requirements to some other packages (jsonschema, virtualenv) on older versions of python. There is no status quo or policy about using these packages on sagemath itself. Meanwhile, I believe it has already been agreed not to use them beyond python 3.11, so developers have to be at least warned about this (this is If there is a useful feature available on 3.11 or 3.12 but not on older python, that might be a good reason. Otherwise, it's just simpler to discourage them altogether to avoid somebody inadvertently using some feature that causes trouble later. That is my position, but we don't know what is yours. |
But @tornaria, this is obviously false, as very first link in the PR description shows. |
|
Irrelevant, as I am sure you understand. |
|
merge conflict |
…ow to import importlib.resources
… to import from importlib.resources
…imports of backports are not possible
mkoeppe
force-pushed
the
backport_packages_clarification
branch
from
7779ec5 to
7c52361
Compare
github-actions
bot
added
s: needs review
and removed
s: positive review
s: needs work
labels
typing_extensions,importlib_resources,importlib_metadata#35203 added these packages as unconditional dependenciesHere 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 #37399.
📝 Checklist
⌛ Dependencies