Skip to content

Conversation

@mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented May 14, 2025

EDIT: This is ready to go

Note

The large number of changed lines is misleading. Most of the lines are for the licenses that are bundled with the wheels (since the binary wheels uploaded to PyPI need to include some shared libraries)


This PR set up the continuous integration to test creation and uploading of wheels.

Here is the test.pypi page for pygrackle.

You can try installing pygrackle from PyPI by invoking

pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ pygrackle

We are going to need to do a followup PR after this one (I want to test uploading to test.pypi from the main grackle repository before uploading a wheel to pypi).


Checklist:

  • Have we gone through all the files in a sample sdist? Do we want to remove more?
  • Have we uncommented various python ABIs? (currently disabled for testing purpose
  • Have we fixed the testing? (ideally we would fix the tests so that we can run the (at least some) non answer-tests without an editable install)
  • Have we switched to compiling with the full version of hdf5? (following a discussion with Britton, we decided we should ship a version that enables the various compression options)
  • Have we gotten wheels to build for Intel-based Macs?
  • Have we adjusted the minimum python version. It sounds like we will adopt yt's minimum python version.

Future Thoughts

It might be useful to refactor the logic to download and setup HDF5 so that the sdist can use the logic:

  • In other words, we could let people enable an option to automatically download and build HDF5 as part of the build process.
  • While this would be nice, it's a somewhat moot point since people will still need to install a fortran compiler. There's no benefit to just waiting until we remove fortran. At that people will still need to worry about installing a recent-ish C++ compiler that implements a small subset of C++17 features1
  • we aren't doing this in this PR since it involves some work and it would be a little tricky to get "right."
    • In particular, we want to make sure that the standard auditwheel and delocate tools (for renaming and packaging library dependencies) work properly. (it's definitely doable and would be "nice to have", but is not essential)
    • moreover, things will be more messy now that we have decided to install hdf5 with various enabled compression options. We need to decide whether to ship zlib (I think cibuildwheel assumes it's present on some platforms -- but I need to check all platforms we support). We will also be shipping libaec (which appears to be used over szip)

In the future, it might be cool to ship wheels using the CPython's Limited API:

  • Essentially, we would ship 3 binary wheels per platform (platforms include macosx-arm64, macosx-x86_64, manylinux-x86_64, multilinux-x86_64, etc.):
    1. a wheel compatible with CPython == 3.10.x (full api)
    2. a wheel compatible with CPython == 3.11.x (full api)
    3. a wheel compatible with CPython >= 3.12.x (uses limited-api)
  • According to this Cython document the limited API is slower (I found some issues suggesting a lot slower), and is still somewhat experimental.
  • I'm leaving this for the future, since we would definitely need to test the staple-API wheel with every supported minor version of CPython.
    • In other words, we compile the wheel with 3.12, but then we should run tests with that wheel against 3.12, 3.13, (and soon 3.14)
    • This is doable, but it would take some finagling with GitHub Actions and it feels a little beyond the scope of this PR

Footnotes

  1. This won't be a significant burden and should be easy for everyone to do. But the point still stands that some platforms may not ship this by default.

@mabruzzo mabruzzo added the gracklepy Relates to the gracklepy Python interface. label May 14, 2025
@mabruzzo mabruzzo force-pushed the binary-wheel branch 3 times, most recently from 92ed798 to 55fb5f8 Compare May 14, 2025 12:42
@mabruzzo mabruzzo marked this pull request as draft May 14, 2025 13:46
mabruzzo added a commit to mabruzzo/grackle that referenced this pull request May 19, 2025
Prior to this PR, our test suite would fail unless we had an editable
install. In reality, all test-cases outside of test_models infer the
location of the data files based on the location of the test-files,
which are never part of the wheel.[^1] Therefore, this PR introduces a
few small tweaks to support testing all test-cases outside of
test_models.

Overall, this will be really useful for providing a more rigorous check
of the correctness of our precompiled wheels (see PR grackle-project#320).

[^1]: I know that some packages do ship their test-suites as part of
      the wheel, but that currently makes absolutely no sense for us
      unless we provide users with an easy way to get the datafiles.
      We can always revisit this choice in the future after we merge
      PRs grackle-project#235 & grackle-project#237 (or come up with an equivalent)
@mabruzzo mabruzzo force-pushed the binary-wheel branch 4 times, most recently from cc67fbf to 710c51f Compare June 1, 2025 14:04
@mabruzzo mabruzzo force-pushed the binary-wheel branch 5 times, most recently from c578901 to bc5092e Compare June 3, 2025 15:19
mabruzzo added 2 commits June 3, 2025 11:45
This verifies that all libraries included in the wheel have a license entry
and confirm that we didn't include any unnecessary licenses

This is invoked as part of building the binary wheels
I changed 2 parts:
- HDF5: I added the HDF5 SPDX identifier.
- GCC runtime library: I made a change reflecting a recent numpy commit
@mabruzzo
Copy link
Collaborator Author

mabruzzo commented Jun 4, 2025

@brittonsmith this is ready for review.

@mabruzzo mabruzzo marked this pull request as ready for review June 4, 2025 04:43
Copy link
Contributor

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks fine to me to the extend I could follow it without seeing it in action. I'm fine to merge it and we can work out any kinks in testing.

@brittonsmith brittonsmith merged commit ce146c0 into grackle-project:main Jun 10, 2025
4 checks passed
mabruzzo added a commit to mabruzzo/grackle that referenced this pull request Jun 10, 2025
@mabruzzo mabruzzo deleted the binary-wheel branch June 10, 2025 21:27
brittonsmith added a commit that referenced this pull request Jun 11, 2025
fixing a bug in pyproject.toml that arose because PRs #320 and #350 introduced conflicting changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gracklepy Relates to the gracklepy Python interface.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants