Fix CasADi in config mode, and use pybind11's FindPython mode
#62
Conversation
This partially reverts commit 37a00ad.
| @@ -1,5 +1,5 @@ | |||
| cmake_minimum_required(VERSION 3.13) | |||
| cmake_policy(SET CMP0074 NEW) | |||
There was a problem hiding this comment.
This policy is too old; it prevents us from using https://cmake.org/cmake/help/latest/module/FindPython.html as it gets disabled when this policy is enabled.
| @@ -1,5 +1,5 @@ | |||
| cmake_minimum_required(VERSION 3.13) | |||
| cmake_policy(SET CMP0074 NEW) | |||
| cmake_minimum_required(VERSION 3.26) | |||
There was a problem hiding this comment.
The earliest version of CMake to support the new FindPython mode is 3.26. We don't pin CMake in pyproject.toml, and our use of CMake isn't too heavy either, so the current 4.1.0 works fine. Please let me know if you'd like me to upper-pin this in pyproject.toml as well, though I think it shouldn't be needed.
| # casadi seems to compile without the newer versions of std::string | ||
| add_compile_definitions(_GLIBCXX_USE_CXX11_ABI=0) | ||
|
|
||
| set(PYBIND11_FINDPYTHON ON) |
There was a problem hiding this comment.
This will help with WASM builds :)
| execute_process( | ||
| COMMAND "${Python_EXECUTABLE}" -c | ||
| "import pathlib, casadi; print(pathlib.Path(next(iter(casadi.__path__))).joinpath('cmake'))" | ||
| OUTPUT_VARIABLE CASADI_DIR | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE) |
There was a problem hiding this comment.
The idea is that the previous version of this command, i.e.,
import os; import sysconfig; print(os.path.join(sysconfig.get_path('purelib'), 'casadi', 'cmake'))thinks that it finds CasADi in something like /Users/agriyakhetarpal/Desktop/pybammsolvers/venv/lib/python3.12/site-packages/casadi/cmake, which will never exist – this is because it just constructs the path for where CasADi would be placed.
What we needed to do here is to import casadi – this is always guaranteed to work as we require casadi as a build-time dependency, and hence is available in sys.modules during build isolation. I switched to this, and we correctly get the build-time CasADi:
/private/var/folders/b3/2bq1m1_50bs4c7305j8vxcqr0000gn/T/pip-build-env-ihqwpmd7/overlay/lib/python3.12/site-packages/casadi/cmake
There was a problem hiding this comment.
Hi @MarcBerliner, I believe that with these changes, we should be able to perform an editable/non-editable installation pretty seamlessly, with the previous explicit specification of the CasADi library and include directories no longer needed. Could you please try this on your macOS machine? Thank you!
|
The failing unit and integration tests are unrelated; the code compiles correctly. I know the fix as well; I'll push it later today. It is more important for the "Build wheels" job(s) to pass. :) |
|
Argh, it looks like |
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the CasADi configuration in CMake by utilizing CasADi's config mode and pybind11's FindPython mode. The changes remove manual path discovery logic and rely on CMake's package configuration system for more robust dependency management.
- Removes custom CasADi include/library directory discovery code from setup.py
- Switches to using
find_package(casadi CONFIG)in CMake for both Python and non-Python CasADi builds - Enables pybind11's FindPython mode and updates build configurations to use
pybind11[global]
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| setup.py | Removes manual CasADi path discovery and simplifies build configuration |
| CMakeLists.txt | Streamlines CasADi package finding using config mode and enables pybind11 FindPython |
| .github/workflows/build_wheels.yml | Updates build frontend and pybind11 installation to use global mode |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| message(FATAL_ERROR "Could not find CasADi include directory") | ||
| endif () | ||
| execute_process( | ||
| COMMAND "${Python_EXECUTABLE}" -c |
There was a problem hiding this comment.
The variable Python_EXECUTABLE is not defined. Since pybind11's FindPython mode is enabled, this should be Python3_EXECUTABLE or you need to ensure Python_EXECUTABLE is properly set from the FindPython module.
| COMMAND "${Python_EXECUTABLE}" -c | |
| COMMAND "${Python3_EXECUTABLE}" -c |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is incorrect; FindPython defines Python_EXECUTABLE and not Python3_EXECUTABLE.
|
I guess Copilot is useless at identifying fixes for failing CI jobs :P |
|
It looks like |
|
Okay, switching to the cibuildwheel action has resolved the issues with the wheel builds. However, there's another problem during the wheel repair process for me to investigate. We recently encountered a similar issue with Python paths in cibuildwheel when it's used outside of the action, specifically with setup-python + pip-installing it, as we were doing in this case (see pypa/cibuildwheel#2566). |
Building up on and partially reverting the changes in #61; I was able to figure out the problem CMake was having with the builds for CasADi in
configmode, and this PR cuts down on a lot of unnecessary configuration. Comments below annotate the reasoning for changes.