-
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
WIP: Use venv for Python dependencies (macOS) #21844
WIP: Use venv for Python dependencies (macOS) #21844
Conversation
Work in progress. I'm at the point where separating real failures from those caused by my local testing environment (e.g. OpenGL doesn't work) is becoming relevant. Also, some of the failures (e.g. the cpplint unit test) are not obvious how to fix and may need a second set of eyes. |
@drake-jenkins-bot mac-arm-ventura-clang-bazel-experimental-release please |
@jwnimmer-tri, since we're only doing macOS for now, |
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-experimental-release please |
63543b1
to
98dc23b
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-experimental-release please |
98dc23b
to
09aa7e0
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-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 22 files at r1, 3 of 6 files at r2.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @mwoehlke-kitware)
a discussion (no related file):
Is there instead a way to make
@venv
a stub (for now) on not-macOS?
Change repository.bzl
to check for macOS vs Linux (i.e., os_name = repo_ctx.os.name # "linux" or "mac os x"
). On macOS, still have it the venv helper scripts like you already have. On Ubuntu, have it do mkdir bin site-packages
to make empty placeholders that satisfy package.BUILD.bazel
.
requirements-base.in
line 1 at r3 (raw file):
# PyPI packages to make available as Drake's bazel `@venv` repository.
All of the requirements files should still remain approximately in their existing homes (somewhere under setup/...), not at the top-level.
09aa7e0
to
04bb79d
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-experimental-release please |
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Do we need arch-specific copies? If "yes", can |
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: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @mwoehlke-kitware)
requirements-base.in
line 1 at r3 (raw file):
Do we need arch-specific copies?
Let's try "no" for now. Since we don't store checksums in the lockfile, there is a realistic probability that upgrading just the version numbers will be sufficient, under the assumption that the arch support matrix for any given wheel is uniform across all of its contemporary versions.
(To be clear, we still want separate files for macos versus ubuntu, just not macos arm64 vs x86_64).
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
...but do we think |
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: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @mwoehlke-kitware)
requirements-base.in
line 1 at r3 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
...but do we think
upgrade
can be run on Ubuntu to upgrade the macOS lists, or vice versa?
Probably not. Each platform will run upgrade independently, so I suppose the upgrade script should prove which OS it's being run in order to know which lockfiles to adjust.
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: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @mwoehlke-kitware)
requirements-base.in
line 1 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Probably not. Each platform will run upgrade independently, so I suppose the upgrade script should prove which OS it's being run in order to know which lockfiles to adjust.
s/prove/probe/
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: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @mwoehlke-kitware)
requirements-base.in
line 1 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
s/prove/probe/
(For now, since it's macOS only, it would be OK to hard-code it to only ever upgrade the macOS lockfiles.)
04bb79d
to
4513607
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-experimental-release please |
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Done. |
4513607
to
e0776cb
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-experimental-release please |
e0776cb
to
e09ad90
Compare
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-experimental-release please |
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Looking at this, it honestly seems easier to just run |
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 22 files at r1, 1 of 12 files at r5.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @mwoehlke-kitware)
a discussion (no related file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Looking at this, it honestly seems easier to just run
setup
on Ubuntu also, especially as we're expecting to want that at some point anyway...
Debugging dead code on Ubuntu is going to be more hassle than I'm willing to entertain at this point.
I can push the patch, if you need.
.gitignore
line 26 at r5 (raw file):
/bazel-* # Platform artifacts generated by `install_prereqs`
BTW By convention, config-selection symlinks at setup live should be grouped inside gen
, so that 's the only "source tree pollution" folder that we need to ignore.
e09ad90
to
d544556
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, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
tools/workspace/python/venv_sync
line 7 at r19 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Why not put the venv in the output base? Wouldn't that enable
bazel clean
to delete it, and be consistent with the venv being created by a Bazel build step?
The output base is owned and managed by Bazel. If we start dumping things there, we cannot predict what will happen.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I thought build rules are supposed to put their outputs in the output base? Do we have precedent of other things that are part of |
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 jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
tools/workspace/python/venv_sync
line 7 at r19 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
I thought build rules are supposed to put their outputs in the output base? Do we have precedent of other things that are part of
bazel build
putting outputs in places not managed by Bazel?
The venv is a special case because it is non-hermetic on purpose. Yes, we could put the venv into .
during the repository rule and then bazel would clean it up for us. The downside is that it would make builds be incredibly slow, because re-installing the venv from scratch every time takes forever. We are putting the venv outside of Bazel's purview because that's the only way to accomplish an incremental sync.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Why would the venv be reinstalled every time? It should only be reinstalled if |
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 jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
tools/workspace/python/venv_sync
line 7 at r19 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Why would the venv be reinstalled every time? It should only be reinstalled if
requirements*
changes.
Yes, the repository rule is run when requirements.txt
changes. It is also run when the bazel background server boots up, i.e., after not running anything for a few hours.
You can see that locally by adding a execute_or_fail(repo_ctx, ["sleep", "20"])
to the python repository rule, and then doing something like:
bazel test //... --config=lint # always slow the first time
bazel test //... --config=lint # fast
bazel test //... --config=lint # fast
bazel shutdown
bazel test //... --config=lint # slow again after the daemon restart
bazel test //... --config=lint # fast
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Uh... why? Why does "the Bazel server restarted" make things out of date? |
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.
To be clear on what I expect as follow-ups from the kitware side:
(1) Any open review discussions. (At the moment, basically none.)
(2) Have you reviewed all of my changes, and understand them? Is all of the code clear and maintainable? In general, check (as an overall diff vs current master) that all of the code and documentation quality is up-to-par for what we expect in a Drake pull request.
(3) Anything and everything macOS-specific. Does it work well on macOS, as a human developing Drake on macOS? Run it locally as-if you were a developer, put it through its paces and find any usability glitches. Are the macOS wheel builds still correct? Packaging builds? If someone wants to change the requirements, does it work okay when they naively follow the written instructions?
Reviewed 1 of 1 files at r22, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
tools/workspace/python/venv_sync
line 7 at r19 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Uh... why? Why does "the Bazel server restarted" make things out of date?
Let me get back to you on that.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
So far (mostly doing eyeball review) the changes seem reasonable. It is... surprising, however, that restarting Bazel would make things out of date. It certainly seems like the right thing would be for Bazel to treat the venv as a build artifact in the usual manner. (Presumably that also gives us niceties like caching, as well, obviously, as |
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Apparently (at least part of) the answer is that repository rules are special and don't respect the usual rules for running incrementally. That said, I discovered we aren't the only ones running into this: bazelbuild/bazel#23720. |
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 jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
tools/workspace/python/venv_sync
line 7 at r19 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Apparently (at least part of) the answer is that repository rules are special and don't respect the usual rules for running incrementally. That said, I discovered we aren't the only ones running into this: bazelbuild/bazel#23720.
The challenge is that we want to do two activities in our rule -- first, interrogate the local system about it's Python interpreter; second, sync a virtual environment to a requirements file. It's the first part where cold-boot re-running is relevant.
In any case, this isn't a rathole we need to chase down -- we can write the little new helper script and get this PR landed, and revisit later if/when necessary. The whole story will change when we port to bzlmod, so I don't want to overthink this prototype.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Right. Uh... on that note, isn't |
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-experimental-everything-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.
Does it work well on macOS, as a human developing Drake on macOS?
Uh... sure? It works, I didn't have to do anything special. I don't know what else to expect, so I think we're fine here.
Reviewed 2 of 22 files at r1, 1 of 1 files at r9, 1 of 4 files at r13, 1 of 3 files at r14, 2 of 17 files at r17, 2 of 12 files at r18, 32 of 41 files at r19, 1 of 3 files at r20, 1 of 1 files at r22, 4 of 4 files at r24, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW We should still run some testing with Unprovisioned builds. Until this lands, the provisioned builds will have a snapshot of installed wheels using
pip --break-system-packages
, so we won't be sure that a from-scratch build will succeed unless we test Unprovisioned.
Sure, but my point was that we're no longer stuck with this PR leaving provisioned broken.
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Done (mostly). There are probably loose ends, but we can discuss those in situ -- I'll close out this thread.
Agreed. We pass in which Python to use, and figuring out which requirements to use seems sane. AFAICT the stuff that occurred to me is reasonably covered. There's no option for a CMake build to include tests, but per other discussion we're not sure we even want that, and we can add it later if it's needed.
setup/mac/binary_distribution/requirements.txt
line 1 at r24 (raw file):
# PyPI packages to install via pip for binary Drake distributions.
BTW, I keep wondering if this should indicate in some manner that these packages are also used for source builds...
tools/jupyter/test/jupyter_notebook_test.py
line 11 at r24 (raw file):
def test_help(self): """Ensures that `jupyter notebook` is installed (#12042).""" manifest = runfiles.Create()
This is now inconsistent with tools/install/install_test.py
...
tools/wheel/macos/build-wheel.sh
line 83 at r24 (raw file):
# ----------------------------------------------------------------------------- # Set up a Python virtual environment.
FYI, originally this comment was the same as prior to this PR ("Set up a Python virtual environment with the latest setuptools"). When the wheel added a venv, the comment below ("Install tools to build the wheel.") was added. This section doesn't actually touch setuptools
, so I changed it. However, an argument can also be made for keeping the prior wording but removing the "Install tools to build the wheel", which would be closer to the reversion which we are doing here per the advice of the TODO we are implementing.
WDYT?
setup/mac/source_distribution/requirements-base.in
line 13 at r16 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
That's for you to figure out.
Lacking any evidence to the contrary, I am going to assume that everything in the anzu requirements is spurious.
setup/mac/source_distribution/requirements-test.in
line 24 at r24 (raw file):
# a comment explaining the need for the constraint. # (None)
Note to self: this should be fixed before merge. (But I want to let CI run, as this won't have a functional effect on anything.)
tools/workspace/venv/upgrade
line 15 at r17 (raw file):
# Operate in temporary directory. cd "$tmpdir"
Why is running the tool in a scratch directory a problem? Doing so helped ensure that operation wouldn't be affected by any files lying around. It also made clean-up easy. With this change, the script is now littering the user's working tree.
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 1 of 1 files at r23, 4 of 4 files at r24, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
setup/mac/binary_distribution/requirements.txt
line 1 at r24 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
BTW, I keep wondering if this should indicate in some manner that these packages are also used for source builds...
I think the WARNING paragraph here is enough. All of our "source_distribution" subdirs necessarily and unconditionally pull in the "binary_distribution" dependencies across the board. I don't think that needs extra documentation, but if it does then it would go somewhere more central, not in each file one by one.
This requirements file is not special except in that developers need to remember to re-lock, which the WARNING covers.
tools/jupyter/test/jupyter_notebook_test.py
line 11 at r24 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
This is now inconsistent with
tools/install/install_test.py
...
I am not sure what you are getting at here? If it's just a note to yourself to work on later then that's fine, but if this something you'd like to work on or weigh on, then I'll need some more detail.
tools/wheel/macos/build-wheel.sh
line 83 at r24 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
FYI, originally this comment was the same as prior to this PR ("Set up a Python virtual environment with the latest setuptools"). When the wheel added a venv, the comment below ("Install tools to build the wheel.") was added. This section doesn't actually touch
setuptools
, so I changed it. However, an argument can also be made for keeping the prior wording but removing the "Install tools to build the wheel", which would be closer to the reversion which we are doing here per the advice of the TODO we are implementing.WDYT?
Thanks for fixing my cut-paste-edit comment pasta salad. I am fine leaving this alone in its current version.
(In the future, I imagine that we should have a requirements-build-wheel.in
which does -r requirements-build.in
and then tacks on the setuptools
and friends into its related .txt
lockfile, so that we will have a repeatable wheel build. In that case, this whole paragraph of code disappears entirely.)
tools/workspace/python/venv_expunge
line 11 at r24 (raw file):
readonly venv_root="$(bazel info output_base).venv" # Remove the venv
nit Missing period on sentence.
tools/workspace/python/venv_sync
line 7 at r19 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Right. Uh... on that note, isn't
local = True
a lie, now? Is that going to break anything?
I still need to get back to you on that. The documentation of the flag is not sufficient for us to reason about what we need here, so I need to look under the hood.
tools/workspace/venv/upgrade
line 15 at r17 (raw file):
Why is running the tool in a scratch directory a problem? Doing so helped ensure that operation wouldn't be affected by any files lying around.
IIRC you were the one who changed this. I believe critical point is that we needed the -r ../binary_distribution/stuff
line(s) in the requirements-foo.in
file(s) to resolve sibling-directory paths correctly.
With this change, the script is now littering the user's working tree.
Only if it fails. And even then, it takes 5 seconds for the developer to recover by hand. Having this script be ultra-simple is more important than automating cleanup when things crash.
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.
Note: previous unprovisioned builds passed, the latest round of changes has no effect on builds, so I'm not going to re-run them unless something else changes.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
tools/jupyter/test/jupyter_notebook_test.py
line 11 at r24 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I am not sure what you are getting at here? If it's just a note to yourself to work on later then that's fine, but if this something you'd like to work on or weigh on, then I'll need some more detail.
tools/install/install_test.py
either uses the manifest (macOS) or not (not-macOS). This uses the manifest if available, and falls back.
tools/wheel/macos/build-wheel.sh
line 83 at r24 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Thanks for fixing my cut-paste-edit comment pasta salad. I am fine leaving this alone in its current version.
(In the future, I imagine that we should have a
requirements-build-wheel.in
which does-r requirements-build.in
and then tacks on thesetuptools
and friends into its related.txt
lockfile, so that we will have a repeatable wheel build. In that case, this whole paragraph of code disappears entirely.)
Yes, we can probably do that, and since IIRC Bazel already knows if we're building a wheel, maybe that overrides requirements_flavor
. (Or we add another choice to requirements_flavor
.)
...except I'd call it requirements-wheel.in
. 🙂
tools/workspace/python/venv_expunge
line 11 at r24 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Missing period on sentence.
Done.
tools/workspace/python/venv_sync
line 7 at r19 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I still need to get back to you on that. The documentation of the flag is not sufficient for us to reason about what we need here, so I need to look under the hood.
Okay. I'll let you judge whether this needs to block the PR at this point. If not, from my end the only remaining maybe-issue is the comment re: tools/jupyter/test/jupyter_notebook_test.py
. I thereby kick the question whether this can be removed from draft status to you.
setup/mac/source_distribution/requirements-test.in
line 24 at r24 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Note to self: this should be fixed before merge. (But I want to let CI run, as this won't have a functional effect on anything.)
Done.
tools/workspace/venv/upgrade
line 15 at r17 (raw file):
For me, it is both succeeding and littering.
IIRC you were the one who changed this.
Hmm, so I did. I guess I had the wrong range selected for diffs when I was trying to see what you'd changed since my previous push. Sorry. 😳
...and yes, the relative path is why it had to go away (I remember now). Oh, well. I guess I'll just make the obvious fix to get rid of requirements.tmp
, then, and hope that's good enough.
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 r25, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
Incidentally, the lock files are already out of date, but I'm going to refrain from updating them again (in this PR) until/unless something else changes in order to not invalidate the previous CI results. |
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 r25, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
tools/jupyter/test/jupyter_notebook_test.py
line 11 at r24 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
tools/install/install_test.py
either uses the manifest (macOS) or not (not-macOS). This uses the manifest if available, and falls back.
I see. Yes, it would be better if this code checked sys.platform == "darwin"
instead.
tools/workspace/python/venv_sync
line 7 at r19 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Okay. I'll let you judge whether this needs to block the PR at this point. If not, from my end the only remaining maybe-issue is the comment re:
tools/jupyter/test/jupyter_notebook_test.py
. I thereby kick the question whether this can be removed from draft status to you.
Right. My main question is whether I want to try to sneak this in before the next release, or delay merging until afterwards. I'm still pondering that, too.
tools/workspace/venv/upgrade
line 15 at r17 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
For me, it is both succeeding and littering.
IIRC you were the one who changed this.
Hmm, so I did. I guess I had the wrong range selected for diffs when I was trying to see what you'd changed since my previous push. Sorry. 😳...and yes, the relative path is why it had to go away (I remember now). Oh, well. I guess I'll just make the obvious fix to get rid of
requirements.tmp
, then, and hope that's good enough.
Oops, how did I not notice the lingering tmp? Oh well.
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 jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
tools/jupyter/test/jupyter_notebook_test.py
line 11 at r24 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I see. Yes, it would be better if this code checked
sys.platform == "darwin"
instead.
Done.
tools/workspace/venv/upgrade
line 15 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Oops, how did I not notice the lingering tmp? Oh well.
...git status
filled with other junk, perhaps? 😉 Since my git status
(on the mac, anyway; the Ubuntu machine is another story!) is otherwise clean, it stood out.
@drake-jenkins-bot mac-arm-sonoma-unprovisioned-clang-bazel-experimental-everything-release please @jwnimmer-tri, for the jupyter test change, is the above enough, or do we also need the CMake and/or wheel 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.
I've decided that we will try to get this merged before our next release. The only open question is re: local = True
, which I will keep working on.
In the meantime, @mwoehlke-kitware please close this PR and open a different one with new PR number, so that we can start the next round of reviews with a clean slate. You can also squash the branch if you prefer.
... do we also need the CMake and/or wheel build?
It's enough for now. Before we land this PR, we'll do a final spin of the entire CI matrix.
Reviewed 1 of 1 files at r26, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
tools/jupyter/test/jupyter_notebook_test.py
line 1 at r26 (raw file):
import os
nit This import should be sys
not os
. (The test is currently failing CI.)
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 jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
tools/jupyter/test/jupyter_notebook_test.py
line 1 at r26 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit This import should be
sys
notos
. (The test is currently failing CI.)
Done.
Will reopen with a clean discussion history... |
Create a series of scripts to set up and manage a Python virtual environment that can be used to build (and test) Drake. Remove Python packages from macOS setup. Add dependencies to the virtual environment as needed.
This change is