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

[Cherrypick 0.13] Add M1 build and test jobs #6112

Conversation

YosuaMichael
Copy link
Contributor

@YosuaMichael YosuaMichael commented May 31, 2022

Cherrypick from main: #5948 and #6110

This change will add a file .github/workflows/build-m1-binaries.yml which build m1 on the CI.

EDIT: this more generally picks all M1-related stuff, including #6122 and #6132 (and a lot more stuff tracked in #6081 (comment))

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks Yosua

@YosuaMichael
Copy link
Contributor Author

YosuaMichael commented May 31, 2022

Thanks Yosua

Fast! Thanks @NicolasHug !

@datumbox
Copy link
Contributor

FYI we will need to also cherrypick #6111

@YosuaMichael
Copy link
Contributor Author

#6111

@datumbox once #6111 is merged on main, we can put it together on this PR

Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

It needs some more changes to be usable on release branches, please hold until they are done. (For example, one need to change version from 0.14 back to 0.13)

@NicolasHug NicolasHug changed the title [Cherrypick 0.13] Add M1 CI build setup [Cherrypick 0.13] Add M1 build and test jobs Jun 6, 2022
@datumbox datumbox requested a review from malfet June 6, 2022 12:31
datumbox
datumbox previously approved these changes Jun 6, 2022
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

@NicolasHug grabbed the latest differences from numerous M1 related PRs merged on main.

I understand that this is a straight copy-paste of the final configs from main. They all LGTM.

@NicolasHug
Copy link
Member

Before merging we will need @malfet 's input on #6117 (comment).

#6117 has made changes to .github/workflows/build-m1-binaries.yml and I wonder if the env variables are correctly set now.

@datumbox
Copy link
Contributor

datumbox commented Jun 6, 2022

@NicolasHug I think his PR at #6117 does solve the issue and if it's not already cherrypicked, needs to be included here. I agree, let's wait to see what he thinks.

malfet and others added 4 commits June 8, 2022 17:31
* [BE] Unify version computation

Instead of hardcoding dev version in various script, use one from
`version.txt` if `setup_build_version` is called without arguments

Also, pass `--pre` option to M1 build/test pip install commands to build
TorchVision against nightly pytorch

* Pin torchvision dependency to a specific pytorch version
Clean up Conda build folder before every run
Enable artifact upload to GitHub for every workflow run, but upload to Conda/S3 only on nightly pushes

Test plan: `conda install -c pytorch-nightly torchvision; python -c "import torchvision;print(torchvision.io.read_image('hummingbird.jpg').shape)"`
setup_build_version
WHL_NAME=torchvision-${BUILD_VERSION}-cp${PY_VERS/.}-cp${PY_VERS/.}-macosx_11_0_arm64.whl
conda create -yp ${ENV_NAME} python=${PY_VERS} numpy libpng jpeg wheel pkg-config
conda run -p ${ENV_NAME} python3 -mpip install torch --pre -f https://download.pytorch.org/whl/test/cpu/torch_test.html
Copy link
Member

Choose a reason for hiding this comment

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

I have edited this line to rely on the test repo instead of the nightlies, but there are still various references to "nightly" in this script and I'm not sure what to do with them.

@malfet would you mind double checking this whole file?
(perhaps you'd like to wait after #6140?)

Copy link
Member

Choose a reason for hiding this comment

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

Note: This ended up being overwritten in #6140.

Comment on lines +29 to +33
run: |
# reference ends with an RC suffix
if [[ ${GITHUB_REF_NAME} = *-rc[0-9]* ]]; then
echo "CHANNEL=test" >> "$GITHUB_ENV"
fi
Copy link
Member

Choose a reason for hiding this comment

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

@malfet @atalman

It looks like the builds are using the nightly channel instead of the test channel, see e.g. https://github.com/pytorch/vision/runs/6858724488?check_suite_focus=true#step:4:133

Should we update the tag to also consider branches like the current release/0.13?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice followup(cc: @atalman), but for the time being feel free to just change CHANNEL environment variable on line 16 to test...

Copy link
Contributor

Choose a reason for hiding this comment

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

Done merged fix for this

@datumbox datumbox requested review from NicolasHug and datumbox June 13, 2022 13:54
Comment on lines +29 to +33
run: |
# reference ends with an RC suffix
if [[ ${GITHUB_REF_NAME} = *-rc[0-9]* ]]; then
echo "CHANNEL=test" >> "$GITHUB_ENV"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice followup(cc: @atalman), but for the time being feel free to just change CHANNEL environment variable on line 16 to test...

run: |
# reference ends with an RC suffix
if [[ ${GITHUB_REF_NAME} = *-rc[0-9]* ]]; then
echo "CHANNEL=test" >> "$GITHUB_ENV"
Copy link
Contributor

Choose a reason for hiding this comment

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

@atalman : Hmm, don't we need to setup channel for build_conda workflow as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done! Fix is commited: fb9a761

@datumbox
Copy link
Contributor

More things need to be cherrypicked. For example #6158

At this point, with so many changes, it might be worth checking that this PR contains all possible improvements done on main related to M1s.

.github/workflows/build-m1-binaries.yml Outdated Show resolved Hide resolved
.github/workflows/build-m1-binaries.yml Outdated Show resolved Hide resolved
Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
@NicolasHug
Copy link
Member

At this point, with so many changes, it might be worth checking that this PR contains all possible improvements done on main related to M1s.

I tried to keep track of everything and updated #6081 (comment), but yes. There are so many PRs that this will require a very careful review.

@datumbox
Copy link
Contributor

@NicolasHug Agreed. Happy to lend a pair of eyes at the last PR if you want. Ping me when ready.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

The build jobs are currently failing with a dependency resolution conflict https://github.com/pytorch/vision/runs/6864443482?check_suite_focus=true

Marking red so we don't merge by mistake.

@NicolasHug
Copy link
Member

@malfet @atalman would you mind advising on the errors above?

@datumbox datumbox mentioned this pull request Jun 14, 2022
)

* Make sure we are building against test chanell for release

* Cleanup

* Cleanup

* Refactoring logic

* Remove tagged tests
@atalman atalman force-pushed the cherrypick013/m1-build-setup branch from cd32d58 to fb9a761 Compare June 15, 2022 00:34
@datumbox
Copy link
Contributor

datumbox commented Jun 15, 2022

@NicolasHug @YosuaMichael There is another PR that adds M1 support by @atalman at #6167 that passes all the tests. Might be worth merging that one instead.

@YosuaMichael
Copy link
Contributor Author

YosuaMichael commented Jun 15, 2022

@NicolasHug @YosuaMichael There is another PR that adds M1 support by @atalman at #6167 that passes all the tests. Might be worth merging that one instead.

Agree, lets use #6167 for the cherry pick and close this PR, I have edited on the release tracker, what do you think @NicolasHug ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants