Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #362 +/- ##
=======================================
Coverage 87.09% 87.09%
=======================================
Files 43 43
Lines 3860 3860
=======================================
Hits 3362 3362
Misses 498 498 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
📄 Documentation for this branch is available at: https://ncar.github.io/musica/branch/dlopen/ |
K20shores
requested review from
boulderdaze,
Copilot,
mattldawson,
montythind,
mwaxmonsky and
sjsprecious
There was a problem hiding this comment.
Pull Request Overview
This PR aims to correct the Python wheel builds for Linux while adding separate pybind11 targets for GPU and CPU support and updating various project configurations.
- Updated build configurations in pyproject.toml to support only specific python versions.
- Refactored import paths in multiple Python files and updated GPU detection logic.
- Updated CI workflows and CMake configurations to align with new dependency and OS support policies.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated scikit-build overrides and build configuration. |
| musica/types.py | Adjusted import paths and added inline comments regarding state creation. |
| musica/tools/repair_wheel_gpu.sh | Revised GPU wheel repair script to handle missing .so files. |
| musica/tools/prepare_build_environment_linux.sh | Added wget installation and updated CUDA package versions. |
| musica/test/* | Updated import paths and fixes in CUDA detection tests. |
| musica/gpu_binding.cpp, musica/cuda*.cpp | Added new GPU binding code and updated the CUDA API. |
| musica/cpu_binding.cpp | Modified CPU bindings to include CUDA functions. |
| musica/init.py | Updated dynamic backend selection logic for GPU/CPU. |
| CMakeLists.txt & setup_musica_target.cmake | Reworked CMake configuration for Python extension modules. |
| CI workflows (.github/workflows/windows.yml) | Updated Windows runner and build settings. |
| Dockerfile.wheel, README.md, .dockerignore | Updated packaging instructions and Docker configuration. |
Comments suppressed due to low confidence (2)
musica/types.py:141
- Typo in the comment: 'whould' should be corrected to 'would'.
# # we whould get two states, but we have one
musica/cpu_binding.cpp:10
- Including CUDA bindings in the CPU module may be unexpected; please verify that exposing CUDA functions in the CPU target is intentional.
void bind_cuda(py::module_ &);
mattldawson
left a comment
mattldawson
left a comment
There was a problem hiding this comment.
nice! just one very minor comment
mwaxmonsky
left a comment
mwaxmonsky
left a comment
There was a problem hiding this comment.
Looks great! Just some formatting comments.
| set(MUSICA_MICM_SOURCES | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/micm/micm.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/micm/micm_c_interface.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/micm/state.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/micm/state_c_interface.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/micm/parse.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/micm/v0_parse.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/micm/v1_parse.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/micm/cuda_availability.cpp | ||
| ) |
There was a problem hiding this comment.
In the PR description it says, "Collects all of the sources needed to compile musica into a list." I'm guessing that refers to these changes? Just curious about the specific reason for doing that.
Everything else looks good to me.
There was a problem hiding this comment.
Yes, this is that.
I'm going to write up a How To document soon with more detail. Also, please see the updated picture in the PR description. That, plus my comment below, may help clear this up.
Before this PR, the pybind11 target was using target_link_libraries(_musica PUBLIC musica::musica). Pypi only allows us to upload a single wheel file per architecture and Python version. To support GPUs on Linux, we need to turn on the compile options for GPU support. Initially, we were creating the one _musica.cpython.*.so file for Linux and rewrote the RPATH of the executable to look for the nvidia libraries inside the Python site packages directory. This worked for Linux users with a GPU and CUDA libraries. However, Linux users without a GPU and CUDA libraries were then unable to import our Python library. This is because when the _musica.cpython.*.so was loaded by Python, it would load musica.so, which depended on the CUDA libraries.
The first thing I tried was to load the CUDA libraries dynamically with dlopen. This would have worked if we didn't need to compile musica with CUDA support turned on. When musica.so was loaded, we got a runtime link error because some of our symbols in musica and micm depended on the CUDA libraries being loaded. To get around this, we truly would need a plugin architecture that would allow us to load some musica library with GPU symbols separate from a library with CPU symbols, but we only have one build so both symbols must be mixed.
To get around this problem, we now create two Python libraries (see the picture above). One library is CPU only, one is CPU+GPU. Which is loaded by Python is determined by if the necessary cuda libraries are installed into the Python site packages directory. We still have to rewrite the RPATH of the _musica_gpu target, but now if nvidia libraries aren't find in site packages, a Linux user can still import musica and run.
There was a problem hiding this comment.
Thank you for the detailed information. I see why you now build two separate python libraries
Closes #353
_musica_gpu), one without (_musica).