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

[workspace] Update macOS to python@3.11 #18262

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

@svenevs svenevs added the release notes: feature This pull request contains a new feature label Nov 4, 2022
@svenevs
Copy link
Contributor Author

svenevs commented Nov 4, 2022

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-bazel-experimental-snopt-mosek-packaging please.
@drake-jenkins-bot mac-x86-big-sur-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-bazel-experimental-snopt-mosek-packaging please.

Copy link
Contributor Author

@svenevs svenevs 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 (waiting on @svenevs)


tools/workspace/python/repository.bzl line 57 at r1 (raw file):

    # - Update URLs on doc/_pages/pip.md (`cpXY-cpXY` components), and
    # - Tables on from_source.md and installation.md (python version number).
    "macos_wheel": ["3.11"],

Working, local wheel build failure

ERROR: /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/external/lcm/BUILD.bazel:175:10: Compiling lcm-python/module.c failed: (Aborted): wrapped_clang failed: error executing command external/local_config_cc/wrapped_clang '-D_FORTIFY_SOURCE=1' -fstack-protector -fcolor-diagnostics -Wall -Wthread-safety -Wself-assign -fno-omit-frame-pointer -g0 -O2 -DNDEBUG '-DNS_BLOCK_ASSERTIONS=1' ... (remaining 50 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
external/lcm/lcm-python/module.c:46:34: error: expression is not assignable
    Py_TYPE(&pylcmeventlog_type) = &PyType_Type;
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
external/lcm/lcm-python/module.c:47:26: error: expression is not assignable
    Py_TYPE(&pylcm_type) = &PyType_Type;
    ~~~~~~~~~~~~~~~~~~~~ ^
external/lcm/lcm-python/module.c:48:39: error: expression is not assignable
    Py_TYPE(&pylcm_subscription_type) = &PyType_Type;

Copy link
Contributor Author

@svenevs svenevs 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: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers


tools/workspace/python/repository.bzl line 58 at r1 (raw file):

    # - Tables on from_source.md and installation.md (python version number).
    "macos_wheel": ["3.11"],
    "manylinux": ["3.8", "3.9", "3.10"],

Working, failure that hit drake external examples too

[10:21:57 PM]  external/pybind11/include/pybind11/detail/type_caster_base.h:540:26: error: member access into incomplete type 'PyFrameObject' (aka '_frame')
[10:21:57 PM]              frame = frame->f_back;
[10:21:57 PM]                           ^
[10:21:57 PM]  external/python/include/_opt_homebrew_opt_python@3.11_Frameworks_Python.framework_Versions_3.11_include_python3.11/pytypedefs.h:22:16: note: forward declaration of '_frame'
[10:21:57 PM]  typedef struct _frame PyFrameObject;

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri 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: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @svenevs)


tools/workspace/python/repository.bzl line 57 at r1 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

Working, local wheel build failure

ERROR: /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/external/lcm/BUILD.bazel:175:10: Compiling lcm-python/module.c failed: (Aborted): wrapped_clang failed: error executing command external/local_config_cc/wrapped_clang '-D_FORTIFY_SOURCE=1' -fstack-protector -fcolor-diagnostics -Wall -Wthread-safety -Wself-assign -fno-omit-frame-pointer -g0 -O2 -DNDEBUG '-DNS_BLOCK_ASSERTIONS=1' ... (remaining 50 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
external/lcm/lcm-python/module.c:46:34: error: expression is not assignable
    Py_TYPE(&pylcmeventlog_type) = &PyType_Type;
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
external/lcm/lcm-python/module.c:47:26: error: expression is not assignable
    Py_TYPE(&pylcm_type) = &PyType_Type;
    ~~~~~~~~~~~~~~~~~~~~ ^
external/lcm/lcm-python/module.c:48:39: error: expression is not assignable
    Py_TYPE(&pylcm_subscription_type) = &PyType_Type;

FYI See https://docs.python.org/3/whatsnew/3.11.html near Py_SET_TYPE() for a compatibility macro we'll need to use for LCM.

For expediency, we could do it as a drake/tools/workspace/lcm/patches/... file and submit it upstream later.


tools/workspace/python/repository.bzl line 58 at r1 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

Working, failure that hit drake external examples too

[10:21:57 PM]  external/pybind11/include/pybind11/detail/type_caster_base.h:540:26: error: member access into incomplete type 'PyFrameObject' (aka '_frame')
[10:21:57 PM]              frame = frame->f_back;
[10:21:57 PM]                           ^
[10:21:57 PM]  external/python/include/_opt_homebrew_opt_python@3.11_Frameworks_Python.framework_Versions_3.11_include_python3.11/pytypedefs.h:22:16: note: forward declaration of '_frame'
[10:21:57 PM]  typedef struct _frame PyFrameObject;

FYI This one like a missing #include; hopefully will be easy to fix.

@svenevs
Copy link
Contributor Author

svenevs commented Nov 4, 2022

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-bazel-experimental-snopt-mosek-packaging please.

Copy link
Contributor Author

@svenevs svenevs 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: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @svenevs)


tools/workspace/python/repository.bzl line 57 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI See https://docs.python.org/3/whatsnew/3.11.html near Py_SET_TYPE() for a compatibility macro we'll need to use for LCM.

For expediency, we could do it as a drake/tools/workspace/lcm/patches/... file and submit it upstream later.

I included a patch, upstreaming it is a little less clear (does lcm-python support python 2?)


tools/workspace/python/repository.bzl line 58 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI This one like a missing #include; hopefully will be easy to fix.

RobotLocomotion/pybind11#60

Pushed a commit with a test in that. I'm getting a really bizarre issue, I think I'm missing one update somewhere. All of drake builds but then in the final wheel stage locally (without python 3.9 installed) it's trying to create python 3.9 names.

Successfully installed delocate-0.10.3 packaging-21.3 pyparsing-3.0.9 setuptools-65.5.0 typing-extensions-4.4.0 wheel-0.38.1
WARNING: You are using pip version 21.2.4; however, version 22.3 is available.
You should consider upgrading via the '/opt/drake-wheel-build/python/bin/python3 -m pip install --upgrade pip' command.
Using packages=['pydrake', 'drake', 'pydrake.manipulation', 'pydrake.multibody', 'pydrake.common', 'pydrake.systems', 'pydrake.examples', 'pydrake.solvers', 'pydrake.visualization']
running bdist_wheel
running build
running build_py
creating build
creating build/lib.macosx-10.9-universal2-cpython-39
creating build/lib.macosx-10.9-universal2-cpython-39/pydrake

Perhaps CI will result in something different.

@svenevs svenevs force-pushed the fix/brew-python-3.11 branch from 6d17d37 to b8d92f0 Compare November 7, 2022 14:25
@svenevs
Copy link
Contributor Author

svenevs commented Nov 7, 2022

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-bazel-experimental-snopt-mosek-packaging please.

@svenevs svenevs force-pushed the fix/brew-python-3.11 branch from b8d92f0 to e7e1edf Compare November 7, 2022 16:53
@svenevs
Copy link
Contributor Author

svenevs commented Nov 7, 2022

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please.

Copy link
Contributor Author

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

OK macOS wheel build issues resolved, linux still works as expected. Closer!

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @svenevs)


tools/workspace/python/repository.bzl line 57 at r1 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

I included a patch, upstreaming it is a little less clear (does lcm-python support python 2?)

LCM patch currently has no PR, AFAICT the patch here will break python 2 support and the status of the repo is unclear to me.


tools/workspace/python/repository.bzl line 58 at r1 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

RobotLocomotion/pybind11#60

Pushed a commit with a test in that. I'm getting a really bizarre issue, I think I'm missing one update somewhere. All of drake builds but then in the final wheel stage locally (without python 3.9 installed) it's trying to create python 3.9 names.

Successfully installed delocate-0.10.3 packaging-21.3 pyparsing-3.0.9 setuptools-65.5.0 typing-extensions-4.4.0 wheel-0.38.1
WARNING: You are using pip version 21.2.4; however, version 22.3 is available.
You should consider upgrading via the '/opt/drake-wheel-build/python/bin/python3 -m pip install --upgrade pip' command.
Using packages=['pydrake', 'drake', 'pydrake.manipulation', 'pydrake.multibody', 'pydrake.common', 'pydrake.systems', 'pydrake.examples', 'pydrake.solvers', 'pydrake.visualization']
running bdist_wheel
running build
running build_py
creating build
creating build/lib.macosx-10.9-universal2-cpython-39
creating build/lib.macosx-10.9-universal2-cpython-39/pydrake

Perhaps CI will result in something different.

Working, the patch here works, we can proceed with merging RobotLocomotion/pybind11#60 and then I can update the repo rules to use a different commit after it merges. Not sure what's going on with the CI there.

@svenevs svenevs force-pushed the fix/brew-python-3.11 branch from e7e1edf to b818987 Compare November 8, 2022 16:03
Copy link
Contributor Author

@svenevs svenevs 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: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri)


tools/workspace/python/repository.bzl line 58 at r5 (raw file):

    #   under "Nightly Releases".
    # - Wheel URLs in tools/release_engineering/download_release_candidate.py
    #   (`cpXY-cpXY` components).

Working, I think #18148 is going to land first, this PR needs to update tools/release_engineering/download_release_candidate.py wheel urls.

@sherm1
Copy link
Member

sherm1 commented Nov 10, 2022

Is this ready to have a feature reviewer assigned?

@svenevs
Copy link
Contributor Author

svenevs commented Nov 10, 2022

Yes, we need TRI to decide how to handle the patches. I think we should merge the pybind11 PR (part of this review) and keep the lcm patch local to drake.

I need to update some urls in the release candidate download script still.

@jwnimmer-tri
Copy link
Collaborator

From f2f:

  • The LCM patchfile can land in Drake for now.
  • This PR should point to the pybind11 PR temporary sha, to prove it works OK.
  • We can and should land both the pybind11 PR and a Drake sha bump PR of pybind11, ahead of this 3.11 PR.

@svenevs svenevs force-pushed the fix/brew-python-3.11 branch 2 times, most recently from c8d8a2b to b7f717a Compare November 10, 2022 16:32
@svenevs
Copy link
Contributor Author

svenevs commented Nov 10, 2022

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please.

Copy link
Contributor Author

@svenevs svenevs 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: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri)

a discussion (no related file):
Working, found something in CMakeLists.txt that needs to update with this PR and python versions


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r9, 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" (waiting on @svenevs)

@svenevs svenevs force-pushed the fix/brew-python-3.11 branch from de6850f to a343497 Compare February 2, 2023 17:09
@svenevs
Copy link
Contributor Author

svenevs commented Feb 2, 2023

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please.

@svenevs
Copy link
Contributor Author

svenevs commented Feb 2, 2023

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please.

Retesting now that RobotLocomotion/drake-ci#197 has landed.

@svenevs
Copy link
Contributor Author

svenevs commented Feb 14, 2023

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please.

@svenevs svenevs force-pushed the fix/brew-python-3.11 branch from 213b551 to abc169b Compare February 14, 2023 02:33
@svenevs
Copy link
Contributor Author

svenevs commented Feb 14, 2023

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot mac-x86-monterey-unprovisioned-clang-wheel-experimental-snopt-mosek-release please.
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please.

Copy link
Contributor Author

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

-(status: do not merge)

Reviewed 1 of 7 files at r10.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)


doc/_pages/pip.md line 120 at r7 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

Works for me, FYI I may not have access to a computer next week in which case you should be able to push to the branch / finalize directly.

Done, Homebrew/homebrew-core#114154 has now landed so we should be ready to switchover here. I have the date set for 2023-02-15 AKA if this merges tomorrow (2023-02-14) then nightly will produce the desired artifacts here.

Copy link
Contributor Author

@svenevs svenevs 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: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)


doc/_pages/pip.md line 120 at r7 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

Done, Homebrew/homebrew-core#114154 has now landed so we should be ready to switchover here. I have the date set for 2023-02-15 AKA if this merges tomorrow (2023-02-14) then nightly will produce the desired artifacts here.

Or does drake desire to wait until Homebrew/homebrew-core#123165 lands?

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 7 files at r10, 2 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @svenevs)


doc/_pages/pip.md line 120 at r7 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

Or does drake desire to wait until Homebrew/homebrew-core#123165 lands?

I can't tell exactly what that last PR changes.

The invariant we want for Drake is that when a user runs python3 on mac (i.e., /usr/local/bin/python3), the python version it runs is the python version we support.

Copy link
Contributor Author

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

+@rpoyner-tri for platform review please 🙂

Reviewed 3 of 9 files at r1, 1 of 3 files at r4, 1 of 2 files at r6, 2 of 6 files at r7, 4 of 6 files at r9, 4 of 7 files at r10, 2 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform) (waiting on @rpoyner-tri)


doc/_pages/pip.md line 120 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I can't tell exactly what that last PR changes.

The invariant we want for Drake is that when a user runs python3 on mac (i.e., /usr/local/bin/python3), the python version it runs is the python version we support.

OK, those aliases were updated in the original PR we were tracking which has now merged so we're good to go. I also don't understand what the last PR is doing, but brew has switched over now.

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 9 files at r1, 1 of 3 files at r4, 1 of 2 files at r6, 2 of 6 files at r7, 4 of 6 files at r9, 4 of 7 files at r10, 2 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform) (waiting on @svenevs)

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @svenevs)

@rpoyner-tri rpoyner-tri merged commit 500dc6a into RobotLocomotion:master Feb 14, 2023
@svenevs svenevs deleted the fix/brew-python-3.11 branch February 14, 2023 18:06
svenevs added a commit to svenevs/drake that referenced this pull request Feb 14, 2023
Follow-up to RobotLocomotion#18262.

- deprecation_autocomplete_test: add __getstate__(, refs:
  https://docs.python.org/3/library/pickle.html#object.__getstate__

  Changed in version 3.11: Added the default implementation of the
  __getstate__() method in the object class.
- styleguide: temporarily mark the failing test as manual to unblock
  CI, proper fix to adjust import paths TBD.
svenevs added a commit to svenevs/drake that referenced this pull request Feb 14, 2023
Follow-up to RobotLocomotion#18262.

- deprecation_autocomplete_test: add __getstate__(, refs:
  https://docs.python.org/3/library/pickle.html#object.__getstate__

  Changed in version 3.11: Added the default implementation of the
  __getstate__() method in the object class.
- styleguide: temporarily mark the failing test as manual to unblock
  CI, proper fix to adjust import paths TBD.
svenevs added a commit that referenced this pull request Feb 14, 2023
Follow-up to #18262.

- deprecation_autocomplete_test: add __getstate__(, refs:
  https://docs.python.org/3/library/pickle.html#object.__getstate__

  Changed in version 3.11: Added the default implementation of the
  __getstate__() method in the object class.
- styleguide: temporarily mark the failing test as manual to unblock
  CI, proper fix to adjust import paths TBD.
marcoag pushed a commit to marcoag/drake that referenced this pull request Mar 8, 2023
marcoag pushed a commit to marcoag/drake that referenced this pull request Mar 8, 2023
Follow-up to RobotLocomotion#18262.

- deprecation_autocomplete_test: add __getstate__(, refs:
  https://docs.python.org/3/library/pickle.html#object.__getstate__

  Changed in version 3.11: Added the default implementation of the
  __getstate__() method in the object class.
- styleguide: temporarily mark the failing test as manual to unblock
  CI, proper fix to adjust import paths TBD.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants