Modify install_prereqs to create a venv #22477
Conversation
|
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-cmake-experimental-packaging please. |
|
BTW, I'm not sure if this should get release notes or not. That Drake now expects to create its own venv might be a "gotcha" for existing users. |
jwnimmer-tri
added
the
release notes: feature
mwoehlke-kitware
force-pushed
the
macos-install-uses-venv
branch
from
ab9a4c3 to
dde1d53
Compare
|
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-cmake-experimental-packaging please. |
mwoehlke-kitware
force-pushed
the
macos-install-uses-venv
branch
from
dde1d53 to
50b9968
Compare
|
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-cmake-experimental-packaging please. |
|
+@jwnimmer-tri for feature review, please |
There was a problem hiding this comment.
LGTM on paper -- just one question in a new top-level thread below ...
Reviewed 2 of 7 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Has this been tested against DEE CI? I didn't see any.
It's probably worth opening a DEE PR that changes this line to point to the experimental tgz published from this pull request, and make sure everything is still happy.
setup/mac/binary_distribution/install_prereqs.sh line 64 at r3 (raw file):
brew bundle --file="${BASH_SOURCE%/*}/Brewfile" --no-lock if ! command -v pip3.12 &>/dev/null; then
BTW Is this still the best condition to check for? Does PDM call pip3.12 under the hood, or just do its own thing?
setup/BUILD.bazel line 10 at r3 (raw file):
exports_files([ "mac/binary_distribution/Brewfile", "mac/binary_distribution/requirements.txt",
nit This line seems stale now?
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Note that #22040 task (2) is still unfinished in this PR. I find it slightly suspicious to mix a PDM lockfile with a Homebrew version of numpy. I would imagine that their versions are not always strictly compatible? Or maybe the homebrew numpy is dead code now, if the venv sys.patch excludes it?
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
I would like to land #22503 ahead of this PR. I don't trust our testing here when the PDM dependencies are unpinned but still installed into the same venv as Drake's dependencies.
mwoehlke-kitware
force-pushed
the
macos-install-uses-venv
branch
from
50b9968 to
d6a5b74
Compare
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
setup/BUILD.bazel line 10 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit This line seems stale now?
Probably. Done.
setup/mac/binary_distribution/install_prereqs.sh line 64 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Is this still the best condition to check for? Does PDM call
pip3.12under the hood, or just do its own thing?
This is the check for "did brew correctly install Python?". I suppose we could just check for python3.12 instead, but since we need python3.12 -m venv to give us pip3, checking that pip already exists doesn't seem unreasonable. (But I'll entertain arguments either way.)
Checking for pdm at this point isn't right, because we haven't installed it yet! And, while I don't know if PDM uses pip under the hood, we do use pip to install pdm itself.
TL;DR: We're still about to call pip, we've just changed from <brew>/pip3.12 to <venv>/pip3.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
setup/mac/binary_distribution/install_prereqs.sh line 64 at r3 (raw file):
... we do use
pipto installpdmitself.
Aha, this is definitely a good reason. I hadn't connected those dots, but I probably should have. No changes necessary.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Note that #22040 task (2) is still unfinished in this PR. I find it slightly suspicious to mix a PDM lockfile with a Homebrew version of numpy. I would imagine that their versions are not always strictly compatible? Or maybe the homebrew numpy is dead code now, if the venv sys.patch excludes it?
(Oops, I meant to say "if the venv sys.path excludes it".)
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
(Oops, I meant to say "if the venv sys.path excludes it".)
That's an interesting question. pdm.lock pins numpy and it looks like it's being installed by the venv in practice. Do we still need numpy from Brew? Maybe we should PR its removal first?
I believe we've been installing it via the venv since pip-tools; at least, I see it was already in the pip-tools lock files. I didn't realize we were also installing it via brew.
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I would like to land #22503 ahead of this PR. I don't trust our testing here when the PDM dependencies are unpinned but still installed into the same venv as Drake's dependencies.
WFM.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
We definitely need a usable numpy, the only question is where it comes from.
Maybe we should PR its removal first?
We can't do it first, because then otherwise it would be missing for tgz users. (We can't install it system-wide under --break-system-packages.)
It seems to me like it would be safest to remove it in this PR, instead of a future one?
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
WFM.
Done.
mwoehlke-kitware
force-pushed
the
macos-install-uses-venv
branch
from
d6a5b74 to
baea07a
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
We definitely need a usable numpy, the only question is where it comes from.
Maybe we should PR its removal first?
We can't do it first, because then otherwise it would be missing for tgz users. (We can't install it system-wide under
--break-system-packages.)It seems to me like it would be safest to remove it in this PR, instead of a future one?
Makes sense. Done.
|
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-cmake-experimental-packaging please. |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
mwoehlke-kitware
force-pushed
the
macos-install-uses-venv
branch
from
baea07a to
45d0672
Compare
|
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-cmake-experimental-packaging please. |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
|
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-cmake-experimental-packaging please. |
1 similar comment
|
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-cmake-experimental-packaging please. |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Has this been tested against DEE CI? I didn't see any.
It's probably worth opening a DEE PR that changes this line to point to the experimental tgz published from this pull request, and make sure everything is still happy.
There was a problem hiding this comment.
+@sammy-tri for platform review per schedule, please.
Reviewable status: LGTM missing from assignee sammy-tri(platform)
|
FYI @mwoehlke-kitware needs a rebase (re-lock). |
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1, 1 of 3 files at r2, 1 of 2 files at r3, 1 of 3 files at r4, 2 of 3 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
doc/_pages/from_binary.md line 83 at r6 (raw file):
```bash sudo env/share/drake/setup/install_prereqs
This didn't have a sudo before. What changed?
Modify the macOS install_prereqs script used for Drake tarball installations to create a Python virtual environment in the location where Drake has been extracted. Also use this to install Drake's Python dependencies using PDM. Update user documentation accordingly. This means we are now using the pinned versions of our dependencies and are no longer keeping a duplicate copy of this list solely for installations. This also means that we are installing a pinned numpy in the venv for installations. This was already the case for source builds, but means we now no longer have any reason to install numpy from Homebrew, so remove it from there.
mwoehlke-kitware
force-pushed
the
macos-install-uses-venv
branch
from
45d0672 to
35aa703
Compare
There was a problem hiding this comment.
Done. Note, there are no effective changes to the pyproject.toml, so the only change to pdm.lock is updating the input hash. (What changed, besides a comment, is that numpy is explicitly listed as a dependency. But it was already an indirect dependency, so nothing in the lock file changes.)
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
doc/_pages/from_binary.md line 83 at r6 (raw file):
What changed is that this line:
(On Ubuntu, the script might ask to be run under
sudo.)
...was removed. In practice, "might" is "will"; n.b. ubuntu/binary_distribution/install_prereqs.sh#L28-L31. Since Ubuntu and macOS now have separate instructions, there is no need to omit the sudo with a note that it "might" be necessary.
That said, there is active work happening with the install scripts and whether they want sudo or not, but AFAICT, as of writing sudo is needed.
|
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Clarification (pedantic): (¹ There are cases where we disallow running via |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees sammy-tri(platform),jwnimmer-tri(platform)
doc/_pages/from_binary.md line 83 at r6 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Clarification (pedantic):
sudois required unless you are alreadyroot(e.g. in Docker). But since it should be harmless in that case¹, it seems simpler to just specify it.(¹ There are cases where we disallow running via
sudowhen alreadyroot, but those seem to apply only to building from source, not to binary distributions.)
Ok, thanks for the explanation!
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status:
|
@drake-jenkins-bot linux-jammy-gcc-bazel-experimental-debug please |
Modify the install_prereqs script used for Drake tarball installations to create a Python virtual environment in the location where Drake has been extracted. On macOS, also use this to install Drake's Python dependencies using PDM. Update user documentation accordingly.
The changes for Ubuntu are mainly to preserve consistency. For macOS, this change means we are now using the pinned versions of our dependencies and are no longer keeping a duplicate copy of this list solely for installations.
Towards #22040.
This change is