-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add aarch64 wheel build support #3414
Conversation
9f1e558
to
6a32cbf
Compare
@jcwchen, could you please merge this PR. Thanks in advance. |
Could @onnx/sig-archinfra-approvers please review this PR? Thanks. |
Could you please review this PR? Thanks |
@odidev : Please fix pipeline failures: |
branches: [master, rel-*] | ||
pull_request: | ||
branches: [rel-*, master] # TODO remove master before merge | ||
|
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.
add workflow_dispatch: on line 11 to enable manual trigger similar to : https://github.com/onnx/onnx/blob/master/.github/workflows/release_linux_x86_64.yml#L11
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.
Done.
|
||
jobs: | ||
build: | ||
runs-on: ubuntu-latest |
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.
add an if here similar to https://github.com/onnx/onnx/blob/master/.github/workflows/release_linux_x86_64.yml#L15
to enable manual trigger
We enable manual triggers from master by setting a label "run release CIs" on the PR
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.
Done.
auth_header="$(git config --local --get http.https://github.com/.extraheader)" | ||
git submodule sync --recursive | ||
git -c "http.extraheader=$auth_header" -c protocol.version=2 submodule update --init --force --recursive --depth=1 | ||
- name: Install python dependencies |
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.
nit: add a space after even task for better readability
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.
this workflow is different from others since you are running inside a docker container because this is aarch64 platform. Please add detailed comments through out this yml to help understand this workflow.
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.
Done.
run: | | ||
docker run --rm -v ${{ github.workspace }}:/ws:rw --workdir=/ws \ | ||
${{ env.img }} ${{ env.py }} -m pip install -U numpy protobuf==3.11.3 | ||
- name: Install dependencies |
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.
Can you club "Install python dependencies" and "Install dependencies" together?
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.
Done.
run: | | ||
docker run --rm -v ${{ github.workspace }}:/ws:rw --workdir=/ws \ | ||
${{ env.img }} \ | ||
bash -exc '${{ env.py }} -m pip install virtualenv && ${{ env.py }} -m venv .env && \ |
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.
why do you need virtualenv?
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.
By using virtual environment we are making already installed dependencies available for different tasks.
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.
git actions does let you do this across tasks... you dont need virtual env for that
bash -exc '${{ env.py }} -m pip install virtualenv && ${{ env.py }} -m venv .env && \ | ||
source .env/bin/activate && \ | ||
python -m pip install --upgrade pip && \ | ||
pip install dist/*-manylinux2014_aarch64.whl && \ |
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 dist/*-manylinux2014_aarch64.whl should be changes to dist/*manylinux2014_aarch64.whl
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.
Done.
if: (github.event_name == 'schedule') # Only triggered by weekly event | ||
run: | | ||
python -m pip install -q twine | ||
twine upload --verbose dist/*.whl --repository-url https://test.pypi.org/legacy/ -u ${{ secrets.TESTPYPI_USERNAME }} -p ${{ secrets.TESTPYPI_PASSWORD }} |
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.
we added a few more verification steps to the pipelines to cut the manual testing effort. Please add them for this pipeline too. You can refer to: https://github.com/onnx/onnx/blob/master/.github/workflows/release_linux_x86_64.yml#L75
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.
Done.
push: | ||
branches: [master, rel-*] | ||
pull_request: | ||
branches: [rel-*, master] # TODO remove master before merge |
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.
nit: this #TODO comment can be removed now
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.
Removed.
strategy: | ||
matrix: | ||
# the different python versions for building wheels | ||
python-version: [cp37-cp37m, cp38-cp38, cp39-cp39] |
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.
How about Python 3.6?
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.
Done.
bash -exc '${{ env.py }} -m pip install virtualenv && ${{ env.py }} -m venv .env && \ | ||
source .env/bin/activate && \ | ||
${{ env.py }} -m pip install -U numpy protobuf==3.11.3 && \ | ||
yum install -y protobuf-compiler protobuf-devel |
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 assume the protobuf version from yum package is not 3.11.3, right? I think a better way is to install protobuf from source like this:
# Build protobuf |
BTW, I think anyway protobuf will be installed by entrypoint.sh? Is the installation here required?
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.
Yes, I checked it, it is not required as it is already installed.
@odidev : can you please resolve conflicts? |
@odidev : we can check this PR in once the conflicts are resolved |
@odidev : FYI - The code freeze date for ONNX 1.10 is July 15th (today) since this PR is reviewed and ready we can wait until July 20th. Otherwise this PR will miss ONNX 1.10 release. |
Done. |
Signed-off-by: odidev <odidev@puresoftware.com>
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.
Thanks for updates! Overall looks good to me. Let me reopen this PR to test this new CI.
Added aarch64 wheel build support.
Related to #3413. @postrational could you please review this PR?