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

[Build] Implement install_requirements.sh with Python #4748

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

python3kgae
Copy link
Contributor

@python3kgae python3kgae commented Aug 16, 2024

Move the details of install_requirements.sh into install_requirements.py. Then, call install_requirements.py from install_requirements.sh.

This will avoid duplication when adding Windows version of installing_requirements.

This is for issue #4661.

Release Note: Moved the main install_requirements.sh logic into install_requirements.py so that it can also run on Windows.

Test Plan:
Tested on a mac.

Ran ./install_requirements.sh --pybind xnnpack under python 3.10, and it completed successfully. To make sure that xnnpack was actually installed, I manually ran the steps from the notebook at https://colab.research.google.com/drive/1qpxrXC3YdJQzly3mRg-4ayYiOjC6rue3, and it passed.

When running the same in a clean python 3.8 environment, it failed as expected with

% ./install_requirements.sh --pybind xnnpack
Collecting packaging
  Using cached packaging-24.1-py3-none-any.whl.metadata (3.2 kB)
Using cached packaging-24.1-py3-none-any.whl (53 kB)
Installing collected packages: packaging
Successfully installed packaging-24.1
ERROR: ExecuTorch does not support python version 3.8.19: must satisfy ">=3.10"

Also tested under WSL and native Windows.

Copy link

pytorch-bot bot commented Aug 16, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4748

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (1 Unrelated Failure)

As of commit dd5b83e with merge base d3da92d (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@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 Aug 16, 2024
@python3kgae
Copy link
Contributor Author

Tested install_requirements.py under WSL and native Windows only.
Need help to test under Linux and Mac.

@manuelcandales manuelcandales added module: build Related to buck2 and cmake build triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Aug 16, 2024
dbort
dbort previously requested changes Aug 16, 2024
Copy link
Contributor

@dbort dbort left a comment

Choose a reason for hiding this comment

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

This is looking great! Thank you so much for taking the time to do this. It'll be great to reduce the duplication between windows and non-windows.

install_requirements.py Outdated Show resolved Hide resolved
install_requirements.py Outdated Show resolved Hide resolved
install_requirements.py Outdated Show resolved Hide resolved
install_requirements.py Outdated Show resolved Hide resolved
install_requirements.py Show resolved Hide resolved
install_requirements.py Show resolved Hide resolved
install_requirements.py Outdated Show resolved Hide resolved
install_requirements.py Show resolved Hide resolved
install_requirements.py Outdated Show resolved Hide resolved
install_requirements.sh Show resolved Hide resolved
Move the details of install_requirements.sh into install_requirements.py.
Then, call install_requirements.py from install_requirements.sh.

This will avoid duplication when add Windows version of installing_requirements.

This is for issue pytorch#4661.
Copy link
Contributor

@dbort dbort left a comment

Choose a reason for hiding this comment

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

Thanks for the update! This looks great. I'll try running it locally when I have a chance.

install_requirements.py Show resolved Hide resolved
@dbort dbort dismissed their stale review August 20, 2024 00:14

Changes have been made

@dbort
Copy link
Contributor

dbort commented Aug 20, 2024

Worked for me on mac. I updated the PR summary with the steps that I followed. I'm waiting for jobs to finish, but I should be able to merge this soon.

install_requirements.py Show resolved Hide resolved
@dbort
Copy link
Contributor

dbort commented Aug 20, 2024

Undid the nightly string change at @lucylq's request; we're not ready to bump it yet.

@dbort
Copy link
Contributor

dbort commented Aug 20, 2024

I just removed the date-changing commit from this PR, going back to a previous state that's already been tested. Merging.

@dbort dbort merged commit f887d72 into pytorch:main Aug 20, 2024
75 of 77 checks passed
@dbort
Copy link
Contributor

dbort commented Aug 20, 2024

Thank you again for doing this @python3kgae! This is something we've wanted to do for a while, and it really helps cut down on the duplication when making it work for Windows.

@python3kgae
Copy link
Contributor Author

Thank you again for doing this @python3kgae! This is something we've wanted to do for a while, and it really helps cut down on the duplication when making it work for Windows.

Thank you for the review and testing. We are now closer to achieving a native Windows build.

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. module: build Related to buck2 and cmake build triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants