Conversation
|
📄 Documentation for this branch is available at: https://ncar.github.io/musica/branch/windows-arm/ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #663 +/- ##
==========================================
- Coverage 76.10% 75.91% -0.20%
==========================================
Files 105 107 +2
Lines 7768 7864 +96
==========================================
+ Hits 5912 5970 +58
- Misses 1856 1894 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7e306a1 to
f3868ac
Compare
There was a problem hiding this comment.
Pull request overview
This PR enables building Python wheels for Windows ARM64 by updating pybind11 to v3.0.1 and addressing flang compiler compatibility issues. The changes consolidate test data paths, improve memory management in Fortran interface code, and add platform-specific build configurations.
Key changes:
- Updates pybind11 from v3.0.0 to v3.0.1 to fix visibility flag issues on Windows ARM64
- Fixes flang compiler compatibility by improving pointer initialization and deallocation patterns in Fortran interface code
- Consolidates test data from
test/data/intoconfigs/directory for consistent access across test suites - Makes ussa1976 an optional test dependency (unavailable on Windows ARM64 due to netcdf4 limitations)
Reviewed changes
Copilot reviewed 26 out of 39 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tuvx/interface_radiator_map.F90 | Adds explicit pointer initialization and safer deallocation logic for flang compatibility |
| src/tuvx/interface_profile_map.F90 | Adds explicit pointer initialization and safer deallocation logic for flang compatibility |
| src/tuvx/interface_grid_map.F90 | Adds early error_code initialization and safer deallocation logic for flang compatibility |
| src/test/unit/util.cpp | Updates test config file path to use consolidated configs directory |
| src/test/unit/tuvx/tuvx_run_from_config.cpp | Updates test config file paths to use consolidated configs directory |
| src/test/CMakeLists.txt | Removes duplicate test data copying, relies on single configs directory |
| python/test/unit/test_util_full_mechanism.py | Updates test data for full mechanism configuration |
| python/test/unit/test_parser.py | Updates config file path to use consolidated configs directory |
| python/test/integration/test_tuvx.py | Updates config file paths and removes unnecessary directory changes |
| python/test/integration/test_sulfate_box_model.py | Adds platform-specific skip for Windows ARM64 due to ussa1976 unavailability |
| python/test/integration/test_carma_aluminum.py | Adds platform-specific skip for Windows ARM64 due to ussa1976 unavailability |
| python/test/examples/v1/full_configuration/full_configuration.yaml | File deleted, configuration moved to configs directory |
| python/test/examples/v1/full_configuration/full_configuration.json | File deleted, configuration moved to configs directory |
| pyproject.toml | Moves ussa1976 to optional test dependencies, adds Windows ARM64 build overrides, configures cibuildwheel testing |
| fortran/test/unit/util.F90 | Updates test config file path to use consolidated configs directory |
| fortran/test/unit/tuvx.F90 | Updates test config file paths to use consolidated configs directory |
| configs/v1/full_configuration/full_configuration.yaml | Updates reaction configurations with corrected species references |
| configs/v1/full_configuration/full_configuration.json | Updates reaction configurations with corrected species references |
| configs/util_index_mapping_from_file.json | New test configuration file in consolidated location |
| configs/tuvx/data/* | Multiple new and updated test data files in consolidated location |
| cmake/dependencies.cmake | Updates pybind11 dependency from v3.0.0 to v3.0.1 |
| README.md | Updates Python workflow badge reference from python-tests to python-wheels |
| .github/workflows/python-wheels.yml | Adds Windows ARM64 build matrix entry and workflow steps |
| .github/workflows/python-tests.yml | File deleted, testing now handled by cibuildwheel in python-wheels workflow |
| .github/windows_arm64_steps/action.yml | Updates Windows ARM64 setup with required build tools |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
890b1cb to
107becf
Compare
- Updatig to pybind11 3.0.1 - 3.0.0 lead to a visibility compiler error on windows arm - 3.0.1 includes a fix for this
- attempting to set hdf and netcdf directories
nullifying radiator
- select type seems to lose the pointer attribute on fortran derived
types
- this lead to an error similar to the one below
- moving the deallocate to after the select type made flang happy
musica/src/tuvx/interface_radiator_map.F90:200:18: error: Name in DEALLOCATE statement must have the ALLOCATABLE or POINTER attribute
deallocate(f_radiator)
^^^^^^^^^^
- only installing ussa1976 on non widows non arm systems - disabling carma on windows arm, some sort of internal error and I don't know how to debug it since I don't have a windows arm machine - arm wheel build
- removing intel dockerfile
- moving tuvx data into configs - removing redundant full_config files - updating C++/Fortran/C tests to use new tuvx data location
af400fa to
2ba5ada
Compare
- windows fortran was timing out on mkdir -p - I don't know why, but disabling diagnostics for tests should fix this - this commit copies files from tuvx to allow us to do that - something in this pr probably affected this, but I don't know why since all I did was change a path...
This PR
flangis the only fortran compiler available on windows arm (that I know of) and we ended up having some issues show up. Incidentally, those are the same issues listed in Fix the errors raised by the Intel compiler #249. I addressed them here.So, by adding windows arm support, we broadened both our number of supported platforms and compilers.
The Windows Fortran tests continue to fail, but they already fail on main. See this failed action
Closes #249