Update InstaNovo: Use GPU version of Pytorch #52231
Conversation
BioGeek
changed the title
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
recipes/instanovo/meta.yaml (2)
65-65: Remove trailing whitespaceThere is a trailing whitespace after the DOI that should be removed.
- - doi:10.1101/2023.08.30.555055 + - doi:10.1101/2023.08.30.555055🧰 Tools
🪛 yamllint
[error] 65-65: trailing spaces
(trailing-spaces)
Line range hint
20-45: Consider adding channel configurationBased on the PR objectives mentioning conda channel configuration, we should specify the required channels to ensure proper installation of GPU dependencies.
Consider adding channel configuration at the package level:
channels: - conda-forge - defaults - biocondaThis will help users avoid installation issues with GPU-related packages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/instanovo/meta.yaml(3 hunks)
🧰 Additional context used
🪛 yamllint
recipes/instanovo/meta.yaml
[error] 65-65: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
recipes/instanovo/meta.yaml (2)
15-15: Verify if build number increment is sufficient
The build number increment is appropriate for tracking this change. However, given that this is a significant change in the underlying PyTorch implementation (CPU to GPU), consider if a minor version bump might be more appropriate.
57-57: LGTM: Summary update is clear and informative
The expanded summary better describes the package's capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
recipes/instanovo/meta.yaml (2)
Line range hint
46-51: Add CUDA availability verification testTo ensure the GPU support is properly configured, we should add a test to verify CUDA availability.
Add this test command:
commands: - python -c "import instanovo; print(instanovo.__version__)" + - python -c "import torch; print(f'CUDA available: {torch.cuda.is_available()}')"
65-65: Fix trailing whitespaceRemove the trailing space after the DOI.
- - doi:10.1101/2023.08.30.555055 + - doi:10.1101/2023.08.30.555055🧰 Tools
🪛 yamllint
[error] 65-65: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/instanovo/meta.yaml(3 hunks)
🧰 Additional context used
🪛 yamllint
recipes/instanovo/meta.yaml
[error] 65-65: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
recipes/instanovo/meta.yaml (3)
15-15: LGTM: Build number increment is appropriate
The build number increment from 0 to 1 is correct as this represents a rebuild with different dependencies (CPU → GPU PyTorch).
Line range hint 1-65: Consider specifying minimum CUDA version requirement
Since we're adding GPU support, it would be helpful to specify the minimum required CUDA version in the package metadata to ensure compatibility.
Would you like assistance in determining and adding the appropriate CUDA version constraints?
44-44:
Additional CUDA dependencies required for GPU support
The switch to conda-forge::pytorch-gpu alone isn't sufficient to ensure GPU functionality, as evidenced by the CUDA availability check returning False in the PR description.
I agree with the previous review comment. Please add the following dependencies:
- conda-forge::pytorch-gpu
+ - cudatoolkit
+ - cuda-nvcc
- tqdm
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
recipes/instanovo/meta.yaml (2)
54-54: Verify CUDA test implementationThe CUDA availability test has been added as suggested. However, consider adding a more comprehensive GPU check:
- python -c "import torch; print(f'CUDA is available: {torch.cuda.is_available()}')" + python -c "import torch; print(f'CUDA available: {torch.cuda.is_available()}'); print(f'GPU count: {torch.cuda.device_count()}'); print(f'GPU device: {torch.cuda.get_device_name(0) if torch.cuda.is_available() else "N/A"}')"This will provide more detailed information about the GPU setup during testing.
68-68: Fix trailing whitespace in DOI identifier lineRemove the trailing space after the DOI.
- - doi:10.1101/2023.08.30.555055 + - doi:10.1101/2023.08.30.555055🧰 Tools
🪛 yamllint
[error] 68-68: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/instanovo/meta.yaml(3 hunks)
🧰 Additional context used
🪛 yamllint
recipes/instanovo/meta.yaml
[error] 68-68: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
recipes/instanovo/meta.yaml (3)
15-15: LGTM: Build number increment is appropriate
The build number increment from 0 to 1 is correct as this represents a significant change in the package's dependencies (CPU to GPU transition).
44-46: Skip comment about GPU configuration requirements
The suggested changes from the previous review have been properly implemented, including the addition of cudatoolkit and cuda-nvcc dependencies.
Line range hint 21-46: Verify PyTorch version compatibility
The requirements section doesn't specify a version constraint for pytorch-gpu. Consider adding version constraints to ensure compatibility with the Python version requirement (>=3.10,<3.12) and other dependencies.
|
What does Is the CPU version non-functional? |
The CPU version is functional but so slow in practice that it can be considered unusable.
That indeed does work! Is that the intended way of installing CUDA-enabled code in the (bio)conda ecosytem? |
Yes. Generally, one should not explicitly specify CPU or GPU in the package metadata if it technically is capable of running on both. |
|
Understood. Closing this PR. |
InstaNovo currently installs the CPU version of PyTorch.
Pytorch recently announced that they will stop publishing to Anaconda and are directing users to use pytorch-gpu from conda-forge
CC: @mfansler