Update Cloudspades #51456
Update Cloudspades #51456
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the In Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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/cloudspades/build.sh (1)
Line range hint
10-17: LGTM! Consider optimizing for macOS.The addition of the case statement to set the
THREADSvariable based on the operating system is a good improvement. It allows for better control over the compilation process on different platforms.However, you might want to consider setting a default thread count for macOS as well, to potentially improve build performance. For example:
case $(uname) in Linux) THREADS="-rj${CPU_COUNT}" ;; Darwin) - THREADS="" + THREADS="-rj$(sysctl -n hw.ncpu)" ;; esacThis change would use the number of available CPU cores on macOS, similar to the Linux setting.
recipes/cloudspades/meta.yaml (1)
42-42: Excellent addition of a test command for 10x format datasets.The new test command is a valuable addition that directly addresses the issue mentioned in the PR objectives. It tests the functionality of
spades.pywith 10x format datasets, which is crucial for ensuring the package works correctly with linked read technologies.Consider enhancing the test to explicitly check for the "unknown enumerated scalar" error related to the "clouds10x" type mentioned in the PR objectives. This could involve parsing the output or return code of the command to ensure the error doesn't occur. For example:
- | spades.py --isolate --gemcode1-1 assembler/test_dataset_cloudspades/10x_format/test_1.fastq.gz --gemcode1-2 assembler/test_dataset_cloudspades/10x_format/test_2.fastq.gz -o cloudspades_test if [ $? -ne 0 ]; then echo "spades.py failed" && exit 1 fi if grep -q "unknown enumerated scalar.*clouds10x" cloudspades_test/spades.log; then echo "Found 'unknown enumerated scalar' error related to clouds10x" && exit 1 fiThis enhancement would provide more robust testing specifically for the issue addressed in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/cloudspades/build.sh (2 hunks)
- recipes/cloudspades/meta.yaml (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
recipes/cloudspades/build.sh (3)
21-21: LGTM! Improved compilation command.The modification to use
./spades_compile.sh "${THREADS}"is a good improvement. It simplifies the compilation command and ensures consistent usage of theTHREADSvariable across different platforms.
Line range hint
1-23: Overall good improvements, with minor adjustments needed.The changes to this build script generally improve the build process for the cloudspades package. The main improvements include:
- Potentially resolving conflicts by commenting out environment variable exports.
- Adding OS-specific thread settings.
- Simplifying the compilation command.
- Ensuring correct package structure by creating directories and moving files.
However, there are a few minor issues and potential improvements:
- Verify that commenting out the environment variables doesn't negatively impact the build.
- Consider optimizing thread usage for macOS.
- Fix the incorrect
mv -rcommand.After addressing these points, the build script should be more robust and efficient across different platforms.
5-8: Verify the impact of commenting out environment variable exports.The export statements for
LIBRARY_PATH,INCLUDE_PATH,CFLAGS, andCXXFLAGShave been commented out. While this might resolve conflicts with the conda environment, it's important to ensure that the build process still works correctly without these explicit settings.Please confirm that the package builds and functions correctly without these environment variables. You may want to run the following script to check for any hardcoded paths that might be affected:
recipes/cloudspades/meta.yaml (1)
Line range hint
1-67: Version mismatch between package and source URLI noticed that while the package version is set to 3.16.0, the source URL points to a cloudspades-0.1 tag. This mismatch could potentially lead to confusion or issues in the future.
Could you please clarify if this version mismatch is intentional? If not, consider aligning the versions to ensure consistency.
On a positive note, I'd like to acknowledge the good practices in place:
- The use of
run_exportssection, which aligns with the PR objectives' mention of avoiding API, ABI, or CLI breakage issues.- The intentional skip for OSX builds, which aligns with the focus on Linux mentioned in the PR objectives.
|
@BiocondaBot please add label |
|
LGTM, thanks for looking into this @pdimens! |
| @@ -50,7 +50,5 @@ about: | |||
| doc_url: "https://github.com/ablab/spades/tree/cloudspades-ismb" | |||
|
|
|||
| extra: | |||
| additional-platforms: | |||
| - linux-aarch64 | |||
There was a problem hiding this comment.
What was the problem with aarch64 here ?
There was a problem hiding this comment.
It fails to compile on ARM
There was a problem hiding this comment.
I guess it failed to compile due to the usage of signed chars.
The support has been removed with this PR - https://github.com/bioconda/bioconda-recipes/pull/51456/files#diff-be671f9588fb1b742546ddcfd20773418719432b8e42799df8df52a9e7a3df9eL8 (-fsigned-char is gone)
What is the correct way to pass CXXFLAGS for this recipe ?
There was a problem hiding this comment.
It seems to work correctly without any flags but the previous commits that borrowed the CXX flags from the regular spades recipe built, however the resulting installation had errors when running (same for the current spades recipe, which remains unresolved). AFAIK, there isn't an example of a proper spades ARM build on bioconda and the existing spades (regular) build is currently bugged and unresolved. See #51390
There was a problem hiding this comment.
I know just about nothing about compilation, so I'm able to identify that there is an issue, but powerless to implement meaningful fixes.
While cloudspades builds to completion, there is an error when using the version from the conda recipe:
However, this error does not appear if I create a new (and empty) conda environment, download the same tarball onto my system and run the compile script manually from within this conda env, without flags, and simply move the
binandsharefolders to the proper${PREFIX}locations. This PR is an attempt to replicate that with the conda recipe.Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add labelcommand.@bioconda/corein a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports(see here for the rationale and comprehensive explanation).Add a
run_exportssection like this:with
...being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}while replacing
"myrecipe"with eithernameif aname|lowervariable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update@BiocondaBot please add labelplease review & mergelabel.@BiocondaBot please fetch artifactsYou can use this to test packages locally.
Note that the
@BiocondaBot please mergecommand is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>and will automatically re-post them to notify the addressed<team>.