Add pydustmasker
#51118
Add pydustmasker
#51118
Conversation
📝 WalkthroughWalkthroughThis pull request introduces two new files for the Changes
Possibly related PRs
Suggested labels
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 (5)
recipes/pydustmasker/build.sh (2)
5-7: Improve macOS-specific setupThe macOS-specific setup is good for cross-platform compatibility. However, there are two minor improvements we can make:
- Quote the
unamecommand to prevent word splitting.- Declare and assign the
HOMEvariable separately to avoid masking return values.Here's the suggested fix:
-if [ `uname` == Darwin ]; then - export HOME=`mktemp -d` +if [ "$(uname)" == Darwin ]; then + HOME=$(mktemp -d) + export HOME fi🧰 Tools
🪛 Shellcheck
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 6-6: Declare and assign separately to avoid masking return values.
(SC2155)
15-17: LGTM: Proper package build and installationThe package build using maturin and the subsequent installation with pip look correct. The use of
--no-depsand--ignore-installedflags is appropriate for conda environments.For consistency with the rest of the script, consider using
$PYTHONfor the maturin command as well:-maturin build --interpreter python --release +$PYTHON -m maturin build --releaseThis change ensures that the same Python interpreter is used for both building and installing the package.
recipes/pydustmasker/meta.yaml (3)
17-25: LGTM: Requirements are well-defined. Consider pinning Python version.The requirements section is correctly structured:
- Build requirements include the C compiler, necessary for compiling Rust code.
- Host requirements (pip, maturin, python) are appropriate for building a Rust-based Python package.
- Run requirements only include python, which is correct for a compiled extension.
However, consider pinning the Python version to ensure compatibility:
requirements: host: - python >=3.7 run: - python >=3.7This helps prevent potential issues with incompatible Python versions while still allowing for some flexibility.
27-29: LGTM: Basic import test is present. Consider adding more comprehensive tests.The import test is sufficient to verify that the package can be imported. However, to ensure the package's functionality, consider adding more comprehensive tests. For example:
test: imports: - pydustmasker commands: - python -c "import pydustmasker; assert pydustmasker.__version__ == '1.0.0'" - python -c "from pydustmasker import dust_mask; assert callable(dust_mask)"These additional tests would verify the package version and the presence of expected functions.
1-37: Minor style suggestion: Remove extra blank line at the end of the file.The yamllint tool reported a warning about too many blank lines at the end of the file. Consider removing the extra blank line to adhere to YAML style guidelines.
Note: The syntax error reported by yamllint for the Jinja2 templating syntax (
{%) can be safely ignored, as it's a false positive. Jinja2 templating is correctly used in Conda recipes.🧰 Tools
🪛 yamllint
[warning] 37-37: too many blank lines
(1 > 0) (empty-lines)
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/pydustmasker/build.sh (1 hunks)
- recipes/pydustmasker/meta.yaml (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/pydustmasker/build.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 6-6: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 yamllint
recipes/pydustmasker/meta.yaml
[warning] 37-37: too many blank lines
(1 > 0) (empty-lines)
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/pydustmasker/build.sh (1)
1-3: LGTM: Proper script setupThe script starts with the correct shebang and uses
set -exfor proper error handling and debugging. This is a good practice for bash scripts.recipes/pydustmasker/meta.yaml (4)
1-6: LGTM: Package name and version are correctly defined.The package name and version are properly set using Jinja2 templating, which is the recommended approach for Conda recipes. The version "1.0.0" matches the information provided in the PR objectives.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
12-15: LGTM: Build section is correctly configured.The build section is well-defined:
- Build number is set to 0, which is appropriate for a new package.
- The
run_exportssection uses the recommendedpin_subpackagemacro withmax_pin="x.x", ensuring compatibility up to minor version changes. This aligns with the PR objectives regarding API, ABI, and CLI stability.
31-36: LGTM: About section is comprehensive and well-defined.The about section provides all necessary metadata:
- Home URL is correctly specified.
- License information (MIT) is provided along with the license file location.
- The summary clearly describes the package's functionality.
This information is crucial for users and complies with Bioconda's requirements.
8-10: Verify the source URL structure.The source section is generally well-defined with the correct URL format and SHA256 checksum. However, there seems to be a potential issue with the URL structure:
url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gzThe package name appears twice in the URL (
{{ name }}-{{ name }}), which is unusual. This might lead to a 404 error when trying to download the source. Please verify if this is correct or if it should be:url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gzTo confirm the correct URL, you can run the following command:
This will check both URL variants and show which one returns a successful HTTP status code.
| curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain nightly --profile=minimal -y | ||
|
|
||
| export PATH="$HOME/.cargo/bin:$PATH" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using stable Rust toolchain
The Rust installation and PATH update look good. However, using the nightly toolchain might lead to instability in the build process. Unless there's a specific requirement for nightly features, it's generally recommended to use the stable toolchain.
Consider changing the Rust installation command to use the stable toolchain:
-curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain nightly --profile=minimal -y
+curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain stable --profile=minimal -yIf nightly is required, please add a comment explaining why.
📝 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.
| curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain nightly --profile=minimal -y | |
| export PATH="$HOME/.cargo/bin:$PATH" | |
| curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain stable --profile=minimal -y | |
| export PATH="$HOME/.cargo/bin:$PATH" |
|
|
||
| export PATH="$HOME/.cargo/bin:$PATH" | ||
|
|
||
| export CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER="$CC" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cross-platform linker setup
The current linker setup is specific to the x86_64 Linux target. To improve cross-platform support, consider making this setup more generic.
Here's a suggestion for a more generic linker setup:
-export CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER="$CC"
+if [ -n "$CC" ]; then
+ # Get the target triple from rustc
+ TARGET_TRIPLE=$(rustc -vV | sed -n 's|host: ||p')
+ # Set the linker for the current target
+ export CARGO_TARGET_${TARGET_TRIPLE//-/_}_LINKER="$CC"
+fiThis change will set the appropriate linker for the current target, regardless of the platform.
📝 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.
| export CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER="$CC" | |
| if [ -n "$CC" ]; then | |
| # Get the target triple from rustc | |
| TARGET_TRIPLE=$(rustc -vV | sed -n 's|host: ||p') | |
| # Set the linker for the current target | |
| export CARGO_TARGET_${TARGET_TRIPLE//-/_}_LINKER="$CC" | |
| fi |
|
@BiocondaBot please add label |
This PR adds the
pydustmaskerPython library.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>.Summary by CodeRabbit
New Features
pydustmaskerpackage (version 1.0.0) for nucleotide sequence processing.Documentation
meta.yamlfile, such as package dependencies and project information.