-
Notifications
You must be signed in to change notification settings - Fork 366
Workflow to build python CLI snapshot for nightly publishing (sdist + wheels) #3036
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
Conversation
|
@HonahX very nice work. We are almost there to have package in PyPI. |
snazy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason to not extent the release-automation workflows?
We have to follow the ASF release policy. It seems there are some steps missing in this PR:
- checksums
- gpg signatures
- RC staging
- staging the source tarball and artifacts to vote on
- eventually publishing exactly the artifacts that have been voted on
|
Hi @snazy Thanks for highlighting these important elements of the final release candidate for python CLI. Given the number of missing pieces in the CLI release process, my goal is to separate the pipeline into 2 milestone:
This PR aims to focus on the first milestone in order to have additional CI to ensure that all changes are compatible with the release process and unblock nightly build. The signature/checksum/release candidate are goals in the second milestone. This way we could first determine what to release and start testing the artifacts and then finishing the rest part. WDYT? |
9247338 to
36a45e4
Compare
|
@HonahX is this PR a draft PR or not? The summary says yes, the PR status says no. |
|
Hi @snazy It's ready for review. I checked the current PR description and it does not seem to imply this as a draft. Could you please point me to the part of the summary that may lead to the wrong implication here? Just want to eliminate all potential confusion. |
snazy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR adds completely new release workflows for separate Python releases.
We already have release workflows in place, so I do not see a point having a separate set of workflows.
Technically it's missing a lot of the requirements for release artifacts for ASF projects and the ASF release process.
A huge amount of time and work already went into the existing release automation, because it is not an easy task to get it right.
|
Hi @snazy, I think there might be some confusion in the original PR summary and the workflow name. I should not put the "release" in the workflow title as the intention was not to make this a separate release procedure than the existing release automation. Ultimately, the python client release process should be part of the release automation. I've updated the PR to simplify the workflow file and updated the summary to clarify the intention. We will focus on build the right wheels and sdist and make nightly build working first. Sorry for any confusion above. Please take a look again and let me know WDYT! |
|
I can take another look, but due to time constraints likely not before next week. |
| uses: pypa/cibuildwheel@v3.1.4 | ||
| with: | ||
| output-dir: wheelhouse | ||
| package-dir: "./client/python" | ||
| config-file: "{package}/pyproject.toml" | ||
| env: | ||
| # Ignore 32 bit architectures | ||
| CIBW_ARCHS: "auto64" | ||
| CIBW_PROJECT_REQUIRES_PYTHON: ">=3.10,<3.14" | ||
| CIBW_REPAIR_WHEEL_COMMAND_MACOS: "" # Disable wheel repair since there will be no binary in the wheel | ||
| CIBW_REPAIR_WHEEL_COMMAND_LINUX: "" | ||
| CIBW_ENVIRONMENT: POLARIS_CLI_SKIP_CLIENT_GENERATION="true" # java is not available in linux containers during wheel build, skip client generation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: does this command archive the same result?
run: |
make client-build FORMAT=wheel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be mostly the same for MacOs/windows, but for linux it will help build both manylinux and musllinux wheel to have broad compatibility.
flyrain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @HonahX !
snazy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do not need the overhead of CPython.
Without that, I suspect the Python package build can be moved into the Makefile and the calling workflows would just need to setup the necessary Python versions.
Then the existing nightly-build workflow can be extended with one job to setup Python, call that Makefile target and publish to test.pypi.org.
| make client-build FORMAT=sdist | ||
|
|
||
| - name: Build wheels | ||
| uses: pypa/cibuildwheel@v3.1.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actions in ASF workflows must reference the Git SHA of an ASF-approved version of the action, see this policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action docs mention that the wheels can contain libraries from the build container.
Which ones are included and what are the licenses for those libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. We definitely should use the approved version here: https://github.com/apache/infrastructure-actions/blob/7a58482c11f14b79ea238d4a57093077cc18673c/approved_patterns.yml#L174C3-L174C61
The action docs mention that the wheels can contain libraries from the build container.
The dependencies may be brought by the wheel repair operation. In our case, we explicitly disabled the wheel repair:
CIBW_REPAIR_WHEEL_COMMAND_MACOS: ""
CIBW_REPAIR_WHEEL_COMMAND_LINUX: ""
so no additional dependencies will be brought in.
But based on other comments and discussion, cibuildwheel may not be the best way here, let me try another approach first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to use make client-build FORMAT=wheel directly instead of cibuildwheel
| strategy: | ||
| matrix: | ||
| os: [ ubuntu-24.04, macos-14, macos-15] # current build script is not compatible with windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything that's performance critical in the Python package?
I'd prefer to not build native code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, ideally we only need a universal wheel since everything is pure python. However, since we need to leverage a custom build script to do open api client generation, poetry thinks it needs to generate platform-specific wheels. This is a known limitation and seems we have no native way to override this behavior now.
Should the situation changed in the future, we should definitely switch to universal wheel if possible.
| if [ -n "${{ inputs.version }}" ]; then | ||
| ARTIFACT_NAME="python-client-artifacts-${{ inputs.version }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this special case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could make the output zip file name indicate that a custom version has been provided hence the built sdist and wheels will have its package version changed. Just for convenience.
abe51b0 to
2578b89
Compare
|
I honestly do not think that this code is reusable from actual release automation, as many things like signing and checksums are not considered. Can you simplify this by focusing only on snapshot publishing? |
- Remove version input - auto-generate PEP 440 compliant dev version
Format: {base_version}.devYYYYMMDDHHMMSS+commitshort
- Generate version once and share across all build jobs
- Include version in final merged artifact name
- Rename workflow to "Python Client Build Snapshot Artifacts"
2578b89 to
f5c52e3
Compare
- Add client-get-version Makefile target to get current version
- Use make client-get-version instead of grep/sed parsing
- Simpler version format: {version}.dev{timestamp}
- Add artifact-name output for nightly workflow integration
|
Hi @snazy I've removed the version input option and make it the snapshot version we want to tag our nightly build, e.g. The workflow file is targeted to be used in 2 places:
For the nightly build, the example usage is like ...
python-client-snapshot-build:
if: github.repository == 'apache/polaris'
uses: ./.github/workflows/python-client-build-artifacts.yml
python-client-snapshot-publish:
runs-on: ubuntu-latest
needs: python-client-snapshot-build
if: github.repository == 'apache/polaris'
steps:
- name: Download artifacts
uses: actions/download-artifact@v4
with:
name: ${{ needs.python-client-snapshot-build.outputs.artifact-name }}
path: dist/
- name: List artifacts
run: ls -la dist/
- name: Publish to TestPyPI
id: publish-testpypi
continue-on-error: true
uses: pypa/gh-action-pypi-publish@release/v1
with:
repository-url: https://test.pypi.org/legacy/
skip-existing: true
verbose: trueIn this way we do not need to put the multi-OS wheel built logic in the nightly publishing workflow. The artifacts downloaded in Please let me know WDYT |
|
If we migrate to |
Exactly. |
|
Hi @HonahX , I might not have phrased my previous reply as clear as I wanted it to be. While I think the recent cleanup puts this PR in a much better shape (thanks for that!), I think the workflow should build the artifacts and also publish those directly to test-pypi. This is because (chained) GitHub workflow invocations have some restrictions, but more importantly to keep the (not so complex) logic in a single workflow (file). (Edit: lost an important word during writing) |
|
@snazy Thanks for the explanation. I think the previous complexity here is that we need to build wheels for different operating system and that's why I want a separate workflow for that to prevent over-complicating the publishing workflow. But with the uv migration happened recently, we can build universal wheel and we do not need this anymore. Thanks for reviewing! I will close this one. |
This PR include a github workflow, "Python Client Build Artifacts", to build wheels for multiple os/platform for nightly build and release automation in the future. It will produce a zip containing
contain sdist and wheels built for macos and linux (TODO: support windows)
The expected usage for this workflow is:
The build of a release candidate that include proper sig, checksum, is out-of-scope of this PR and should be included in separate PR/workflow for python client release automation
cc: @binarycat0 @MonkeyCanCode @flyrain @snazy
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)