-
Notifications
You must be signed in to change notification settings - Fork 581
Ensure docker installs the torch version we need #1901
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
Conversation
Summary of ChangesHello @nvmbreughe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential issue in Docker builds where the intended PyTorch version might not be correctly installed due to conflicts with Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request correctly identifies the need to ensure a specific version of torch is installed in the Docker images, overriding what might be installed from requirements.txt. The proposed solution of using --force-reinstall is functional. However, it is inefficient as it leads to torch being installed twice. I've provided a suggestion to improve the Docker build efficiency by filtering torch out of the requirements.txt installation step and then installing it separately. This avoids the redundant download and installation, speeding up the build process.
| pip3 install -r /install/requirements.txt | ||
| pip3 install torch --index-url https://download.pytorch.org/whl/${CUDA_VERSION} | ||
| pip3 install --force-reinstall torch --index-url https://download.pytorch.org/whl/${CUDA_VERSION} |
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.
While using --force-reinstall works, it's inefficient because it installs torch twice (once from requirements.txt with the default index, and then again from the specific index). This can significantly slow down the Docker build process as torch is a large package.
A more efficient approach is to avoid installing torch from requirements.txt in the first place. You can filter it out and then install it separately from the correct index. This avoids the unnecessary installation and uninstallation cycle.
| pip3 install -r /install/requirements.txt | |
| pip3 install torch --index-url https://download.pytorch.org/whl/${CUDA_VERSION} | |
| pip3 install --force-reinstall torch --index-url https://download.pytorch.org/whl/${CUDA_VERSION} | |
| pip3 install -r <(grep -vE '^torch\b' /install/requirements.txt) | |
| pip3 install torch --index-url https://download.pytorch.org/whl/${CUDA_VERSION} |
yzh119
left a comment
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.
LGTM
<!-- .github/pull_request_template.md --> ## 📌 Description Github default runner's disk space is too small and results in OOM issues: https://github.com/flashinfer-ai/flashinfer/actions/runs/18389642821/job/52402828516 ## 🔍 Related Issues #1901 ## 🚀 Pull Request Checklist Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete. ### ✅ Pre-commit Checks - [x] I have installed `pre-commit` by running `pip install pre-commit` (or used your preferred method). - [x] I have installed the hooks with `pre-commit install`. - [x] I have run the hooks manually with `pre-commit run --all-files` and fixed any reported issues. > If you are unsure about how to set up `pre-commit`, see [the pre-commit documentation](https://pre-commit.com/). ## 🧪 Tests - [x] Tests have been added or updated as needed. - [ ] All tests are passing (`unittest`, etc.). ## Reviewer Notes cc @nvmbreughe
📌 Description
adds --force-reinstall when installing torch. This ensures we install the version we want, even if requirements.txt installed another one.
🔍 Related Issues
🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Reviewer Notes