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

Switch to Python 3.12 on macOS #285

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Switch to Python 3.12 on macOS #285

merged 1 commit into from
Mar 6, 2024

Conversation

svenevs
Copy link
Contributor

@svenevs svenevs commented Mar 1, 2024

Relates: RobotLocomotion/drake#20294

Just getting this teed up and ready.


This change is Reviewable

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, platform LGTM missing (waiting on @svenevs)


drake_cmake_external/CMakeLists.txt line 38 at r1 (raw file):

  NO_DEFAULT_PATH
)
find_package(PythonInterp ${FIND_PYTHON_INTERP_VERSION} MODULE REQUIRED)

working, as it turns out, the way we have things setup here is not exactly ideal IMO. I was surprised CI passed job log here

-- Found PythonInterp: /usr/local/bin/python3.11 (found suitable exact version "3.11.8") 
CMake Warning (dev) at CMakeLists.txt:26 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonLibs: /usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.11/lib/libpython3.11.dylib (found suitable exact version "3.11.8") 
CMake Deprecation Warning at /opt/drake/lib/cmake/pybind11/pybind11-config.cmake:12 (cmake_policy):
  Compatibility with CMake < 3.5 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.
Call Stack (most recent call first):
  src/simple_bindings/CMakeLists.txt:3 (find_package)


CMake Warning (dev) at /usr/local/Cellar/cmake/3.28.3/share/cmake/Modules/CMakeFindDependencyMacro.cmake:76 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

Call Stack (most recent call first):
  /opt/drake/lib/cmake/pybind11/pybind11-config.cmake:27 (find_dependency)
  src/simple_bindings/CMakeLists.txt:3 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonInterp: /usr/local/bin/python3.11 (found version "3.11.8") 
CMake Warning (dev) at /usr/local/Cellar/cmake/3.28.3/share/cmake/Modules/CMakeFindDependencyMacro.cmake:76 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

Call Stack (most recent call first):
  /opt/drake/lib/cmake/pybind11/pybind11-config.cmake:28 (find_dependency)
  src/simple_bindings/CMakeLists.txt:3 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonLibs: /usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.11/lib/libpython3.11.dylib (found version "3.11.8") 
CMake Warning (dev) at /opt/drake/lib/cmake/pybind11/FindPythonLibsNew.cmake:98 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

Call Stack (most recent call first):
  /opt/drake/lib/cmake/pybind11/pybind11Tools.cmake:50 (find_package)
  /opt/drake/lib/cmake/pybind11/pybind11-config.cmake:132 (include)
  src/simple_bindings/CMakeLists.txt:3 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonInterp: /usr/local/bin/python3.11 (found suitable version "3.11.8", minimum required is "3.6") 
-- Found PythonLibs: /usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.11/lib/libpython3.11.dylib
-- Configuring done (4.2s)
-- Generating done (0.2s)
-- Build files have been written to: /Users/runner/work/drake-external-examples/drake-external-examples/drake_cmake_installed/build

The CMake code should probably be updated (possibly ahead of this and then rebase this to just update to 3.12). We should likely be doing find_package(Python3 COMPONENTS Interpreter Development VERSION ${variable_with_minimum_version_eg_312} EXACT REQUIRED)?

I don't want to keep using the FIND_PYTHON_INTERP_VERSION variable, ideally rename it. But https://cmake.org/cmake/help/latest/module/FindPython3.html is what we should switch to. @mwoehlke-kitware may have suggestions, but will likely agree with the update.

Question for @jwnimmer-tri is the usage of EXACT. If we were using it currently, this CI would have failed when I updated it to 3.12. As it currently stands, it somehow found something we did not want it to. Does that seem right to you (to fail)?


scripts/setup/mac/install_prereqs line 32 at r1 (raw file):


pip3.12 install \
    --upgrade --user --break-system-packages \

working, we removed --user in the drake PR and should do so here too

Copy link
Contributor

@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 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, platform LGTM missing (waiting on @svenevs)


drake_cmake_external/CMakeLists.txt line 38 at r1 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

working, as it turns out, the way we have things setup here is not exactly ideal IMO. I was surprised CI passed job log here

-- Found PythonInterp: /usr/local/bin/python3.11 (found suitable exact version "3.11.8") 
CMake Warning (dev) at CMakeLists.txt:26 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonLibs: /usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.11/lib/libpython3.11.dylib (found suitable exact version "3.11.8") 
CMake Deprecation Warning at /opt/drake/lib/cmake/pybind11/pybind11-config.cmake:12 (cmake_policy):
  Compatibility with CMake < 3.5 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.
Call Stack (most recent call first):
  src/simple_bindings/CMakeLists.txt:3 (find_package)


CMake Warning (dev) at /usr/local/Cellar/cmake/3.28.3/share/cmake/Modules/CMakeFindDependencyMacro.cmake:76 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

Call Stack (most recent call first):
  /opt/drake/lib/cmake/pybind11/pybind11-config.cmake:27 (find_dependency)
  src/simple_bindings/CMakeLists.txt:3 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonInterp: /usr/local/bin/python3.11 (found version "3.11.8") 
CMake Warning (dev) at /usr/local/Cellar/cmake/3.28.3/share/cmake/Modules/CMakeFindDependencyMacro.cmake:76 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

Call Stack (most recent call first):
  /opt/drake/lib/cmake/pybind11/pybind11-config.cmake:28 (find_dependency)
  src/simple_bindings/CMakeLists.txt:3 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonLibs: /usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.11/lib/libpython3.11.dylib (found version "3.11.8") 
CMake Warning (dev) at /opt/drake/lib/cmake/pybind11/FindPythonLibsNew.cmake:98 (find_package):
  Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
  are removed.  Run "cmake --help-policy CMP0148" for policy details.  Use
  the cmake_policy command to set the policy and suppress this warning.

Call Stack (most recent call first):
  /opt/drake/lib/cmake/pybind11/pybind11Tools.cmake:50 (find_package)
  /opt/drake/lib/cmake/pybind11/pybind11-config.cmake:132 (include)
  src/simple_bindings/CMakeLists.txt:3 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Found PythonInterp: /usr/local/bin/python3.11 (found suitable version "3.11.8", minimum required is "3.6") 
-- Found PythonLibs: /usr/local/opt/[email protected]/Frameworks/Python.framework/Versions/3.11/lib/libpython3.11.dylib
-- Configuring done (4.2s)
-- Generating done (0.2s)
-- Build files have been written to: /Users/runner/work/drake-external-examples/drake-external-examples/drake_cmake_installed/build

The CMake code should probably be updated (possibly ahead of this and then rebase this to just update to 3.12). We should likely be doing find_package(Python3 COMPONENTS Interpreter Development VERSION ${variable_with_minimum_version_eg_312} EXACT REQUIRED)?

I don't want to keep using the FIND_PYTHON_INTERP_VERSION variable, ideally rename it. But https://cmake.org/cmake/help/latest/module/FindPython3.html is what we should switch to. @mwoehlke-kitware may have suggestions, but will likely agree with the update.

Question for @jwnimmer-tri is the usage of EXACT. If we were using it currently, this CI would have failed when I updated it to 3.12. As it currently stands, it somehow found something we did not want it to. Does that seem right to you (to fail)?

The objective of DEE is to give users a template recipe to copy. The recipe should be as durable as possible for what actual users would experience. How it operates in CI is really secondary to that goal. Ideally CI would yell when users are going to be sad, but the most important thing we want is to have something that is the best idea to copy + paste.

It doesn't really matter which python version we end up with. Either 3.11 or 3.12 are supposed to work. What we probably want is to always use $(homebrew)/bin/python3 on macOS, whatever that resolves to. So anytime homebrew upgrades, the user will follow suit. We should not try to pin the minor versions, we should just let homebrew decide.

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.

Reviewed 1 of 3 files at r1.
Reviewable status: 1 unresolved discussion, platform LGTM missing (waiting on @svenevs)


drake_cmake_external/CMakeLists.txt line 38 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The objective of DEE is to give users a template recipe to copy. The recipe should be as durable as possible for what actual users would experience. How it operates in CI is really secondary to that goal. Ideally CI would yell when users are going to be sad, but the most important thing we want is to have something that is the best idea to copy + paste.

It doesn't really matter which python version we end up with. Either 3.11 or 3.12 are supposed to work. What we probably want is to always use $(homebrew)/bin/python3 on macOS, whatever that resolves to. So anytime homebrew upgrades, the user will follow suit. We should not try to pin the minor versions, we should just let homebrew decide.

done, what I'd like to do is let this one merge and solve RobotLocomotion/drake#21099 at a later date

@svenevs svenevs force-pushed the feat/python-3.12 branch from de1d080 to 475fee1 Compare March 5, 2024 23:38
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.

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [svenevs] (waiting on @svenevs)


scripts/setup/mac/install_prereqs line 32 at r1 (raw file):

Previously, svenevs (Stephen McDowell) wrote…

working, we removed --user in the drake PR and should do so here too

done

Copy link
Contributor

@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 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [svenevs] (waiting on @svenevs)

@svenevs svenevs merged commit 66e82c8 into main Mar 6, 2024
6 checks passed
@svenevs svenevs deleted the feat/python-3.12 branch March 6, 2024 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants