add learnMSA #51042
add learnMSA #51042
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
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: 2
🧹 Outside diff range and nitpick comments (2)
recipes/tiberius/build.sh (1)
9-15: Consider long-term strategy for learnMSA integrationWhile the current changes provide a working solution for incorporating learnMSA, there are some concerns about the long-term strategy:
- The temporary nature of this solution is clear, but there's no indication of when or how it will be replaced with a permanent solution.
- This approach of cloning and installing during the build process might not be ideal for a conda package, which typically relies on pre-built dependencies.
Consider the following suggestions:
- Add a TODO comment with a timeline or condition for removing this temporary solution.
- Investigate if learnMSA can be packaged separately as a conda package, which would allow it to be included as a dependency in meta.yaml instead of being installed in the build script.
- If the current approach is necessary for now, document the reasoning and any plans for future changes in a comment within the script or in the package documentation.
🧰 Tools
🪛 Shellcheck
[warning] 12-12: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/tiberius/meta.yaml (1)
39-39: LGTM: Good addition of Keras accessibility test.The new test command to verify Keras installation and accessibility is a valuable addition. It ensures that Keras is properly installed and can be imported, which is crucial given the new explicit requirement for Keras.
Consider extending the test to verify the specific version of Keras, given the strict version requirement in the run dependencies. You could modify the test command as follows:
test: commands: - tiberius -h - python -c "import tensorflow.keras as keras; assert keras.__version__ == '2.10.0', f'Expected Keras version 2.10.0, but got {keras.__version__}'; print('Keras 2.10.0 is installed and accessible')"This will ensure that not only is Keras accessible, but it's also the exact version specified in the requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/tiberius/build.sh (1 hunks)
- recipes/tiberius/meta.yaml (3 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/tiberius/build.sh
[warning] 12-12: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (2)
recipes/tiberius/build.sh (1)
7-7: LGTM: Appropriate shebang additionAdding the Python 3 shebang to the tiberius script is a good practice. It ensures that the script will be executed with the correct interpreter.
recipes/tiberius/meta.yaml (1)
13-13: LGTM: Build number increment is appropriate.The build number has been correctly incremented from 0 to 1. This is the right approach when making changes to a package without altering its version number.
| #Adding learnMSA | ||
| git clone https://github.com/Gaius-Augustus/learnMSA | ||
| cd learnMSA | ||
| git checkout parallel | ||
| pip install . | ||
| cd .. |
There was a problem hiding this comment.
Improve error handling and cleanup in temporary learnMSA installation
While the temporary installation of learnMSA is clearly documented, there are a few improvements that could make this process more robust:
- Add error handling to the git and pip commands to ensure the script fails if these critical steps fail.
- Return to the original directory after the installation is complete.
- Consider cleaning up the cloned repository after installation to save space.
Here's a suggested improvement:
#Temporary: until the specific branch of learnMSA compatible with Tiberius is merged
#Adding learnMSA
-git clone https://github.com/Gaius-Augustus/learnMSA
-cd learnMSA
-git checkout parallel
-pip install .
-cd ..
+git clone https://github.com/Gaius-Augustus/learnMSA || exit 1
+cd learnMSA || exit 1
+git checkout parallel || exit 1
+pip install . || exit 1
+cd ..
+rm -rf learnMSAThis change addresses the static analysis warning about cd potentially failing, adds error handling to all commands, and includes cleanup of the cloned repository.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #Temporary: until the specific branch of learnMSA compatible with Tiberius is merged | |
| #Adding learnMSA | |
| git clone https://github.com/Gaius-Augustus/learnMSA | |
| cd learnMSA | |
| git checkout parallel | |
| pip install . | |
| cd .. | |
| #Temporary: until the specific branch of learnMSA compatible with Tiberius is merged | |
| #Adding learnMSA | |
| git clone https://github.com/Gaius-Augustus/learnMSA || exit 1 | |
| cd learnMSA || exit 1 | |
| git checkout parallel || exit 1 | |
| pip install . || exit 1 | |
| cd .. | |
| rm -rf learnMSA |
🧰 Tools
🪛 Shellcheck
[warning] 12-12: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
There was a problem hiding this comment.
Do not suggest to add such verbose line by line error handling.
There was a problem hiding this comment.
Don't do that for any recipe script in this repo.
There was a problem hiding this comment.
@johanneskoester Understood, I will avoid suggesting such verbose line-by-line error handling in the future.
(_/)
(•_•)
/>📝
✏️ Learnings added
Learnt from: johanneskoester
PR: bioconda/bioconda-recipes#51042
File: recipes/tiberius/build.sh:9-15
Timestamp: 2024-09-30T16:19:54.319Z
Learning: Avoid suggesting verbose line-by-line error handling in scripts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@johanneskoester, understood. I will avoid suggesting such verbose line-by-line error handling in recipe scripts for this repository in the future.
✏️ Learnings added
Learnt from: johanneskoester
PR: bioconda/bioconda-recipes#51042
File: recipes/tiberius/build.sh:9-15
Timestamp: 2024-09-30T16:23:14.125Z
Learning: For the bioconda/bioconda-recipes repository, do not suggest adding verbose line-by-line error handling in recipe scripts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| - tensorflow >=2.9 | ||
| - keras ==2.10.0 | ||
| - python >=3.10 | ||
| - pip | ||
| - tensorflow | ||
| - numpy >=1.21 |
There was a problem hiding this comment.
Review and adjust package requirements.
The changes to the requirements section are generally good, but there are a few points to consider:
-
The Python version requirement has been significantly increased from >=3.7 to >=3.10. This is a major change that could affect compatibility. Please ensure this is necessary and documented.
-
The strict version requirement for keras (==2.10.0) might be too restrictive. Consider using a more flexible version specifier like
>=2.10.0,<2.11to allow for minor updates and patches. -
The order of requirements is not standard. Typically,
pythonis listed first. Consider reordering the requirements.
Suggested changes:
requirements:
run:
- python >=3.10
- tensorflow >=2.9
- keras >=2.10.0,<2.11
- numpy >=1.21
- pip
# ... (rest of the requirements)Also, please provide justification for the significant increase in the Python version requirement, as this could impact users of the package.
There was a problem hiding this comment.
@coderabbitai, when reviewing package dependencies here, always mention that they should align as close as possible with the upstream description of the packaged software.
There was a problem hiding this comment.
Do that for all recipes in this repo.
There was a problem hiding this comment.
@johanneskoester, understood. I'll make sure to mention that package dependencies should align as closely as possible with the upstream description when reviewing dependencies in all recipes in this repository.
(_/)
(•ᵕ•)
📖
✏️ Learnings added
Learnt from: johanneskoester
PR: bioconda/bioconda-recipes#51042
File: recipes/tiberius/meta.yaml:23-27
Timestamp: 2024-09-30T16:18:06.774Z
Learning: When reviewing package dependencies in this repository, always mention that they should align as closely as possible with the upstream description of the packaged software.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@johanneskoester, thank you for the guidance. In future reviews, I will ensure to mention that package dependencies should align as closely as possible with the upstream description of the packaged software.
(_/)
(•‿•)
/ 📝
✏️ Learnings added
Learnt from: johanneskoester
PR: bioconda/bioconda-recipes#51042
File: recipes/tiberius/meta.yaml:23-27
Timestamp: 2024-09-30T16:18:35.932Z
Learning: When reviewing package dependencies, always mention that they should align as closely as possible with the upstream description of the packaged software.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| #Temporary: until the specific branch of learnMSA compatible with Tiberius is merged | ||
| #Adding learnMSA | ||
| git clone https://github.com/Gaius-Augustus/learnMSA |
There was a problem hiding this comment.
You could use a URL in the meta.yml like this: https://github.com/Gaius-Augustus/learnMSA/archive/refs/heads/parallel.tar.gz
However, we should not release branches that are not in main IMHO. maybe a new package?
There was a problem hiding this comment.
On the tiberius repository, they indicate to use the parallel branch to run the tool. I asked the question on leanrMSA's repository. Here's their answer: Gaius-Augustus/learnMSA#12 (comment)
Hello,
I'm opening this PR to add learnMSA to the tiberius conda package.
Thank you! Have a good day!
Summary by CodeRabbit
New Features
Bug Fixes
tiberius.pyfile is executable and created a symbolic link.Documentation