-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
PEP 817: Wheel Variants: Beyond Platform Tags #4740
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
base: main
Are you sure you want to change the base?
Conversation
|
Oh thanks @hugovk I totally didn't see that one being published within 20min :D Let me know which number you want me to pick and I'll do the update ;) |
|
Thanks! @DEKHTIARJonathan You may continue with 817. |
AA-Turner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very brief review, I haven't yet read beyond the end of Motivation.
A
|
Just a brief bystander comment: I'm so stoked to see this PEP draft published! |
Co-authored-by: Adam Turner <[email protected]>
Thanks Jannis! Took some significant amount of work but we eventually got there |
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
e34e8fa to
64f32ed
Compare
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Reflow the text to restore correct text width after all the inline changes and applied suggestions. Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Remove accidental double spaces that vim's `gq` introduced while I was reflowing the text. Thanks to @konstin for noticing. Signed-off-by: Michał Górny <[email protected]>
| enabling the optional provider or selecting the variant explicitly. | ||
|
|
||
|
|
||
| Package ABI matching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super excited about this one by the way :D
|
@hugovk @AA-Turner what needs to be done in order for the PEP to be merged ? |
|
Sorry for my delay here. I will review the remainder of the PEP this evening (UK time). A |
|
Fantastic, thanks Adam ;) |
|
Note this will be the longest PEP..! Thanks Adam for reviewing. |
I can pay with cookies for any emotional trauma induced by this work :D Take your time, it took us close to one year to formalize everything, build prototypes and write the PEP. |
|
I've completed my first detailed read-through (on printed paper!), but it's now firmly in the wee hours, so I'm going to continue in the morning. Writing up the review comments should go much quicker though! A |
AA-Turner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry again for the further delay, this review has taken much longer than expected. I was half hoping someone else would beat me to it!
I've added several inline comments, albeit I ran out of steam a little (it is long past my bedtime!), so I might revisit the end of the PEP again.
General points:
- I think the specification especially needs to be tightened up, to make it clear what exactly is being required of installers, builders, indexes, etc in as concicse wording as possible. The prose text in Specification should be moved elsewhere.
- Significant chunks of text could be deleted/moved to an appendix. I've highlighted some obvious ones
- I have found myself getting a bit confused by the jargon in the PEP, especially e.g. AoT, which might usefully be called 'static' plugins? The glossary was quite useful though.
- Parts of the PEP appear to be saying conflicting things. This might be in my reading, but it'd be useful to clarify regardless.
So long, and thanks for all the fish cookies,
Adam
| The goal is for a familiar ``[uv] pip install <package>`` to provide | ||
| the best user experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The goal is for a familiar ``[uv] pip install <package>`` to provide | |
| the best user experience. | |
| The goal is for the obvious installation commands (``{tool} install <package>``) | |
| to select the most appropriate wheel, and provide the best user experience. |
| The Python packaging ecosystem has evolved to support increasingly | ||
| diverse computing environments. The current software ecosystem often |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"software ecosystem" here is software in general rather than just Python? Would be useful to disambiguate from "packaging ecosystem" in the previous sentence.
| format cannot adequately express the diversity of features present | ||
| in modern hardware. | ||
|
|
||
| For example, packages such as `PyTorch <https://pytorch.org/>`_ need to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current style is fine, but using the same PyTorch link reference with different link targets/URIs technically creates ambiguous references in the RST spec. Prefer double-trailing-underscore to fix this.
| According to the `2024 Python Developers Survey | ||
| <https://lp.jetbrains.com/python-developers-survey-2024/#purposes-for-using-python>`__, | ||
| a significant portion of respondents over the last years have been | ||
| successively using Python for scientific computing purposes, covering | ||
| such areas as Data analysis (steadily over 40% respondents), Machine | ||
| learning (grown to 40% in 2024) and Data engineering (around 30%). | ||
| Many of these use cases are directly impacted by suboptimal | ||
| packaging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make this para the start of Motivation, it provides a useful contextualisation
| According to the `2024 Python Developers Survey | |
| <https://lp.jetbrains.com/python-developers-survey-2024/#purposes-for-using-python>`__, | |
| a significant portion of respondents over the last years have been | |
| successively using Python for scientific computing purposes, covering | |
| such areas as Data analysis (steadily over 40% respondents), Machine | |
| learning (grown to 40% in 2024) and Data engineering (around 30%). | |
| Many of these use cases are directly impacted by suboptimal | |
| packaging. | |
| The `2024 Python Developers Survey`__ shows that a significant proportion of | |
| Python's users have scientific computing use-cases. This includes data analysis | |
| (40% of respondents), machine learning (30%), and data engineering (30%). | |
| Many of these use-cases are directly impacted by suboptimal Python packaging. | |
| __ https://lp.jetbrains.com/python-developers-survey-2024/#purposes-for-using-python |
| (``mypackage-gpu``, ``mypackage-cpu``, etc.). Each of these approaches | ||
| has significant drawbacks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| (``mypackage-gpu``, ``mypackage-cpu``, etc.). Each of these approaches | |
| has significant drawbacks. | |
| (``mypackage-gpu``, ``mypackage-cpu``, etc.). Each of these approaches | |
| has significant drawbacks and potential security implications. |
| containing file. Its individual keys are sub-dictionaries that are | ||
| described in the subsequent sections, along with the requirements for | ||
| their presence. The tools must ignore unknown keys in the dictionaries | ||
| to permit future backwards compatible updates to the PEP. However, users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| to permit future backwards compatible updates to the PEP. However, users | |
| for forwards compatibility of updates to the PEP. However, users |
| described in the subsequent sections, along with the requirements for | ||
| their presence. The tools must ignore unknown keys in the dictionaries | ||
| to permit future backwards compatible updates to the PEP. However, users | ||
| should not introduce custom keys to avoid potential future conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| should not introduce custom keys to avoid potential future conflicts. | |
| MUST NOT use unsupported keys to avoid potential future conflicts. |
| it, effectively rendering the variants using its properties | ||
| incompatible. If it is ``false``, the provider is considered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand this
| - a ``$schema`` key whose value is the URL corresponding to the schema | ||
| file supplied in the appendix of this PEP. The URL contains the | ||
| version of the format, and a new version must be added to the appendix | ||
| whenever the format changes in the future, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this over a simple version key?
| Plugins are implemented as Python packages. They need to expose two | ||
| kinds of Python objects at a specified API endpoint: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a hard requirement? The PEP also notes/suggests that installers reimplement (popular) provider packages, and e.g. uv is written in Rust, not Python. Does it cease to be a plugin when reimplemented?
Basic requirements (all PEP Types)
pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) andPEPheaderAuthororSponsor, and formally confirmed their approvalAuthor,Status(Draft),TypeandCreatedheaders filled out correctlyPEP-Delegate,Topic,RequiresandReplacesheaders completed if appropriate.github/CODEOWNERSfor the PEPStandards Track requirements
Python-Versionset to valid (pre-beta) future Python version, if relevantDiscussions-ToandPost-HistoryCC: @mgorny @konstin @rgommers @atalman @charliermarsh @msarahan @seemethere @warsaw @dstufft @aterrel
📚 Documentation preview 📚: https://pep-previews--4740.org.readthedocs.build/pep-0817/