Refactor deploy.yml
#1947
Refactor deploy.yml
#1947
Conversation
|
|
robertodr
force-pushed
the
revise-deploy
branch
3 times, most recently
from
54bc33b
to
3547288
Compare
robertodr
force-pushed
the
revise-deploy
branch
from
ab95beb
to
f4cfdf2
Compare
There was a problem hiding this comment.
Thanks for kicking this off. I left a few comments inline. Ihave some concerns that some of the changes here won't work as intended. If I understand correctly you're trying to fix the aarch64 Linux wheel jobs, is the only change needed for that is to use the newer manylinux version? If so maybe we should only update that to limit the scope here.
Also, any changes we make here we should mirror in the build workflow which is used to test wheel builds work on proposed changes: https://github.com/Qiskit/qiskit-aer/blob/main/.github/workflows/build.yml The wheel jobs are essentially duplicated there, so we'll want to keep that in sync
| - uses: pypa/gh-action-pypi-publish@v1 | ||
| with: | ||
| user: __token__ | ||
| password: ${{ secrets.pypi_password }} |
There was a problem hiding this comment.
This won't actually work, we don't have token auth setup on the repo. We'll be moving to trusted publishers soon, but for the time being we need to set the username on the credentials with the password.
There was a problem hiding this comment.
Good to know! I'll revert to using the username and password as stored in the repo secrets.
| - id: set-matrix | ||
| run: | | ||
| python -m pip install cibuildwheel==2.11.2 | ||
| - name: Build wheels | ||
| env: | ||
| AER_CMAKE_OPENMP_BUILD: 1 | ||
| run: python -m cibuildwheel --output-dir wheelhouse | ||
| - uses: actions/upload-artifact@v2 | ||
| with: | ||
| path: ./wheelhouse/*.whl | ||
| - name: Publish Wheels | ||
| env: | ||
| TWINE_PASSWORD: ${{ secrets.TWINE_PASSWORD }} | ||
| TWINE_USERNAME: qiskit | ||
| run : | | ||
| pip install -U twine | ||
| twine upload wheelhouse/* | ||
| build_wheels_aarch64: | ||
| name: Build wheels on ${{ matrix.os }} | ||
| runs-on: ${{ matrix.os }} | ||
| MATRIX=$( | ||
| { | ||
| cibuildwheel --print-build-identifiers --platform linux \ | ||
| | jq -nRc '{"only": inputs, "os": "ubuntu-latest"}' \ | ||
| && cibuildwheel --print-build-identifiers --platform macos \ | ||
| | jq -nRc '{"only": inputs, "os": "macos-latest"}' \ | ||
| && cibuildwheel --print-build-identifiers --platform windows \ | ||
| | jq -nRc '{"only": inputs, "os": "windows-2019"}' | ||
| } | jq -sc ) | ||
| echo "MATRIX IS $MATRIX" | ||
| echo "include=$MATRIX" >> $GITHUB_OUTPUT | ||
| env: | ||
| CIBW_ARCHS_LINUX: x86_64 aarch64 s390x ppc64le i686 | ||
| CIBW_ARCHS_MACOS: x86_64 arm64 | ||
| CIBW_ARCHS_WINDOWS: x86 AMD64 |
There was a problem hiding this comment.
I'm a little wary of doing the job matrix dynamically like this. It seems prone to issues if there are environment changes that could cause the full matrix to grow or shrink outside our control. We wouldn't necessarily notice when that happens except at a release.
| needs: | ||
| - build-wheels | ||
| - build-sdist |
There was a problem hiding this comment.
I see the advantage of this, but the current structure of uploading in each job was intentional. We have a fairly high job failure rate and being able to incrementally push a new release for the jobs that pass is desireable as it allows us to get the new release out while fixing issues. For example, in the case of the aarch64 wheels we'd be blocked by that issue from publishing even though it's an isolated issue to the aarch64 environment.
| manylinux-i686-image = "manylinux2014" | ||
| skip = "pp* cp36* cp37* *musllinux*" | ||
| test-skip = "cp310-win32 cp310-manylinux_i686 cp311-win32 cp311-manylinux_i686" | ||
| skip = "pp* cp36* cp37* cp312* *musllinux*" |
There was a problem hiding this comment.
We can probably unskip/include python 3.12 now, because it's released.
| manylinux-x86_64-image = "manylinux_2_28" | ||
| manylinux-aarch64-image = "manylinux_2_28" | ||
| manylinux-s390x-image = "manylinux_2_28" | ||
| manylinux-ppc64le-image = "manylinux_2_28" |
There was a problem hiding this comment.
This change is a bit concerning to me. We're raising the minimum environment requirements for aer by making this change. Users that were able to install aer before will no longer be able to use it without updating their operating system. Is this really needed? I assume you made this change to fix the aarch64 build error, but is this needed for all the cpu architectures? Also, we'll need to document this change in an upgrade release note (see: https://github.com/Qiskit/qiskit-aer/blob/main/CONTRIBUTING.md#adding-a-new-release-note for the procedure on doing this).
| release: | ||
| types: | ||
| - published |
There was a problem hiding this comment.
This actually will defer our release jobs slightly. The normal workflow we use for releasing is to push a tag directly, then we have a bot service auto generate the release page based on that tag. I think this will still work fine, but we've typically used the tag event to signal a release. We probably should change the matching regex for the tag event to match our versioning scheme so it's a bit more restrictive, but we have tag protection on so only authorized users can push one.
| - name: Build Artifacts | ||
| run: | | ||
| python setup.py sdist | ||
| only: ${{ matrix.only }} |
There was a problem hiding this comment.
Does this end up spawing a separate job for each combination of py version and platform. Our most constrained resource is job parallelism, which is shared with other projects in the qiskit github organization, so it's better to batch things into fewer jobs if we can.
Summary
Refactor
deploy.yml:manylinux_2_28for all architectures excepti686(unsupported) and CUDA (didn't manage to install the toolkit on AlmaLinux 8)This incidentally closes #1872.
Details and comments
I was mostly out to fix the build of
aarch64wheels on Linux 馃槃 I have followed the examples in thecibuildwheeldocumentation.ppc64leas it was failing at installing the testing dependencies.