-
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
Improve how we manage which PDM to use #22378
base: master
Are you sure you want to change the base?
Improve how we manage which PDM to use #22378
Conversation
bc2213d
to
3a05c50
Compare
bbcddc4
to
3644705
Compare
+@BetsyMcPhail for feature review, 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.
On our local mac, I ran venv_upgrade
with the changes from this PR. It successfully updated both pdm.lock
as well as requirements.txt
.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
tools/workspace/python/venv_sync
line 82 at r1 (raw file):
# Aside from that, we want to use the pinned version, so being informed about # newer versions is just noise. export PDM_CHECK_UPDATE=0
BTW The venv_update
script still complained about an out-of-date PDM. e.g.
L ./setup/python/pdm.lock
Changes are written to pdm.lock.
0:00:11 🔒 Lock successful.
INFO: PDM 2.22.0 is installed, while 2.22.1 is available.
Please run pdm self update
to upgrade.
Run pdm config check_update false
to disable the check.
I'm not sure if this is expected with this change? In any case, it did complete the update, so it was just noise.
64ae445
to
7d1d867
Compare
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, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes
tools/workspace/python/venv_sync
line 82 at r1 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
BTW The
venv_update
script still complained about an out-of-date PDM. e.g.L ./setup/python/pdm.lock
Changes are written to pdm.lock.
0:00:11 🔒 Lock successful.
INFO: PDM 2.22.0 is installed, while 2.22.1 is available.
Please runpdm self update
to upgrade.
Runpdm config check_update false
to disable the check.I'm not sure if this is expected with this change? In any case, it did complete the update, so it was just noise.
Ah. It's "expected" in that only venv_sync
has the "please be quiet" logic. However, it's probably sensible if PDM is out of date to update it first. I made a small change to accomplish that (and also noticed a minor error in the process, namely, the repo wasn't monitoring the new requirements.txt
for changes); PTAL.
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 r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
tools/workspace/python/venv_sync
line 82 at r1 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Ah. It's "expected" in that only
venv_sync
has the "please be quiet" logic. However, it's probably sensible if PDM is out of date to update it first. I made a small change to accomplish that (and also noticed a minor error in the process, namely, the repo wasn't monitoring the newrequirements.txt
for changes); PTAL.
I re-ran venv_update
with the latest changes and it completed with no warnings.
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.
+@xuchenhan-tri for platform review, please.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), missing label for release notes (waiting on @mwoehlke-kitware)
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.
@BetsyMcPhail your LGTM stamp is missing.
Platform +(release notes: none)
Reviewed 4 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion
tools/workspace/python/update_requirements.py
line 37 at r2 (raw file):
def main(args):
nit, this file needs some brief documentation on what this does.
On additional poking, there's a slightly better way to do this. Stay tuned. (@BetsyMcPhail, will need a re-review...) |
444e030
to
c135eb0
Compare
Done. Note that I am deliberately not updating the |
Previously, xuchenhan-tri wrote…
No longer applicable. |
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 confirmed venv_upgrade
still upgrades with latest changes.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion
tools/workspace/python/venv_upgrade
line 59 at r3 (raw file):
readonly requirements="./setup/python/requirements.txt" cat > "${requirements}" <<EOF # This file describes does NOT describe the requirements for Drake itself, but
BTW Typo in this sentence
c135eb0
to
d8a97da
Compare
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 xuchenhan-tri(platform),BetsyMcPhail
tools/workspace/python/venv_upgrade
line 59 at r3 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
BTW Typo in this sentence
Fixed. (Also in setup/python/requirements.txt
.)
a4603a6
to
998a5db
Compare
@BetsyMcPhail, can you re-check v6 please?
|
Move our PDM version pin to a separate file. This both makes it easier to manage updates to the same and also allows the pinned version to be usable outside of the venv_sync script. Teach venv_upgrade to use PDM itself to generate this file, which accomplishes the outstanding goal of being able to pin both PDM and its dependencies. Also, modify venv_sync to tell PDM to not complain about newer versions.
998a5db
to
481a3f2
Compare
@drake-jenkins-bot linux-jammy-gcc-bazel-experimental-debug please |
Move our PDM version pin to a separate file. This both makes it easier to manage updates to the same and also allows the pinned version to be usable outside of the
venv_sync
script. Teachvenv_upgrade
to use PDM itself to generate this file, which accomplishes the outstanding goal of being able to pin both PDM and its dependencies. Also, modifyvenv_sync
to tell PDM to not complain about newer versions.Toward #22040.
This change is