Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Port requirements to PEP735#29281
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Note, |
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 was curious about this new PEP, so I took a look through the changes. I think this would be a really neat feature when pip implements it. I know this is just a draft, but I figured I can point out a couple things I noticed, and I had a couple of questions, too.
Uh oh!
There was an error while loading.Please reload this page.
azure-pipelines.yml Outdated
| @@ -71,8 +71,7 @@ stages: | |||
| - bash: | | |||
| python -m pip install --upgrade pip | |||
| python -m pip install --upgrade -r requirements/dev/build-requirements.txt | |||
| python -m pip install -r requirements/testing/all.txt -r requirements/testing/extra.txt | |||
| python -m pip install --group build --group test --group test-extra | |||
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.
We'll have to see if it matters whether or not we were upgrading certain dependencies or not. I suspect not.
Uh oh!
There was an error while loading.Please reload this page.
pyproject.toml Outdated
| # Extra pip requirements for the Python 3.10+ builds | ||
| test-extra = [ | ||
| "--prefer-binary", |
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.
Will command-line arguments andsys.platform references work in this PEP? It isn't entirely clear to me.
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.
Ah, the command-line argument probably won't, but we'll have to see how thepip implementation ends up. Thesys.platform should work as these arestandard dependency specifiers.
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 learned something new! Neat!
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.
So unfortunately, we can't keep--prefer-binary here, so I've moved it to the CI invocation.
.github/workflows/reviewdog.yml Outdated
| @@ -51,7 +51,7 @@ jobs: | |||
| python-version: '3.10' | |||
| - name: Install mypy | |||
| run: pip3 install -r requirements/testing/mypy.txt -r requirements/testing/all.txt | |||
| run: pip3 install --group typing | |||
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.
does this also need group "test"?
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'm hoping not, so we can reduce the install set.
pyproject.toml Outdated
| # | ||
| # You will first need a matching Matplotlib installation | ||
| # e.g (from the Matplotlib root directory) | ||
| # pip install --no-build-isolation --editable .[dev] |
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 the.[dev] correct? Or should it be just.?
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[dev] is an extra, not a dependency group. It was intended to cover both build and runtime dependencies. It didn't really work though, aspip installed. before its dependencies, so you couldn't get the build ones early enough. Based on the other implementations, dependency groups should be strictly better, since they should install before., but we'll have to see how it's finally implemented. I should probably rewrite this section a bit once that's finalized.
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.
Oh, I see. I'm not used to seeing an extra be specified against a directory, so I thought it was a typo. And, yeah, extras wouldn't work for installing dependencies.
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.
No more.[dev] now.
0b93f55 to8ef2f01CompareI rebased and fixed the comments. |
5803913 to6770028CompareThere 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.
This is also going to requirepip >= 25.1 for anyone developing Matplotlib - that should at least go in some sort of release note/changelog entry, and be noted in the development setup docs.
.github/workflows/tests.yml Outdated
| @@ -50,12 +50,12 @@ jobs: | |||
| - name-suffix: "(Minimum Versions)" | |||
| os: ubuntu-22.04 | |||
| python-version: '3.11' | |||
| extra-requirements: '-c requirements/testing/minver.txt' | |||
| extra-requirements: 'python -m pip install --upgrade $PRE --group test-minver' | |||
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 the addition of$PRE here?
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.
This was always used:https://github.com/matplotlib/matplotlib/pull/29281/files#diff-1db27d93186e46d3b441ece35801b244db8ee144ff1405ca27a163bfe878957fL218
But I've changed this line a bit to not require the full invocation.
azure-pipelines.yml Outdated
| python -m pip install -r requirements/testing/all.txt -r requirements/testing/extra.txt | ||
| python -m pip install --upgrade --group build | ||
| python -m pip install --group test | ||
| python -m pip install --prefer-binary --group test-extra |
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.
Worth a comment explaining the reason for using--prefer-binary just fortest-extra?
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.
This came from the original file:https://github.com/matplotlib/matplotlib/pull/29281/files#diff-1823d2c36efddd45c3c2fa83d0fa74b23c3559ee9ceb371d4dfcb14372d9efaeL3
But I'm actually unsure if it's applied to only the requirements file that contained it, or the entire command-line.
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.
Looking athttps://pip.pypa.io/en/stable/reference/requirements-file-format/#global-options it is global, so I'm going to add it on all calls.
pyproject.toml Outdated
| {include-group = "doc"}, | ||
| {include-group = "test"}, | ||
| {include-group = "test-extra"}, | ||
| "ruff", |
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.
pre-commit should be added here, and perhapsruff dropped since running pre-commit runs ruff.
pyproject.toml Outdated
| "types-python-dateutil", | ||
| "types-psutil", | ||
| "sphinx", | ||
| {include-group = "build"}, |
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.
Given thedoc andtest groups dont' depend onbuild, I don't thinktyping should either, and thebuild group can be additionally specified during the CI install stage.
Added a few notes for the pip 25.1 requirement. |
PR summary
PEP735 introduces the concept of "Dependency Groups"; these essentially replace our use of various
requirements.txtfiles.NOTE: This is waiting for
pipto actually implement Dependency Groups:pypa/pip#12963 We may or may not wish to wait for that to bake a bit even after it's been implemented as well.PR checklist