Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[python/ci] Add CI job to test building wheel from sdist #2506

Merged
merged 5 commits into from
May 7, 2024

Conversation

jdblischak
Copy link
Collaborator

@jdblischak jdblischak commented May 3, 2024

Issue and/or context:

TileDB-SOMA-Py is built and distributed in many different ways (from source, PyPI wheels, conda binaries). We want to identify any packaging-related problems immediately in the PR that generates them instead of in a nightly build or worse after the release has been made.

Changes:

This PR includes 2 main changes:

Notes for Reviewer:

See the wiki page Debugging wheel‐build issues for more details

@jdblischak jdblischak self-assigned this May 3, 2024
@johnkerl
Copy link
Member

johnkerl commented May 3, 2024

@jdblischak
Copy link
Collaborator Author

is this partially duplicating https://github.com/single-cell-data/TileDB-SOMA/blob/main/.github/workflows/python-packaging.yml?

I wouldn't describe it as duplication. I would describe it more as preparation for the official building of the PyPI wheels.

Differences:

  • Uses transparent python commands instead of the opaque action pypa/cibuildwheel, and therefore easier to debug
  • Only builds two wheels total (python 3.11 on Ubuntu and macOS), so it is much faster. It's not creating the wheels for distribution, but instead just providing a quick check that all the required files are distributed in the source tarball

In summary, the goal is to identify any potential problems with building the PyPI wheels when a PR is submitted, and not at release time.

@johnkerl
Copy link
Member

johnkerl commented May 3, 2024

@jdblischak can we please get this verbatim into the affected YAML file as a block comment?

Differences:

Uses transparent python commands instead of the opaque action pypa/cibuildwheel, and therefore easier to debug
Only builds two wheels total (python 3.11 on Ubuntu and macOS), so it is much faster. It's not creating the wheels for distribution, but instead just providing a quick check that all the required files are distributed in the source tarball
In summary, the goal is to identify any potential problems with building the PyPI wheels when a PR is submitted, and not at release time.

Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

@jdblischak SGTM as long as CI resource consumption on every PR doesn't become excessive ... main issue being macos runners: choices are 5, 5, 5, or enterprise with no "step up" to 10 or 20 say ...

Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

@jdblischak SGTM as long as CI resource consumption on every PR doesn't become excessive ... main issue being macos runners: choices are 5, 5, 5, or enterprise with no "step up" to 10 or 20 say ...

@jdblischak
Copy link
Collaborator Author

as long as CI resource consumption on every PR doesn't become excessive

This pipeline does spawn a lot of jobs. Two reasons why I think it avoids being excessive:

  1. Aggressive use of path-based filters. This pipeline is only triggered if a file that could affect the Python installation process is affected (eg setup.py, pyproject.toml, etc). A PR that edits the C++ code, R code, or even the non-installation Python files will never trigger these jobs
  2. The macOS jobs all finish in under 10 minutes. So even if it hogs our parallel macOS jobs, it will be for a brief period of time

If it does start to become excessive, the first macOS job I would drop is the one added in this PR, sdist. It is mainly to catch files missing from the source tarball, and that behavior should be consistent across Linux and macOS.

@johnkerl
Copy link
Member

johnkerl commented May 3, 2024

Cool and thanks @jdblischak ! I indeed just looked for paths: in your affected YAML and we have protection there against over-running so my concern was premature (I should have checked there first). But thanks for looking, and thanks for the prioritization if/when "something has to go" someday -- really appreciate it! :)

@jdblischak jdblischak merged commit 33022c6 into single-cell-data:main May 7, 2024
23 checks passed
@jdblischak jdblischak deleted the python-ci-packaging branch May 7, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants