Update rna-seqc to 2.4.2 #51342
Update rna-seqc to 2.4.2 #51342
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces changes to two files: In the Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 yamllint
🔇 Additional comments (7)
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 (4)
recipes/rna-seqc/build.sh (3)
Line range hint
7-19: SeqLib component builds look good, consider optimizationThe build process for SeqLib components (bwa, fermi-lite, htslib) is correctly maintained. The use of environment variables for compiler flags ensures flexibility across different build environments.
Consider creating a function for the repetitive build process to reduce code duplication:
build_component() { pushd "$1" make CC="$CC" CFLAGS="$CFLAGS" LDFLAGS="$LDFLAGS" popd } build_component SeqLib/bwa build_component SeqLib/fermi-lite build_component SeqLib/htslibThis refactoring would make the script more maintainable and easier to extend in the future.
Line range hint
21-31: Main build process looks good, document use of -fcommonThe main build process is correctly maintained, with the addition of the
-fcommonflag to address potential compatibility issues with newer GCC versions.Consider adding a comment explaining the reason for using the
-fcommonflag:make \ - CC="$CXX -fcommon" \ + CC="$CXX -fcommon" # -fcommon flag added for compatibility with GCC 10+ \ INCLUDE_DIRS="-I$PREFIX/include -ISeqLib -ISeqLib/htslib" \ LIBRARY_PATHS="-L$PREFIX/lib -Wl,-rpath $PREFIX/lib"This documentation will help future maintainers understand the reason for this compiler flag.
33-35: Python script installation added, consider error handlingThe addition of Python script installation is a good improvement, extending the functionality of the package.
Consider adding error handling to make the script more robust:
# Install python scripts +if [ ! -d "python" ]; then + echo "Error: python directory not found" + exit 1 +fi cd python -$PYTHON setup.py install +if ! $PYTHON setup.py install; then + echo "Error: Python script installation failed" + exit 1 +fiThis change will ensure that the script fails explicitly if the Python installation encounters any issues, making it easier to diagnose problems during the build process.
recipes/rna-seqc/meta.yaml (1)
34-40: New dependencies added correctly, consider specifying versions.The addition of new Python dependencies (numpy, pandas, matplotlib-base, seaborn, nbformat, and agutil via pip) is appropriate for the updated package version. These are common libraries for data analysis and visualization.
Consider specifying version constraints for these new dependencies to ensure compatibility and reproducibility. For example:
- numpy >=1.18 - pandas >=1.0 - matplotlib-base >=3.2 - seaborn >=0.10 - nbformat >=5.0 - pip: - agutil >=3.0Adjust the versions as appropriate for your specific needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/rna-seqc/build.sh (2 hunks)
- recipes/rna-seqc/meta.yaml (3 hunks)
🧰 Additional context used
🪛 yamllint
recipes/rna-seqc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/rna-seqc/meta.yaml (5)
1-2: Version and checksum update looks good.The package version has been correctly updated to 2.4.2, and the SHA256 checksum has been updated accordingly. This change aligns with the PR objective.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
13-15: Build number reset and run_exports addition are appropriate.The build number has been correctly reset to 1 for the new version. The addition of the run_exports section with the pin_subpackage directive is a good practice to ensure correct version pinning when this package is used as a dependency.
44-45: Import test addition is beneficial.The addition of an import test for
rnaseqcis a good practice. It complements the existing command test and helps ensure that the package is installed correctly and can be imported in Python scripts.
50-50: License file inclusion is correct.Restoring the
license_fileentry is important for legal compliance. It ensures that the license is properly included with the package distribution.
Line range hint
1-50: Overall, the update to rna-seqc 2.4.2 looks good.The changes in this file successfully update the rna-seqc package to version 2.4.2. Key points:
- Version and checksum have been updated correctly.
- Build number has been reset and run_exports added for proper version pinning.
- New Python dependencies have been added, reflecting the updated package requirements.
- An import test has been added to complement the existing command test.
- The license file is now properly included in the package.
These changes align well with the PR objectives. The only suggestion for improvement is to consider specifying version constraints for the new dependencies.
Will create a separate recipe for that
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/rna-seqc/meta.yaml (1)
39-44: Good: License file entry restoredRestoring the
license_fileentry is excellent for compliance and transparency. The rest of the metadata appears to be up-to-date.As a minor suggestion, consider adding a
doc_urlfield pointing to the project's documentation, if available. This can be helpful for users seeking more information about the tool.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/rna-seqc/build.sh (1 hunks)
- recipes/rna-seqc/meta.yaml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- recipes/rna-seqc/build.sh
🧰 Additional context used
🪛 yamllint
recipes/rna-seqc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/rna-seqc/meta.yaml (4)
1-2: LGTM: Version and checksum updated correctlyThe package version has been updated to 2.4.2 as per the PR objective, and the SHA256 checksum has been updated accordingly. This ensures the integrity of the new package version.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
13-15: Excellent: Build number reset and run_exports addedThe build number has been correctly reset to 0 for the new version. The addition of the
run_exportssection with thepin_subpackagedirective is a great improvement. This ensures that packages depending on rna-seqc will use a compatible version, enhancing the overall stability of the ecosystem.
36-36: LGTM: Appropriate test commandThe test command to check the rnaseqc version is appropriate and will help verify that the correct version (2.4.2) is installed.
Line range hint
17-33: Verify dependencies for the new versionThe requirements section remains unchanged. However, the AI summary mentioned new dependencies such as
numpy,pandas,matplotlib-base,seaborn,nbformat, and a pip requirement foragutil. Please verify if these new dependencies are required for version 2.4.2 and update therunsection accordingly if needed.To check for any new dependencies in the source code, you can run:
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
📝 WalkthroughWalkthroughThe pull request introduces modifications to 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
recipes/rna-seqc/build.sh (2)
Line range hint
8-25: LGTM: Improved build commands with consistent flag usageThe build commands have been updated to consistently use both
CFLAGSandLDFLAGS, which is a good practice. The addition of-fcommonsuggests compatibility with newer GCC versions.One minor suggestion:
Consider moving the
-fcommonflag to theCFLAGSenvironment variable at the beginning of the script for consistency:-export CFLAGS="${CFLAGS} -fcommon" +export CFLAGS="${CFLAGS} -fcommon" +export CXXFLAGS="${CXXFLAGS} -fcommon" # ... (rest of the script) make \ - CC="$CXX -fcommon" \ + CC="$CXX" \ INCLUDE_DIRS="-I$PREFIX/include -ISeqLib -ISeqLib/htslib" \ LIBRARY_PATHS="-L$PREFIX/lib -Wl,-rpath $PREFIX/lib"This change would apply the
-fcommonflag consistently to both C and C++ compilations and simplify the final build command.
Line range hint
1-35: Suggestion: Add version checking and build information outputThe build script is well-structured and covers all necessary steps. To further improve its robustness and debuggability, consider adding version checking and build information output.
Add the following at the end of the script:
# Print version information $PREFIX/bin/rnaseqc --version # Print Python package information $PYTHON -m pip show rnaseqcThis addition will help verify that the correct version has been installed and provide useful information for debugging purposes.
recipes/rna-seqc/meta.yaml (1)
34-40: New dependencies added correctly, but consider conda-forge for agutil.The addition of numpy, pandas, matplotlib-base, seaborn, and nbformat as runtime dependencies is appropriate and likely supports the new Python scripts mentioned in the PR objectives. This suggests expanded functionality in the new version of RNA-seqc.
However, installing agutil via pip in a conda recipe is not ideal. If possible, consider adding agutil to conda-forge and then including it as a regular conda dependency. This would improve consistency and make the package easier to manage.
Would you like assistance in checking if agutil is available on conda-forge or in creating a conda-forge recipe for it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/rna-seqc/build.sh (2 hunks)
- recipes/rna-seqc/meta.yaml (3 hunks)
🧰 Additional context used
🪛 yamllint
recipes/rna-seqc/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/rna-seqc/build.sh (1)
6-6: LGTM: Simplified directory navigationThe directory change command has been simplified, which aligns with the update to version 2.4.2. This change makes the build process more robust and less dependent on specific CI environments.
recipes/rna-seqc/meta.yaml (4)
1-2: LGTM: Version and checksum updated correctly.The package version has been updated to 2.4.2, and the SHA256 checksum has been changed accordingly. This aligns with the PR objective to update RNA-seqc to the latest version.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
13-13: LGTM: Build number reset appropriately.The build number has been reset to 1, which is the correct practice when updating to a new version of the package.
20-21: LGTM: run_exports section added correctly.The addition of the
run_exportssection with{{ pin_subpackage("rna-seqc", max_pin="x.x") }}is a good practice. This ensures that when this package is used as a dependency, it will be correctly pinned, allowing for minor version updates but not major ones. This helps maintain compatibility while allowing for bug fixes and minor improvements.
44-45: LGTM: Test import added and license file declared.The addition of the
rnaseqcimport in the test section is appropriate, especially with the new Python dependencies. This helps ensure that the Python module is correctly installed and importable.The license file declaration has been uncommented, which is correct if the LICENSE file is now included in the package distribution.
To confirm the presence of the LICENSE file, please run the following command:
This command will check the source tarball for the presence of a LICENSE file. If found, it confirms that uncommenting the
license_filedeclaration is correct.Also applies to: 50-50
recipes/rna-seqc/build.sh
Outdated
| # Install python scripts | ||
| cd python | ||
| $PYTHON setup.py install |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Approved: Python script installation added
The addition of Python script installation aligns with the PR objectives. However, there's room for improvement in the installation method.
Consider using pip for installation instead of setup.py, as it's the recommended method for modern Python packaging:
# Install python scripts
cd python
-$PYTHON setup.py install
+$PYTHON -m pip install .This change ensures that the package is installed with all its dependencies and metadata correctly handled by pip.
📝 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.
| # Install python scripts | |
| cd python | |
| $PYTHON setup.py install | |
| # Install python scripts | |
| cd python | |
| $PYTHON -m pip install . |
|
@bioconda-bot please add label |
This PR updates RNA-seqc to its current version. It additionally installs the python scripts too.