Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mwoehlke-kitware
Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware commented Jan 2, 2025

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.

Toward #22040.


This change is Reviewable

@mwoehlke-kitware mwoehlke-kitware marked this pull request as draft January 3, 2025 17:48
@mwoehlke-kitware mwoehlke-kitware marked this pull request as ready for review January 3, 2025 18:40
@mwoehlke-kitware mwoehlke-kitware changed the title Teach venv_upgrade to also upgrade PDM Improve how we manage which PDM to use Jan 3, 2025
@mwoehlke-kitware mwoehlke-kitware force-pushed the pdm-update-self branch 2 times, most recently from bbcddc4 to 3644705 Compare January 3, 2025 19:15
@mwoehlke-kitware
Copy link
Contributor Author

+@BetsyMcPhail for feature review, please.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a 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.
:lgtm:

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.

@mwoehlke-kitware mwoehlke-kitware force-pushed the pdm-update-self branch 2 times, most recently from 64ae445 to 7d1d867 Compare January 8, 2025 17:35
Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware left a 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 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.

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.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a 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 new requirements.txt for changes); PTAL.

I re-ran venv_update with the latest changes and it completed with no warnings.

Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware left a 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)

@xuchenhan-tri xuchenhan-tri added the release notes: none This pull request should not be mentioned in the release notes label Jan 8, 2025
Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a 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 :lgtm: +(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.

@mwoehlke-kitware
Copy link
Contributor Author

On additional poking, there's a slightly better way to do this. Stay tuned. (@BetsyMcPhail, will need a re-review...)

@mwoehlke-kitware mwoehlke-kitware force-pushed the pdm-update-self branch 2 times, most recently from 444e030 to c135eb0 Compare January 8, 2025 22:37
@mwoehlke-kitware
Copy link
Contributor Author

Done. Note that I am deliberately not updating the requirements.txt in this commit as it is somewhat awkward to do so without also having extraneous version bumps in pdm.lock. Also, this way it is obvious that venv_upgrade actually does something.

@mwoehlke-kitware
Copy link
Contributor Author

tools/workspace/python/update_requirements.py line 37 at r2 (raw file):

Previously, xuchenhan-tri wrote…

nit, this file needs some brief documentation on what this does.

No longer applicable.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

Copy link
Contributor Author

@mwoehlke-kitware mwoehlke-kitware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: 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.)

@mwoehlke-kitware mwoehlke-kitware force-pushed the pdm-update-self branch 2 times, most recently from a4603a6 to 998a5db Compare January 9, 2025 18:14
@mwoehlke-kitware
Copy link
Contributor Author

@BetsyMcPhail, can you re-check v6 please?

  • requirements.txt also gets regenerated after a PDM update.
  • The header for the same moved to a .in file instead of being inline in the script.
  • There is now a --reuse option to apply pyproject.toml changes while minimizing changes to version pins.

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.
@mwoehlke-kitware
Copy link
Contributor Author

@drake-jenkins-bot linux-jammy-gcc-bazel-experimental-debug please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants