Add support for HDF5 to sdist/wheels by bryanwweber · Pull Request #1727 · Cantera/cantera

bryanwweber · 2024-06-27T21:18:34Z

Changes proposed in this pull request

Build Python sdist and wheel using scikit-build-core
- scikit-build-core will find system libraries in appropriate places because it uses CMake for configuration
Enable building with HDF5 support
Enable building the PythonExtensionManager (I think this is new?)

🚧 TODO: 🚧

Refactor SCons* so that system_packages are enabled (right now they're forced to be disabled, but we don't need our private copies anymore)
Refactor SCons* so that scons sdist doesn't run unnecessary system checks
Include files from Eigen and built libraries from SUNDIALS are installed to the wheel, that shouldn't be happening
We don't need to package pyx files in the wheel, or cythonized cpp files in the sdist any more. Check that this is correct
Figure out license packaging requirements (I don't think we need to package anything except the Cantera license for the sdist, but the wheel may require dependency licenses)
Get it working with the sdist/wheel builder workflow repo
Decide if we need to install cmake and/or ninja with the build.system-requires table (see CMake and Ninja are not installed by scikit-build-core scikit-build/scikit-build-core#806). We do not need to do this because I was wrong about when cmake is installed.

  CMake Warning at C:/Users/runneradmin/AppData/Local/Temp/tmp61vecodi/build/_deps/sundials-src/cmake/SundialsBuildOptionsPre.cmake:85 (message):
  SUNDIALS is being built with extensive error checks, performance may be
  affected.

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#205

If applicable, provide an example illustrating new features this pull request is introducing

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

codecov · 2024-06-27T21:40:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.04%. Comparing base (9210b08) to head (8386395).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1727   +/-   ##
=======================================
  Coverage   73.03%   73.04%           
=======================================
  Files         381      381           
  Lines       54132    54164   +32     
  Branches     9233     9240    +7     
=======================================
+ Hits        39534    39562   +28     
+ Misses      11635    11633    -2     
- Partials     2963     2969    +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

speth

Thanks, @bryanwweber. Overall, this looks good to me. I had just a few minor suggestions.

SConstruct

interfaces/python_sdist/build_sdist.py

Add external dependencies to be downloaded at build time. Add a note about cython_cmake extension

This allows us to avoid running a whole bunch of unnecessary configuration checks that are required when compiling code but not when we're just copying files into a folder structure to build the sdist. Refactor the builder script Move building into sconstruct, use some functions, include the license file. Remove unused code Clean up excludes from the wheel Fix Python 3.8 typing problem

Update Boost paths Config updates cibuildwheel config changes Modify env vars for boost Bump manylinux images and use ours with Boost in them Add HDF5 library env var Try to set up for arm builds hack Update DYLD fallback path for wheel repair Use DYLD_FALLBACK_LIBRARY_PATH from the environment This avoids needing to know about special variables Move the container engine config back to pyproject.toml Set Windows test command Add Windows wheel repair command

We don't need it and this will save some config time

This aligns with the configuration in config.h

Prior to this change, build would attempt to build the sdist even if the file copies were incomplete.

Since we aren't configuring the SUNDIALS version at wheel-build time, we can directly substitute to create config.h in the include directory.

bryanwweber · 2024-07-29T01:50:05Z

@speth I think I addressed everything here 😄

speth

Thanks, @bryanwweber. Just a couple of minor comments. Otherwise I think this is good to go.

doc/sphinx/develop/distribution-packages/pypi-sdist-wheel.md

doc/sphinx/develop/index.md

doc/sphinx/develop/distribution-packages/pypi-sdist-wheel.md

Co-authored-by: Ray Speth <[email protected]>

bryanwweber · 2024-07-29T14:57:43Z

Thanks @speth! Good suggestions 😄

bryanwweber force-pushed the bryan-support-hdf5-in-wheels branch 2 times, most recently from 1e58f55 to d639f01 Compare July 14, 2024 12:04

bryanwweber force-pushed the bryan-support-hdf5-in-wheels branch 4 times, most recently from d2995bf to 4272203 Compare July 21, 2024 23:41

bryanwweber marked this pull request as ready for review July 22, 2024 00:45

bryanwweber added enhancement Windows Python Linux macOS compiling labels Jul 22, 2024

speth requested changes Jul 22, 2024

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

SConstruct Show resolved Hide resolved

interfaces/python_sdist/build_sdist.py Outdated Show resolved Hide resolved

bryanwweber changed the title ~~Add support for HDF to sdist/wheels~~ Add support for HDF5 to sdist/wheels Jul 23, 2024

bryanwweber force-pushed the bryan-support-hdf5-in-wheels branch from b6537b9 to 881fa80 Compare July 23, 2024 20:10

bryanwweber added 13 commits July 25, 2024 12:51

Allow scons clean sdist command

d6cc560

Start by adding sckit-build-core

e14f81e

Add external dependencies to be downloaded at build time. Add a note about cython_cmake extension

Update URLs with hashes for deps

84c031c

Set CMake policy to override project variables

30ddf8d

Fix performance warning in SUNDIALS on Windows

f76ede4

Add some explanation of certain features

0aa2dd8

Disable check for Fortran support

acf63a5

We don't need it and this will save some config time

Ensure correct config values are set for cmake/ninja

afc904f

Fix Ninja version pin

046e8a2

Remove Build shared libs option

5d85f6e

Restore BUILD_SHARED_LIBS OFF

19642b0

bryanwweber added 3 commits July 25, 2024 12:51

Pin SUNDIALS to require version 7.0 exactly

a129066

This aligns with the configuration in config.h

[SCons] Check that build_sdist succeeds

545e325

Prior to this change, build would attempt to build the sdist even if the file copies were incomplete.

Remove double config.h substitution

9d3235d

Since we aren't configuring the SUNDIALS version at wheel-build time, we can directly substitute to create config.h in the include directory.

bryanwweber force-pushed the bryan-support-hdf5-in-wheels branch from 881fa80 to 239149d Compare July 28, 2024 12:54

bryanwweber added 2 commits July 28, 2024 21:48

[Doc] Fix a syntax warning in conf.py

738f33b

[Doc] Add packaging docs

e48c4fc

bryanwweber force-pushed the bryan-support-hdf5-in-wheels branch from 239149d to e48c4fc Compare July 29, 2024 01:49

speth requested changes Jul 29, 2024

View reviewed changes

doc/sphinx/develop/distribution-packages/pypi-sdist-wheel.md Outdated Show resolved Hide resolved

doc/sphinx/develop/index.md Outdated Show resolved Hide resolved

doc/sphinx/develop/distribution-packages/pypi-sdist-wheel.md Outdated Show resolved Hide resolved

Address review comments for docs

8386395

Co-authored-by: Ray Speth <[email protected]>

bryanwweber force-pushed the bryan-support-hdf5-in-wheels branch from 91448e7 to 8386395 Compare July 29, 2024 14:57

speth approved these changes Jul 29, 2024

View reviewed changes

speth merged commit a1ee588 into Cantera:main Jul 29, 2024

bryanwweber deleted the bryan-support-hdf5-in-wheels branch August 2, 2024 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for HDF5 to sdist/wheels#1727

Add support for HDF5 to sdist/wheels#1727
speth merged 19 commits intoCantera:mainfrom
bryanwweber:bryan-support-hdf5-in-wheels

bryanwweber commented Jun 27, 2024 •

edited

Loading

Uh oh!

codecov bot commented Jun 27, 2024 •

edited

Loading

Uh oh!

speth left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryanwweber commented Jul 29, 2024

Uh oh!

speth left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryanwweber commented Jul 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bryanwweber commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryanwweber commented Jul 29, 2024

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bryanwweber commented Jul 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bryanwweber commented Jun 27, 2024 •

edited

Loading

codecov bot commented Jun 27, 2024 •

edited

Loading