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

Enable release on Mac arm64 #692

Closed
wants to merge 3 commits into from
Closed

Enable release on Mac arm64 #692

wants to merge 3 commits into from

Conversation

ejguan
Copy link
Contributor

@ejguan ejguan commented Jul 26, 2022

Fixes #676

Changes

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 26, 2022
@facebook-github-bot
Copy link
Contributor

@ejguan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

LGTM! Just some questions:

  1. S3 is not compatible with Windows, correct?
  2. We are building the same way regardless if it is MacOS x86_64 or arm64?

Comment on lines +81 to +85
- name: Setup Python ${{ matrix.python-version }}
if: ${{ startsWith( matrix.os, 'windows' ) }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ: Was windows already setting up Python before? Was this not needed until now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

steps:
- name: Setup Python ${{ matrix.python-version }}
if: ${{ ! startsWith( matrix.os, 'ubuntu' ) }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, we excluded ubuntu from this step because ubuntu used docker. Now, since macos would use conda to install python, windows becomes the only platform that would use this step.
cc: @NivekT

@ejguan
Copy link
Contributor Author

ejguan commented Jul 26, 2022

  1. S3 is not compatible with Windows, correct?

Actually, it is compatible. And, we do compile AWSSDK for PyPI wheels. However, for conda packages, I haven't figured out how to integrate AWSSDK with torchdata. I decide not to invest more time on it unless there are more users' request

  1. We are building the same way regardless if it is MacOS x86_64 or arm64?

Yea, the same workflow.

ejguan added a commit to ejguan/data that referenced this pull request Jul 27, 2022
Summary:
Fixes pytorch#676

- Update the workflow to compile AWSSDK and integrated on arm64 macos
  - Use conda to install python since this [action](https://github.com/actions/setup-python) doesn't work with arm64b macos
  - Exclude python 3.7 from arm64 macos due to the pre-compiled python binary is not available either on conda or python official website
  - Use `bash -l {0}` to make sure `conda` is available across steps
- Please check the manually triggered [workflow](https://github.com/pytorch/data/runs/7528349378?check_suite_focus=true)
  - Wheels: https://github.com/pytorch/data/runs/7528349378?check_suite_focus=true#step:4:1
  - Conda packages: https://github.com/pytorch/data/runs/7528462809?check_suite_focus=true#step:5:1

Pull Request resolved: pytorch#692

Reviewed By: NivekT

Differential Revision: D38172672

Pulled By: ejguan

fbshipit-source-id: d7ae1fd523fc140d5bc7b9d526b9a421cfd2eff6
ejguan added a commit that referenced this pull request Jul 27, 2022
Summary:
Fixes #676

- Update the workflow to compile AWSSDK and integrated on arm64 macos
  - Use conda to install python since this [action](https://github.com/actions/setup-python) doesn't work with arm64b macos
  - Exclude python 3.7 from arm64 macos due to the pre-compiled python binary is not available either on conda or python official website
  - Use `bash -l {0}` to make sure `conda` is available across steps
- Please check the manually triggered [workflow](https://github.com/pytorch/data/runs/7528349378?check_suite_focus=true)
  - Wheels: https://github.com/pytorch/data/runs/7528349378?check_suite_focus=true#step:4:1
  - Conda packages: https://github.com/pytorch/data/runs/7528462809?check_suite_focus=true#step:5:1

Pull Request resolved: #692

Reviewed By: NivekT

Differential Revision: D38172672

Pulled By: ejguan

fbshipit-source-id: d7ae1fd523fc140d5bc7b9d526b9a421cfd2eff6
@@ -62,22 +62,46 @@ jobs:
- macos-latest
- ubuntu-latest
- windows-latest
- macos-m1-12

Choose a reason for hiding this comment

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

Is this an internal Meta self-hosted runner or does GitHub Actions finally have public M1 runners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our self-hosted runner

@andrewkho andrewkho deleted the enable_m1 branch September 26, 2024 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

osx-arm64 build missing from Anaconda pytorch channel
4 participants