-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use PDM to manage macOS wheel dependencies #22437
base: master
Are you sure you want to change the base?
Use PDM to manage macOS wheel dependencies #22437
Conversation
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-wheel-experimental-release please |
+@jwnimmer-tri for feature review, please. |
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-wheel-experimental-release please |
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.
feature. It took me a few false starts until I fully internalized which files were platform-specific and which files weren't, but once I understood that part it was all good.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers
a discussion (no related file):
BTW The image/build-wheel.sh
file is linux-only but doesn't say that in a comment and has what appears to be a lot of dead "if linux" and "if darwin" branches. That's probably worth cleaning up at some point soon.
setup/python/pyproject.toml
line 22 at r1 (raw file):
] [dependency-groups]
nit This has grown enough complexity now that we need more maintainer commentary for this section explaining what's going on, covering:
(1) the "purpose" or "meaning" of each group, so people can decide when/whether to add something into it
(2) some at least minimal citations to the points of control that select which group(s) are active, so developers can trace backwards to understand the effect of anything the edit here
It isn't; "$resource_root/image/build-wheel.sh" |
+@sammy-tri for platform review, please. |
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-wheel-experimental-release please |
This needs a rebase. |
5365b52
to
8cc6a28
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-wheel-experimental-release please |
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-wheel-experimental-release please |
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.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
setup/python/pyproject.toml
line 22 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit This has grown enough complexity now that we need more maintainer commentary for this section explaining what's going on, covering:
(1) the "purpose" or "meaning" of each group, so people can decide when/whether to add something into it
(2) some at least minimal citations to the points of control that select which group(s) are active, so developers can trace backwards to understand the effect of anything the edit here
Remember to still fix this part.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Hmm, this comment only just showed up. I'd prefer to punt on it; this section is going to have at least two more PRs that could make these changes, but right now there isn't a way to do this without conflicting with #22378. |
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
setup/python/pyproject.toml
line 22 at r1 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Hmm, this comment only just showed up.
I'd prefer to punt on it; this section is going to have at least two more PRs that could make these changes, but right now there isn't a way to do this without conflicting with #22378.
Ideally either this PR or the other PR would add the commentary, but I suppose if there's a third PR in the wings to be opened in the next couple days, that's sufficient.
8cc6a28
to
83a7e65
Compare
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Okay, with #22378 landed and this still waiting on platform, rebased and done. |
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 issues from me, just waiting on CI.
Reviewed 2 of 6 files at r1, 2 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),jwnimmer-tri(platform)
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.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion
setup/python/pyproject.toml
line 43 at r3 (raw file):
"websockets", ] # Dependencies needed to build a Drake wheel.
BTW Are these in addition to the dependencies = [
listed above (i.e., does this add more deps, or completely reset to just these three).
If these are additive, then the comment is incomplete / misleading.
If these are the only things installed into the venv, then I don't understand how the CMake build/install step during the wheel build works correctly (where does PyYAML come from in that case)?
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.
Reviewable status: 1 unresolved discussion
setup/python/pyproject.toml
line 43 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Are these in addition to the
dependencies = [
listed above (i.e., does this add more deps, or completely reset to just these three).If these are additive, then the comment is incomplete / misleading.
If these are the only things installed into the venv, then I don't understand how the CMake build/install step during the wheel build works correctly (where does PyYAML come from in that case)?
I guess given that "test" seems to be additive with the base dependencies, then the only problem here is that the comments in [dependency-groups]
fail to communicate that these are additive versus the baseline.
I also wonder if we want the pdm
group to be additive...
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The |
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Done. |
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.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),jwnimmer-tri(platform)
setup/python/pyproject.toml
line 43 at r3 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Done.
Okay that seems sensible, so it was only a comment/clarity problem. However, I don't see any push yet with the new comments.
83a7e65
to
2f268b7
Compare
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Oops, accidentally pushed to the wrong branch. Should be better now. |
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.
Reviewed all commit messages.
Reviewable status: 1 unresolved discussion
setup/python/pyproject.toml
line 43 at r3 (raw file):
The
pdm
group is special; we never install it viapdm sync
. It's there so we canpdm export
that group from the lock file as arequirements.txt
, and we do use--no-default
when doing so.
Don't just tell me in reviewable. Copy-paste this (or a rephrasing with equal entropy) into the comments. Otherwise it's too high of a bar for people to tease out on their own.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
"Note that these are used to regenerate setup/python/requirements.txt and are installed via pip." I don't want to document how |
2f268b7
to
0ee898b
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-wheel-experimental-release please (Not expecting surprises, but since |
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.
Reviewable status: 1 unresolved discussion
setup/python/pyproject.toml
line 43 at r3 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
"Note that these are used to regenerate setup/python/requirements.txt and are installed via pip."
I don't want to document how
venv_upgrade
works inpyproject.toml
. I added a note to refer to the script, and added additional documentation to the script.
Well, this commit isn't going to merge until the code is clear instead of confusing. I appreciate the desire to "not repeat yourself" -- that is a good habit -- but sometimes in documentation we need be a little repetitive, to help people along. We don't need the repeat everything, but the key fact that the pdm group does not incorporate the default list of dependencies
must appear here, in some form. That fact is an invariant of this design, so will not bitrot in case the sync code changes.
I don't understand this. Why would anyone expect "Dependencies needed to manage Drake's dependencies" to include "Dependencies needed to build Drake", especially when the other groups are explicitly labeled as "Additional dependencies ..."?
I don't understand this. |
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.
Reviewable status: 1 unresolved discussion
setup/python/pyproject.toml
line 43 at r3 (raw file):
Why would anyone expect "Dependencies needed to manage Drake's dependencies" to include "Dependencies needed to build Drake", especially when the other groups are explicitly labeled as "Additional dependencies ..."?
Because not everyone will read the tea leaves that "Additional" is so special that not saying "Additional" in one place is supposed to communicate they they are not additional in that case. As you say PDM by default does include the dependencies
as a baseline for all the groups. The fact that we carefully opt-out of that baseline behavior every single time we use this particular group is not obvious here.
Of course some people could draw the conclusion without any extra comments, because it would be odd to include e.g. matplotlib in our venv managment tools. However, it was not immediately clear to me so we need more comments. It will also be unclear to some future fraction of Drake's maintainers. The cost of a comment is vanishingly small, and the benefit is potentiality large.
Here's a concrete suggestion.
Dependencies needed to manage Drake's dependencies. Note that these are used to regenerate setup/python/requirements.txt (without including the baseline
dependencies
list above) and are installed via pip.
That fact is an invariant of this design, so will not bitrot in case the sync code changes.
I don't understand this.
That's fine, it's tangential and we can ignore it.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Alternatively, let's just not conflate these at all. See #22473. |
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.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),jwnimmer-tri(platform)
setup/python/pyproject.toml
line 43 at r3 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Alternatively, let's just not conflate these at all. See #22473.
OK since this will be solve by that PR, I'll close the thread and you can land this one first if you prefer.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Let's land that one, since the resolution here depends on that, then this can remove the unnecessary documentation. (They're going to conflict in either order.) |
Use Drake's PDM-managed virtual environment (@python), rather than a separate virtual environment, to manage the additional Python bits that are needed to build a wheel. This allows us to pin the versions of these dependencies, for better reproducibility and is one less place where such things are being managed. Having the wheel dependencies installed "up front" is also in line with how Linux wheels are built.
0ee898b
to
16782a3
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-wheel-experimental-release please |
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.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),jwnimmer-tri(platform)
FYI the final edits are small enough that we don't need to wait for Sam to re-look before merging this. |
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, agreed, I'm mainly waiting on CI. (Partly because I forgot to check that the previous run passed before pushing again... 😳)
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),jwnimmer-tri(platform)
Use Drake's PDM-managed virtual environment (@python), rather than a separate virtual environment, to manage the additional Python bits that are needed to build a wheel. This allows us to pin the versions of these dependencies, for better reproducibility and is one less place where such things are being managed. Having the wheel dependencies installed "up front" is also in line with how Linux wheels are built.
Toward #22042.
This change is