[workspace] Update macOS to python@3.12#21013
Conversation
jwnimmer-tri
added
the
release notes: feature
|
Thanks @MarcusSorealheis! +@BetsyMcPhail or +@svenevs or +@mwoehlke-kitware whoever has time, please sign up for feature review here (and un-assign the others). +@jwnimmer-tri self-assigning for platform review. Most of the work for this upgrade will be refreshing our CI machines, which will take some days. |
|
@drake-jenkins-bot ok to test |
svenevs
changed the title
|
@MarcusSorealheis I did not do anything other than a quick pass, nominally things are looking good here! Please compare with the diff in #18262 (our last macOS upgrade). You're missing some things in the wheel build. For the wheel files in particular, that one is an "unprovisioned build" meaning that the
Specifically, once we get the dust settled here, I'll be rebaking our "provisioned images" on the backend. Until that is done, the pre-merge check mac-arm-ventura-clang-bazel-experimental-release will continue to fail on this PR (which is expected! just letting you know). Thanks for getting this started! Edit: whoops! apparently doing it in a code-block, the bot still picked it up! The first wheel build is here, expected to fail at least in the testing phase. |
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
-@BetsyMcPhail -@mwoehlke-kitware
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),svenevs
|
Also, re: wheel changes drake/tools/wheel/wheel_builder/macos.py Lines 19 to 22 in d899c03 when referring to #18262 you will not have instruction on this part. The wheel gets built for both 3.11 and 3.12 right now, your PR should be changing this to just have 3.12 now: python_targets = (
PythonTarget(3, 12),
) Down the road, when 3.13 is out, we'll add it to the list there (but the drake default will remain 3.12 until the next homebrew switch) Edit: or make it a list |
|
@svenevs Actually can we keep building wheels for both 3.11 and 3.12 on macOS? It seems like it would be possible, and of great value to our users. |
|
@jwnimmer-tri Yes, I think we could do that, in which case the change to |
|
The wheel builder does |
|
Ok, sorry for the confusion, #20616 changed things a lot more than I realized. We should discuss in f2f later today |
|
@drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-bazel-experimental-release please |
|
Hi @MarcusSorealheis, it looks like we're going to need to solve #8392 and possibly some other bits of CI changes before we're going to be able to make progress on this. High level overview is homebrew disabled using Thanks for helping get this started so quickly (and therefore helping us identify the need to fix some other parts)! |
Ah, that's a bummer but positive overall. I am happy to help with this in any way if you want or need. This project's vitality is very important to me. At a minimum, tonight I will get up to speed on that issue from 2018 and then test a few options out. If I get interesting data, I will share it as well. Thanks for being helpful. |
|
Maybe we're overthinking this. Using a |
MarcusSorealheis
force-pushed
the
fix/brew-python-3.12
branch
from
16315d3 to
c14ec65
Compare
I'm investigating now, but I made the change. I need to run it on my Mac. |
|
I'll need to schedule some time to dig into this issue tomorrow if someone doesn't beat me to it. A cursory look into the logs here shows a problem I am not seeing on my local Mac, as far as I can tell. I am on a different version than the CI. |
|
Note that the build labeled as The build that runs the setup scripts is: @drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-bazel-experimental-release please |
|
Is there anything else to do here @jwnimmer-tri besides reflashing the images? |
|
The build that we do need to be passing right now ( The bottom line is this: CI is just running setup scripts that we have in git. Our users will run the same scripts. Presumably, since they are failing in CI they will also fail for you when you run them on your local computer. Until |
MarcusSorealheis
force-pushed
the
fix/brew-python-3.12
branch
from
c14ec65 to
471179d
Compare
|
@drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-bazel-experimental-release please |
|
@jwnimmer-tri I read the related PEP (0668) and also the updated canonical spec and it seems that the latest update in this branch, provided it passes, should work. It also feels very clear that we should move off of it soon, and I will be happy to lend a hand with the migration to
However, I do consider this PR a step forward and something that should happen prior to virtualization. |
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee svenevs, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @MarcusSorealheis)
tools/workspace/styleguide/repository.bzl line 11 at r6 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, new PR in bound
@MarcusSorealheis FYI I had to rebase and force-push here, your local branch will be out of sync with what's on your fork's branch
Step 1 is up: RobotLocomotion/styleguide#54
@jwnimmer-tri I cannot assign reviewers there, please assign yourself if you have bandwidth. Otherwise I'll request on slack.
|
@drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-bazel-experimental-release please this one may actually pass! |
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee svenevs, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @MarcusSorealheis)
tools/jupyter/test/jupyter_notebook_test.py line 10 at r7 (raw file):
"""Ensures that `jupyter notebook` is installed (#12042).""" status = subprocess.call( args=[sys.executable, "-m", "notebook", "--help"])
working, waiting on test results, from commit message
-
We appear to be installing the wrong PyPI package!
- https://jupyter.org/install#jupyter-notebook
- https://pypi.org/project/jupyter/ (very old...)
- https://pypi.org/project/notebook/ (what we want...)
should be what we want now
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @MarcusSorealheis)
setup/mac/source_distribution/requirements-test-only.txt line 2 at r7 (raw file):
flask notebook
nit This one is already in the binary prereqs requirements.txt. So if this is really the one we want, then we can just drop this line.
It's fine to just fix that in the separate, ahead-of-time PR where we land this patch on its own, on master.
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @MarcusSorealheis)
setup/mac/source_distribution/requirements-test-only.txt line 2 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit This one is already in the binary prereqs
requirements.txt. So if this is really the one we want, then we can just drop this line.It's fine to just fix that in the separate, ahead-of-time PR where we land this patch on its own, on master.
gah! right you are... is there any reason that this PR cannot simply remove it?
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
Reviewed 6 of 8 files at r1, 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @MarcusSorealheis)
setup/mac/source_distribution/requirements-test-only.txt line 2 at r7 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
gah! right you are... is there any reason that this PR cannot simply remove it?
I want to land all of the "be compatible with python 3.12" as separate PRs ahead of time. The "switch from 3.11 to 3.12" should only be switching, not any fixing. That keeps the concerns separated, and makes revert wars and git history easier to work with.
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee svenevs, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @MarcusSorealheis)
setup/mac/source_distribution/requirements-test-only.txt line 2 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I want to land all of the "be compatible with python 3.12" as separate PRs ahead of time. The "switch from 3.11 to 3.12" should only be switching, not any fixing. That keeps the concerns separated, and makes revert wars and git history easier to work with.
Good plan, future us will be thankful 😉 => #21085
svenevs
force-pushed
the
fix/brew-python-3.12
branch
from
28f2155 to
a7537b9
Compare
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
I went ahead and cancelled the CI here, looks like we're one rebase away 😎 (jupyter fix)
Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee svenevs
tools/jupyter/test/jupyter_notebook_test.py line 10 at r7 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, waiting on test results, from commit message
We appear to be installing the wrong PyPI package!
- https://jupyter.org/install#jupyter-notebook
- https://pypi.org/project/jupyter/ (very old...)
- https://pypi.org/project/notebook/ (what we want...)
should be what we want now
working, thread block until #21085 lands and can rebase here.
tools/workspace/styleguide/repository.bzl line 11 at r6 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
Step 1 is up: RobotLocomotion/styleguide#54
@jwnimmer-tri I cannot assign reviewers there, please assign yourself if you have bandwidth. Otherwise I'll request on slack.
done, styleguide rebase is in here now
svenevs
force-pushed
the
fix/brew-python-3.12
branch
from
a7537b9 to
8080b24
Compare
|
@drake-jenkins-bot linux-jammy-unprovisioned-clang-bazel-experimental-debug please |
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),svenevs
tools/jupyter/test/jupyter_notebook_test.py line 10 at r7 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, thread block until #21085 lands and can rebase here.
done, after diagnosing, we realized the problem was pip install --user. Removing that, we have the jupyter command available and a small cruft removal of an unrelated jupyter python package 🙃
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee svenevs
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, 2 of 3 files at r8, 3 of 3 files at r9, all commit messages.
Reviewable status: LGTM missing from assignee svenevs
|
Thanks everyone for all the help here. A lot of big projects lose the vitality that the cutting edge ecosystem offers because they do not upgrade Python regularly. What a journey. |
|
Is the next bit of work here @jwnimmer-tri to flash the images? In that case, I would assume the PR will be on pause until then. |
|
Yup. The kitware folks need to prepare and deploy the Provisioned images, at which point we can concurrently merge this PR and switch Provisioned CI to use those image. |
svenevs
left a comment
svenevs
left a comment
There was a problem hiding this comment.
Reviewable status:
You'll have to blame @jwnimmer-tri 🙂 Drake evolves quickly, while at the same time maintaining stability. That's a hard balance! |
|
I've added a PR to stop breaking system Python packages, which was introduced by this PR: #21269 |
5 participants
This PR Addresses the first task in this #20294 (switch Drake's default to 3.12)
If it looks ok and it lands, I can take on the external examples task as well. My company needs it for our project. The hope is to contribute more and more to Drake's maintenance and innovation over time since we benefit from it.
This change is