Skip to content

[Build] Implement install_requirements.sh with Python - #4748

Merged
dbort merged 3 commits into
pytorch:mainfrom
python3kgae:sh_to_py
Aug 20, 2024
Merged

[Build] Implement install_requirements.sh with Python#4748
dbort merged 3 commits into
pytorch:mainfrom
python3kgae:sh_to_py

Conversation

@python3kgae

@python3kgaepython3kgae commented Aug 16, 2024

Copy link
Copy Markdown
Contributor

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.

@pytorch-bot

pytorch-botBot commented Aug 16, 2024

Copy link
Copy Markdown

🔗 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-botfacebook-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
Copy Markdown
ContributorAuthor

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

@manuelcandalesmanuelcandales added module: build/install Issues related to the cmake and buck2 builds, and to installing ExecuTorch 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

@dbortdbort left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadinstall_requirements.py Outdated
Comment threadinstall_requirements.py Outdated
Comment threadinstall_requirements.py
Comment threadinstall_requirements.py Outdated
Comment threadinstall_requirements.py
Comment threadinstall_requirements.py
Comment threadinstall_requirements.py Outdated
Comment threadinstall_requirements.py
Comment threadinstall_requirements.py Outdated
Comment threadinstall_requirements.sh
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.

@dbortdbort left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadinstall_requirements.py
@dbort
dbort dismissed their stale reviewAugust 20, 2024 00:14

Changes have been made

@dbort

Copy link
Copy Markdown
Contributor

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.

Comment threadinstall_requirements.py
@dbort

Copy link
Copy Markdown
Contributor

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

@dbort

Copy link
Copy Markdown
Contributor

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:mainAug 20, 2024
@dbort

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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.

@GregoryComerGregoryComer mentioned this pull request Aug 28, 2025
20 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.module: build/installIssues related to the cmake and buck2 builds, and to installing ExecuTorchtriagedThis 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

@python3kgae@dbort@facebook-github-bot@manuelcandales