Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add pantax #50983

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

add pantax #50983

wants to merge 11 commits into from

Conversation

zwh82
Copy link
Contributor

@zwh82 zwh82 commented Sep 26, 2024

Describe your pull request here


Please read the guidelines for Bioconda recipes before opening a pull request (PR).

General instructions

  • If this PR adds or updates a recipe, use "Add" or "Update" appropriately as the first word in its title.
  • New recipes not directly relevant to the biological sciences need to be submitted to the conda-forge channel instead of Bioconda.
  • PRs require reviews prior to being merged. Once your PR is passing tests and ready to be merged, please issue the @BiocondaBot please add label command.
  • Please post questions on Gitter or ping @bioconda/core in 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_exports section like this:

build:
  run_exports:
    - ...

with ... being one of:

Case run_exports statement
semantic versioning {{ pin_subpackage("myrecipe", max_pin="x") }}
semantic versioning (0.x.x) {{ pin_subpackage("myrecipe", max_pin="x.x") }}
known breakage in minor versions {{ pin_subpackage("myrecipe", max_pin="x.x") }} (in such a case, please add a note that shortly mentions your evidence for that)
known breakage in patch versions {{ pin_subpackage("myrecipe", max_pin="x.x.x") }} (in such a case, please add a note that shortly mentions your evidence for that)
calendar versioning {{ pin_subpackage("myrecipe", max_pin=None) }}

while replacing "myrecipe" with either name if a name|lower variable 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 Merge the master branch into a PR.
@BiocondaBot please add label Add the please review & merge label.
@BiocondaBot please fetch artifacts Post links to CI-built packages/containers.
You can use this to test packages locally.

Note that the @BiocondaBot please merge command 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

    • Introduced a build script to streamline the installation process for the Pantax software package across different operating systems.
    • Added a metadata file for the Pantax package, detailing its version, dependencies, and functionality for taxonomic classification of metagenomic data.
  • Documentation

    • Included comprehensive metadata about the package, including its dependencies, build requirements, and testing commands.

Copy link

coderabbitai bot commented Sep 26, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces a build.sh script and a meta.yaml file for the Pantax software package. The build.sh script manages the installation process across different operating systems, handling tasks such as copying files, installing Python packages, and setting up executables. The meta.yaml file defines the package's metadata, including its name, version, dependencies, and testing commands, focusing on strain-level taxonomic classification of metagenomic data.

Changes

Files Change Summary
recipes/pantax/build.sh Introduced a script to manage the installation process for Pantax, including OS checks, installation of Python packages, and setup of executables and tools.
recipes/pantax/meta.yaml Created a metadata file defining the package's name, version, source URLs, dependencies, and testing commands, focusing on bioinformatics tools and functionalities related to metagenomic data classification.

Possibly related PRs

  • Add recipe for consensify #50969: The build.sh script in this PR introduces a shell script for the consensify project, similar to the build.sh script in the main PR for pantax, as both scripts automate the installation process for their respective packages.
  • recipe for pgrc #50973: The build.sh script for pgrc also automates the build process, similar to the build.sh in the main PR, indicating a common approach to package installation across different projects.
  • trnascan-se: add aarch64/arm64 build #50996: The build.sh script for tRNAscan-SE includes modifications to automate the build process, akin to the changes made in the build.sh for pantax, highlighting a shared focus on enhancing build automation in these recipes.

Suggested labels

please review & merge

Suggested reviewers

  • martin-g
  • apeltzer

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (4)
recipes/pantax/build.sh (1)

1-19: Overall script review and suggestions for improvement.

The build script covers the installation process for various components of the Pantax package, including Gurobi, VG tools, Pantax scripts, and Fastix. While it provides a good foundation, there are several areas where it could be improved:

  1. Error Handling: Implement consistent error handling throughout the script, especially for directory changes and critical operations.
  2. Flexibility: Make the script more adaptable to different Python versions and system configurations.
  3. Specificity: Be more specific when copying files to avoid including unnecessary items.
  4. Dependency Checks: Add checks for required tools and dependencies (e.g., Cargo) before attempting to use them.
  5. Documentation: Consider adding comments to explain the purpose of each major section and any non-obvious operations.
  6. Cleanup: Ensure temporary files or directories are cleaned up after installation.

These improvements will make the script more robust, maintainable, and less prone to errors during the installation process.

🧰 Tools
🪛 Shellcheck

[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 12-12: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 16-16: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

recipes/pantax/meta.yaml (3)

8-18: LGTM: Sources are well-defined with proper checksums.

The source section is well-structured with URLs and SHA256 checksums for each component. The use of specific folders for Gurobi and VG is appropriate.

Consider adding a comment to explain why the Gurobi wheel and VG binary are included as separate sources. This would improve clarity for future maintainers.

🧰 Tools
🪛 yamllint

[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


45-47: Consider adding more comprehensive tests.

The current test command pantax -h is a good basic check to ensure the CLI is accessible. However, to improve the robustness of the package, consider adding more comprehensive tests. For example:

  1. Check the version: pantax --version
  2. Run a simple classification task with a small test dataset
  3. Verify the presence of key dependencies: python -c "import pandas, numpy, networkx"

Adding these tests would help ensure that the core functionality of the package is working as expected after installation.


1-56: Minor: Remove trailing spaces for better code hygiene.

The static analysis tool reported several instances of trailing spaces in the YAML file. While these don't affect functionality, removing them improves code cleanliness and adheres to best practices.

Please remove trailing spaces from the following lines:

  • Line 12
  • Line 13
  • Line 14
  • Line 16
  • Line 17
  • Line 23

You can use your text editor's "trim trailing whitespace" feature or a command-line tool like sed to automatically remove these spaces.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 23-23: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 02fdfed and dea4574.

📒 Files selected for processing (2)
  • recipes/pantax/build.sh (1 hunks)
  • recipes/pantax/meta.yaml (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/pantax/build.sh

[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 12-12: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 16-16: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

🪛 yamllint
recipes/pantax/meta.yaml

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 23-23: trailing spaces

(trailing-spaces)

🔇 Additional comments (5)
recipes/pantax/build.sh (1)

1-5: Verify the macOS installation process.

The current implementation for macOS simply copies the entire source directory to the prefix directory. This approach might not be the intended behavior and could potentially include unnecessary files or miss important installation steps.

Please confirm if this is the correct installation process for macOS. If not, consider implementing a more comprehensive installation procedure similar to the one for other operating systems.

recipes/pantax/meta.yaml (4)

1-6: LGTM: Package name and version are well-defined.

The use of Jinja2 templating for the package name and version is a good practice. The version number follows semantic versioning, which is appropriate for package management.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


20-23: LGTM: Build section is correctly configured.

The build number is appropriately set to 0 for a new package. The use of run_exports with pin_subpackage is a good practice for maintaining ABI compatibility. The "x.x" pin allows updates to patch versions, which is generally a safe approach.

🧰 Tools
🪛 yamllint

[error] 23-23: trailing spaces

(trailing-spaces)


49-52: LGTM: About section is informative and complete.

The about section provides all necessary information: the home page URL, license (GPL-3.0), and a concise summary of the package's functionality. This is excellent for users and maintainers to quickly understand the purpose and terms of use for the package.


54-56: Please clarify the reason for skipping the should_be_noarch_python lint.

The skip-lints section is used to ignore the should_be_noarch_python lint. While this may be necessary due to the nature of the package (bioinformatics tool with compiled dependencies), it would be helpful to have a comment explaining why this lint is being skipped. This will help future maintainers understand the reasoning behind this decision.

Could you please add a comment explaining why the should_be_noarch_python lint is being skipped? This will improve the maintainability of the package.

Comment on lines 9 to 11
mkdir -p ${PREFIX}/bin/tools
cd ../vg
cp vg* ${PREFIX}/bin/tools/vg
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and file copying specificity.

  1. Add error handling for the cd command:
-cd ../vg
+cd ../vg || exit 1
  1. Consider being more specific when copying files to avoid including unnecessary files:
-cp vg* ${PREFIX}/bin/tools/vg
+cp vg vg_* ${PREFIX}/bin/tools/

This assumes that the main executable is named vg and any related files start with vg_. Adjust the pattern as needed for your specific use case.

📝 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.

Suggested change
mkdir -p ${PREFIX}/bin/tools
cd ../vg
cp vg* ${PREFIX}/bin/tools/vg
mkdir -p ${PREFIX}/bin/tools
cd ../vg || exit 1
cp vg vg_* ${PREFIX}/bin/tools/
🧰 Tools
🪛 Shellcheck

[warning] 10-10: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

Comment on lines 16 to 18
cd $SRC_DIR/tools/fastix
cargo install fastix --root ./
cp $SRC_DIR/tools/fastix/bin/fastix ${PREFIX}/bin/tools
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and add Cargo availability check.

  1. Add error handling for the cd command:
-cd $SRC_DIR/tools/fastix
+cd $SRC_DIR/tools/fastix || exit 1
  1. Add a check for Cargo availability before attempting to install Fastix:
+if ! command -v cargo &> /dev/null; then
+    echo "Cargo is not installed. Please install Rust and Cargo to build Fastix."
+    exit 1
+fi
cargo install fastix --root ./

This change ensures that the script fails gracefully if Cargo is not available on the system.

📝 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.

Suggested change
cd $SRC_DIR/tools/fastix
cargo install fastix --root ./
cp $SRC_DIR/tools/fastix/bin/fastix ${PREFIX}/bin/tools
cd $SRC_DIR/tools/fastix || exit 1
if ! command -v cargo &> /dev/null; then
echo "Cargo is not installed. Please install Rust and Cargo to build Fastix."
exit 1
fi
cargo install fastix --root ./
cp $SRC_DIR/tools/fastix/bin/fastix ${PREFIX}/bin/tools
🧰 Tools
🪛 Shellcheck

[warning] 16-16: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

Comment on lines 12 to 15
cd $SRC_DIR/scripts
chmod +x pantax
cp $SRC_DIR/scripts/pantax ${PREFIX}/bin
cp $SRC_DIR/scripts/*py ${PREFIX}/bin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and file copying specificity.

  1. Add error handling for the cd command:
-cd $SRC_DIR/scripts
+cd $SRC_DIR/scripts || exit 1
  1. Consider being more specific when copying Python files to avoid including unnecessary files:
-cp $SRC_DIR/scripts/*py ${PREFIX}/bin
+cp $SRC_DIR/scripts/{file1.py,file2.py,file3.py} ${PREFIX}/bin

Replace file1.py, file2.py, etc., with the actual names of the Python scripts that need to be copied. This approach ensures that only the necessary files are included in the installation.

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Shellcheck

[warning] 12-12: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

Comment on lines 6 to 8
cd gurobi11
${PYTHON} -m pip install gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
rm gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and Python version flexibility.

  1. Add error handling for the cd command to ensure the script exits if the directory change fails:
-cd gurobi11
+cd gurobi11 || exit 1
  1. Consider using a more flexible approach for the Python wheel file to support different Python versions:
-${PYTHON} -m pip install gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
+${PYTHON} -m pip install gurobipy-11.0.2-cp${PYTHON_VERSION/./}-cp${PYTHON_VERSION/./}-manylinux2014_x86_64.manylinux_2_17_x86_64.whl

This assumes that the PYTHON_VERSION environment variable is available and set correctly.

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Shellcheck

[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

Comment on lines 25 to 44
requirements:
build:
- {{ compiler('rust') }}
host:
- python ==3.10
- pip
run:
- python =3.10
- samtools =1.19.2
- bcftools =1.19
- htslib =1.19.1
- pggb =0.5.4
- graphaligner =1.0.17
- h5py =3.10.0
- pandas =2.2.0
- tqdm =4.66.1
- numpy =1.26.3
- networkx =3.2.1
- time =1.8

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider relaxing Python version constraint and review 'time' dependency.

The requirements section is generally well-structured with specific versions for dependencies, which is good for reproducibility. However, there are two points to consider:

  1. Python version is strictly pinned to 3.10. Consider allowing a range of Python versions (e.g., >=3.10,<3.11) to provide more flexibility for users while maintaining compatibility.

  2. The 'time' package is listed as a run dependency with version 1.8. This is unusual because 'time' is typically a built-in Python module. Please verify if this is a different 'time' package or if it can be removed from the dependencies.

Please review these points and update the requirements section accordingly.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
recipes/pantax/meta.yaml (4)

20-24: LGTM: Build section is well-defined.

The build section is correctly structured with:

  • Skip for OSX platforms
  • Build number set to 0
  • Proper run exports definition

Consider adding a comment explaining why the build is skipped for OSX, for better maintainability. For example:

build:
  skip: True  # [osx] # Pantax is not compatible with OSX due to <reason>
🧰 Tools
🪛 yamllint

[error] 24-24: trailing spaces

(trailing-spaces)


46-48: LGTM: Basic test is present. Consider adding more comprehensive tests.

The current test command pantax -h verifies that the pantax CLI is accessible and can display help information, which is a good basic check.

Consider adding more comprehensive tests to ensure proper functionality of the package. For example:

  • Test a simple classification task with a small sample dataset.
  • Verify the output format of the classification results.
  • Check if all required dependencies are properly linked and functional.

55-57: LGTM: Lint skip is likely necessary. Consider adding a comment.

The 'should_be_noarch_python' lint is skipped, which is probably necessary given the nature of the package (bioinformatics tool with compiled dependencies).

Consider adding a brief comment explaining why this lint is skipped for better maintainability. For example:

extra:
  skip-lints:
    - should_be_noarch_python  # Pantax requires architecture-specific compiled dependencies

1-57: Minor: Clean up trailing spaces and ignore false positive yamllint warning.

The yamllint tool reported several trailing spaces in the file. While these don't affect functionality, removing them improves code style.

Please remove trailing spaces from lines 12, 13, 14, 16, 17, and 24.

Note: The yamllint warning about the '%' character at the beginning of the file is a false positive. This character is part of the Jinja2 templating syntax used in Conda recipes and is correct in this context.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between dea4574 and 5b16876.

📒 Files selected for processing (1)
  • recipes/pantax/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/pantax/meta.yaml

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)

🔇 Additional comments (4)
recipes/pantax/meta.yaml (4)

1-6: LGTM: Package name and version are correctly defined.

The package name "pantax" and version "1.0.1" are properly set using Jinja2 templating, which is a good practice for Conda recipes.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


8-18: LGTM: Sources are well-defined with proper versioning and checksums.

The source section is correctly structured with:

  1. Main package source (pantax.tar.gz)
  2. Gurobi wheel for Python 3.10
  3. VG binary

Each source has a specific URL and SHA256 checksum, which is excellent for ensuring reproducibility. The use of designated folders for additional sources is also a good practice.

🧰 Tools
🪛 yamllint

[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


26-44: Consider relaxing Python version constraint and review 'time' dependency.

The requirements section is well-structured with specific versions for dependencies. However, there are two points to consider:

  1. Python version is strictly pinned to 3.10. Consider allowing a range of Python versions (e.g., >=3.10,<3.11) to provide more flexibility for users while maintaining compatibility.

  2. The 'time' package is listed as a run dependency with version 1.8. This is unusual because 'time' is typically a built-in Python module. Please verify if this is a different 'time' package or if it can be removed from the dependencies.

These issues were previously mentioned in a past review comment. Please review and update the requirements section accordingly.


50-53: LGTM: About section is concise and informative.

The about section provides essential information:

  • Home page URL
  • License (GPL-3.0)
  • A brief summary of the package's functionality

This information is sufficient for users to understand the purpose and licensing of the package.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
recipes/pantax/meta.yaml (2)

46-48: LGTM: Basic test is present, but consider adding more comprehensive tests.

The current test checks if the CLI is accessible, which is a good start. However, consider adding more comprehensive tests to ensure the functionality of the package. For example:

  1. Test the import of the Python package (if applicable).
  2. Run a small example dataset through the main functionality.
  3. Check the output of a specific subcommand.

1-57: Minor: Remove trailing spaces for better code cleanliness.

The static analysis tool reported several instances of trailing spaces. While these don't affect functionality, removing them improves code cleanliness. Consider using an editor with a trailing space removal feature or running a command to remove them.

You can use the following command to remove trailing spaces:

sed -i 's/[[:space:]]*$//' recipes/pantax/meta.yaml

Note: The syntax error reported for the Jinja2 templating appears to be a false positive and can be ignored.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)


[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5b16876 and fe19494.

📒 Files selected for processing (1)
  • recipes/pantax/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/pantax/meta.yaml

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)


[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)

🔇 Additional comments (6)
recipes/pantax/meta.yaml (6)

1-6: LGTM: Package and version section looks good.

The package name and version are correctly defined using Jinja2 templating, which is a good practice for maintainability.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


20-24: LGTM: Build section is well-defined, but verify OSX skip.

The build section correctly specifies the build number and uses run_exports for version consistency. However, the OSX build is skipped.

Please confirm that skipping the OSX build is intentional and necessary for this package. You may want to check if there are any OS-specific dependencies or incompatibilities that justify this decision.

🧰 Tools
🪛 yamllint

[error] 24-24: trailing spaces

(trailing-spaces)


26-44: Consider relaxing Python version constraint and review 'time' dependency.

The requirements section is well-structured with specific versions for dependencies, which is good for reproducibility. However, there are two points to consider:

  1. Python version is strictly pinned to 3.10. Consider allowing a range of Python versions (e.g., >=3.10,<3.11) to provide more flexibility for users while maintaining compatibility.

  2. The 'time' package is listed as a run dependency with version 1.8. This is unusual because 'time' is typically a built-in Python module. Please verify if this is a different 'time' package or if it can be removed from the dependencies.

These points were raised in a previous review comment. Please address them if they haven't been resolved.

🧰 Tools
🪛 yamllint

[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)


50-53: LGTM: About section is complete and accurate.

The about section provides all necessary information including the home page, license (GPL-3.0), and a concise summary of the package's functionality.


55-57: LGTM: Extra section is present, but verify the lint skip.

The extra section correctly specifies a lint to skip. This aligns with the earlier decision to skip OSX builds, suggesting that the package is not architecture-independent.

Please confirm that skipping the 'should_be_noarch_python' lint is necessary for this package. This decision should be consistent with the package's architecture dependencies and the reason for skipping OSX builds.


8-18: LGTM: Source section is well-defined, but verify the multiple sources approach.

The source section correctly specifies URLs and SHA256 checksums for the main package, Gurobi wheel, and VG binary. This approach ensures reproducibility and security. However, the use of multiple sources is somewhat unusual.

Please confirm that this multiple sources approach is necessary for the package and aligns with Bioconda best practices. You may want to run the following command to check if similar packages use this approach:

🧰 Tools
🪛 yamllint

[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)

@zwh82
Copy link
Contributor Author

zwh82 commented Sep 26, 2024

@BiocondaBot please fetch artifacts

@BiocondaBot
Copy link
Collaborator

Package(s) built are ready for inspection:

Arch Package Zip File / Repodata CI Instructions
linux-64 pantax-1.0.1-py310h4349ce8_0.tar.bz2 LinuxArtifacts.zip Azure
showYou may also use conda to install after downloading and extracting the zip file. From the LinuxArtifacts directory: conda install -c ./packages <package name>

Docker image(s) built:

Package Tag CI Install with docker
pantax 1.0.1--py310h4349ce8_0 Azure
showImages for Azure are in the LinuxArtifacts zip file above.gzip -dc LinuxArtifacts/images/pantax:1.0.1--py310h4349ce8_0.tar.gz | docker load

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between fe19494 and 15006dd.

📒 Files selected for processing (1)
  • recipes/pantax/build.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/pantax/build.sh

[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 15-15: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 21-21: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

🔇 Additional comments (1)
recipes/pantax/build.sh (1)

21-23: ⚠️ Potential issue

Improve error handling and add Cargo availability check.

  1. Add error handling for the cd command:
-cd $SRC_DIR/tools/fastix
+cd $SRC_DIR/tools/fastix || exit 1
  1. Add a check for Cargo availability before attempting to install Fastix:
+if ! command -v cargo &> /dev/null; then
+    echo "Cargo is not installed. Please install Rust and Cargo to build Fastix."
+    exit 1
+fi
cargo install fastix --root ./

This change ensures that the script fails gracefully if Cargo is not available on the system.

Likely invalid or redundant comment.

🧰 Tools
🪛 Shellcheck

[warning] 21-21: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

recipes/pantax/build.sh Show resolved Hide resolved
Comment on lines 10 to 13
mkdir -p ${PREFIX}/bin/tools
cd ${PREFIX}/vg
cp vg* ${PREFIX}/bin/tools/vg
chmod +x ${PREFIX}/bin/tools/vg
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and file copying specificity.

  1. Add error handling for the cd command:
-cd ${PREFIX}/vg
+cd ${PREFIX}/vg || exit 1
  1. Consider being more specific when copying files to avoid including unnecessary files:
-cp vg* ${PREFIX}/bin/tools/vg
+cp vg vg_* ${PREFIX}/bin/tools/

This assumes that the main executable is named vg and any related files start with vg_. Adjust the pattern as needed for your specific use case.

📝 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.

Suggested change
mkdir -p ${PREFIX}/bin/tools
cd ${PREFIX}/vg
cp vg* ${PREFIX}/bin/tools/vg
chmod +x ${PREFIX}/bin/tools/vg
mkdir -p ${PREFIX}/bin/tools
cd ${PREFIX}/vg || exit 1
cp vg vg_* ${PREFIX}/bin/tools/
chmod +x ${PREFIX}/bin/tools/vg
🧰 Tools
🪛 Shellcheck

[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

Comment on lines 15 to 19
cd $SRC_DIR/scripts
chmod +x pantax
chmod +x data_preprocessing
cp $SRC_DIR/scripts/pantax $SRC_DIR/scripts/data_preprocessing ${PREFIX}/bin
cp $SRC_DIR/scripts/*py ${PREFIX}/bin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and file copying specificity.

  1. Add error handling for the cd command:
-cd $SRC_DIR/scripts
+cd $SRC_DIR/scripts || exit 1
  1. Consider being more specific when copying Python files to avoid including unnecessary files:
-cp $SRC_DIR/scripts/*py ${PREFIX}/bin
+cp $SRC_DIR/scripts/{pantax.py,data_preprocessing.py} ${PREFIX}/bin

Replace pantax.py and data_preprocessing.py with the actual names of the Python scripts that need to be copied. This approach ensures that only the necessary files are included in the installation.

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Shellcheck

[warning] 15-15: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

Comment on lines 6 to 8
cd ${PREFIX}/gurobi11
${PYTHON} -m pip install gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
rm gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error handling and Python version flexibility.

  1. Add error handling for the cd command:
-cd ${PREFIX}/gurobi11
+cd ${PREFIX}/gurobi11 || exit 1
  1. Make the Python wheel installation more flexible:
-${PYTHON} -m pip install gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
+${PYTHON} -m pip install gurobipy-11.0.2-cp${PYTHON_VERSION/./}-cp${PYTHON_VERSION/./}-manylinux2014_x86_64.manylinux_2_17_x86_64.whl

This assumes that the PYTHON_VERSION environment variable is available and set correctly. If it's not, you may need to extract it from ${PYTHON} or set it explicitly.

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Shellcheck

[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
recipes/pantax/meta.yaml (4)

8-18: LGTM: Source section is well-defined, but consider adding comments for clarity.

The source section correctly specifies multiple sources with their respective URLs and SHA256 checksums. This is good for security and reproducibility. However, to improve clarity, consider adding comments explaining the purpose of each source, especially for the Gurobi wheel and VG binary.

For example:

source:
  - url: https://github.com/LuoGroup2023/PanTax/releases/download/v{{ version }}/{{ name }}.tar.gz  
    sha256: c14a46b41f580cf39ad502049d267cd31bbfd4dc05028215cc669c0e52f58ce7
    # Main pantax package

  - url: https://files.pythonhosted.org/packages/07/0f/6039cf6e22f9cbec57cdedff103949e4856c18fd2cb714bc70cdc8b52941/gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl 
    sha256: 164e72462c2f1a705fcec73acc060aa639e745827bfe877714ff7b2ccabc5237  
    folder: gurobi11  
    # Gurobi optimization library wheel

  - url: https://github.com/vgteam/vg/releases/download/v1.52.0/vg    
    sha256: 07776b51aa6494b45982e4134eb84005ddc743e81c67e61fa52bff211d81777c 
    folder: vg
    # Variation graph toolkit binary

This addition would make the purpose of each source more evident to maintainers and contributors.

🧰 Tools
🪛 yamllint

[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


26-44: Consider relaxing the Python version constraint.

The requirements section is well-structured with specific versions for dependencies, which is excellent for reproducibility. However, the Python version is strictly pinned to 3.10. Consider allowing a range of Python versions (e.g., >=3.10,<3.11) to provide more flexibility for users while maintaining compatibility.

For example:

  host:
    - python >=3.10,<3.11
    - pip
  run:
    - python >=3.10,<3.11

This change would allow for minor version updates of Python 3.10 while ensuring compatibility with the Gurobi wheel.

The rest of the requirements look good, with appropriate versions specified for all dependencies.

🧰 Tools
🪛 yamllint

[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)


46-48: LGTM: Basic test present. Consider adding more comprehensive tests.

The current test ensures that the pantax command is available and can be executed, which is a good basic check. However, to improve the robustness of the package, consider adding more comprehensive tests. For example:

  1. Check the version of the installed package:

    - pantax --version
  2. Run a simple analysis on a small test dataset (if possible):

    - pantax --test-data
  3. Verify the presence of key dependencies:

    - python -c "import pandas; import numpy; import networkx"

These additional tests would provide more confidence in the correct installation and functionality of the package.


1-57: Minor: Remove trailing spaces for better code style.

The static analysis tool detected trailing spaces on several lines (12, 13, 14, 16, 17, 24, 37, 38). While these don't affect functionality, removing them would improve code style and consistency. Consider using an editor with a trailing space removal feature or running a cleanup script to address this.

Note: The syntax error reported for the Jinja2 templating can be safely ignored as it's valid in the context of a Conda recipe.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)


[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 15006dd and fa9a0c5.

📒 Files selected for processing (1)
  • recipes/pantax/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/pantax/meta.yaml

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)


[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)

🔇 Additional comments (4)
recipes/pantax/meta.yaml (4)

1-6: LGTM: Package and version section looks good.

The package name and version are correctly defined using Jinja2 templating, which is a good practice for maintainability.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


20-24: LGTM: Build section is well-configured.

The build section is correctly configured:

  • Skipping the build for OSX is noted and likely intentional.
  • The build number is appropriately set to 0 for the initial release.
  • The run_exports section ensures proper pinning in dependent packages, using Jinja2 templating for maintainability.
🧰 Tools
🪛 yamllint

[error] 24-24: trailing spaces

(trailing-spaces)


50-53: LGTM: About section is concise and informative.

The about section provides all necessary information:

  • The home URL is correctly specified.
  • The license (GPL-3.0) is clearly stated, which is crucial for open-source software.
  • The summary succinctly describes the package's purpose.

This section effectively communicates the essential details about the package.


55-57: LGTM: Extra section correctly specifies lint skip.

The extra section appropriately specifies skipping the "should_be_noarch_python" lint. This is correct given that the package includes architecture-specific binaries (such as the VG binary), making it not suitable for a noarch package.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between fa9a0c5 and dfb06a8.

📒 Files selected for processing (1)
  • recipes/pantax/build.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/pantax/build.sh

[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 15-15: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)


[warning] 21-21: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

recipes/pantax/build.sh Show resolved Hide resolved
recipes/pantax/build.sh Show resolved Hide resolved
recipes/pantax/build.sh Show resolved Hide resolved
recipes/pantax/build.sh Show resolved Hide resolved
recipes/pantax/build.sh Show resolved Hide resolved
recipes/pantax/meta.yaml Outdated Show resolved Hide resolved
- python ==3.10
- pip
run:
- python =3.10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not recommend to use those very strict pins. Are they needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this setting can match the version of my tool, which is beneficial for fully reproducing my running results.


- url: https://github.com/vgteam/vg/releases/download/v1.52.0/vg
sha256: 07776b51aa6494b45982e4134eb84005ddc743e81c67e61fa52bff211d81777c
folder: vg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you instead require VG in the requirements section?

Copy link
Contributor Author

@zwh82 zwh82 Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is impossible. The requirement PGGB works with vg 1.40, but I need updated versions (vg 1.52) in other parts of my codes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://anaconda.org/bioconda/vg provides a newer version

- url: https://github.com/LuoGroup2023/PanTax/releases/download/v{{ version }}/{{ name }}.tar.gz
sha256: c14a46b41f580cf39ad502049d267cd31bbfd4dc05028215cc669c0e52f58ce7

- url: https://files.pythonhosted.org/packages/07/0f/6039cf6e22f9cbec57cdedff103949e4856c18fd2cb714bc70cdc8b52941/gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please require this as a dependency and do not pip install them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gurobipy can be install with conda install gurobi::gurobi and pip. But I don't know how to set channels gurobi to install it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use only dependencies from conda-forge and bioconda channels. You cannot install from third-party channels and/or Pip

@zwh82
Copy link
Contributor Author

zwh82 commented Sep 27, 2024

@BiocondaBot please fetch artifacts

@BiocondaBot
Copy link
Collaborator

Package(s) built are ready for inspection:

Arch Package Zip File / Repodata CI Instructions
linux-64 pantax-1.0.1-py310h4349ce8_0.tar.bz2 LinuxArtifacts.zip Azure
showYou may also use conda to install after downloading and extracting the zip file. From the LinuxArtifacts directory: conda install -c ./packages <package name>

Docker image(s) built:

Package Tag CI Install with docker
pantax 1.0.1--py310h4349ce8_0 Azure
showImages for Azure are in the LinuxArtifacts zip file above.gzip -dc LinuxArtifacts/images/pantax:1.0.1--py310h4349ce8_0.tar.gz | docker load

Co-authored-by: Martin Grigorov <[email protected]>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)
recipes/pantax/meta.yaml (4)

20-24: Consider supporting OSX builds.

The build section is generally well-configured:

  • The build number is correctly set to 0.
  • The run_exports section is properly used to pin the subpackage.

However, the package is currently skipped for OSX builds. If possible, consider supporting OSX to increase the package's availability across different platforms.

🧰 Tools
🪛 yamllint

[error] 24-24: trailing spaces

(trailing-spaces)


26-31: Consider relaxing the Python version constraint.

The build requirements are generally well-defined:

  • The inclusion of the Rust compiler is appropriate if the package contains Rust code.
  • pip is correctly included in the host requirements for installing Python packages during the build process.

However, the Python version is strictly pinned to 3.10. Consider allowing a range of Python versions (e.g., >=3.10,<3.11) to provide more flexibility for users while maintaining compatibility.


1-57: Clean up trailing spaces.

The static analysis tool reported several instances of trailing spaces in the YAML file. While these don't affect functionality, removing them improves code cleanliness. Consider using an editor with a trailing space removal feature or running a cleanup script to address this.

You can use the following command to remove trailing spaces:

sed -i 's/[[:space:]]*$//' recipes/pantax/meta.yaml
🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)


[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)


1-57: Summary: Well-structured package with areas for improvement

Overall, the meta.yaml file for the pantax package is well-structured and provides a good foundation. However, there are several areas where improvements could be made:

  1. Dependency Management: Consider relaxing version constraints where appropriate, especially for Python and some of the bioinformatics tools. This will improve maintainability and flexibility.

  2. Build Process: The inclusion of pre-built binaries (Gurobi wheel and VG) may pose compatibility and security risks. Consider alternative approaches, such as building from source or using conda-forge packages.

  3. Test Coverage: Enhance the test suite to provide more comprehensive coverage of the package's functionality.

  4. Platform Support: If possible, consider adding support for OSX builds to increase the package's availability.

  5. Code Cleanliness: Remove trailing spaces throughout the file for better maintainability.

These improvements will enhance the package's robustness, maintainability, and user-friendliness. Please address these points and feel free to ask for clarification or assistance on any of the suggested changes.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)


[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between dfb06a8 and 1bef665.

📒 Files selected for processing (1)
  • recipes/pantax/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/pantax/meta.yaml

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)


[error] 24-24: trailing spaces

(trailing-spaces)


[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)

🔇 Additional comments (4)
recipes/pantax/meta.yaml (4)

1-6: LGTM: Package metadata is well-defined.

The package name and version are correctly set using Jinja2 variables, which is a good practice for maintainability. The version number follows semantic versioning.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


50-53: LGTM: About section is well-defined.

The about section provides all necessary information:

  • The home URL correctly points to the GitHub repository.
  • The license is properly specified as GPL-3.0-or-later.
  • The summary is concise and informative, clearly describing the package's functionality.

55-57: Clarify the need to skip the noarch Python lint.

The package is set to skip the "should_be_noarch_python" lint, which suggests it contains architecture-specific components. This is consistent with the inclusion of pre-built binaries in the source section.

However, it's worth considering if this skip is necessary:

  1. Could the architecture-specific components be separated into a different package?
  2. Is it possible to build these components from source instead of including pre-built binaries?

Please clarify why the package cannot be noarch Python. If possible, consider refactoring to make it architecture-independent, as this could simplify distribution and installation.


8-18: Review the source section for potential issues.

While the source section is comprehensive, there are a few concerns:

  1. The inclusion of pre-built binaries (Gurobi wheel and VG) might pose compatibility issues across different systems.
  2. Downloading the VG binary directly from GitHub releases could be a security risk. Consider packaging VG separately or using a more controlled distribution method.
  3. The use of a specific Python wheel for Gurobi (cp310) limits the package to Python 3.10, which may reduce flexibility.

To ensure the VG binary is compatible and secure, please run the following script:

Consider refactoring this section to improve compatibility and security.

🧰 Tools
🪛 yamllint

[error] 12-12: trailing spaces

(trailing-spaces)


[error] 13-13: trailing spaces

(trailing-spaces)


[error] 14-14: trailing spaces

(trailing-spaces)


[error] 16-16: trailing spaces

(trailing-spaces)


[error] 17-17: trailing spaces

(trailing-spaces)

Comment on lines +32 to +44
run:
- python =3.10
- samtools =1.19.2
- bcftools =1.19
- htslib =1.19.1
- pggb =0.5.4
- graphaligner =1.0.17
- h5py =3.10.0
- pandas =2.2.0
- tqdm =4.66.1
- numpy =1.26.3
- networkx =3.2.1
- pyarrow =14.0.2
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Review dependency version constraints.

The run requirements section is comprehensive, but there are some points to consider:

  1. The Python version is strictly pinned to 3.10, which limits flexibility. Consider allowing a range of compatible Python versions.

  2. Many dependencies have very specific version requirements. While this ensures reproducibility, it may make updates challenging. Consider using version ranges (e.g., >=1.19.2,<1.20) for dependencies that are likely to remain compatible across minor versions.

  3. For bioinformatics tools like samtools, bcftools, and htslib, consider if such strict version pinning is necessary. These tools often maintain backwards compatibility, so allowing minor version updates might be beneficial.

  4. Verify if all listed dependencies are directly required by pantax. Remove any that are indirect dependencies to keep the requirements list focused.

Consider relaxing version constraints where appropriate to improve maintainability while preserving compatibility.

🧰 Tools
🪛 yamllint

[error] 37-37: trailing spaces

(trailing-spaces)


[error] 38-38: trailing spaces

(trailing-spaces)

Comment on lines +46 to +48
test:
commands:
- pantax -h
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage.

The current test section includes a basic smoke test to verify that the pantax CLI is accessible and can display help information. While this is a good start, consider adding more comprehensive tests to ensure the package's functionality. Some suggestions:

  1. Add a test that runs a simple analysis on a small dataset.
  2. Verify the output of a known input to ensure correctness.
  3. Test different command-line options to ensure they work as expected.

Expanding the test suite will increase confidence in the package's functionality and help catch potential issues early.

@zwh82
Copy link
Contributor Author

zwh82 commented Sep 27, 2024

@BiocondaBot please add label

@BiocondaBot BiocondaBot added the please review & merge set to ask for merge label Sep 27, 2024
@zwh82
Copy link
Contributor Author

zwh82 commented Oct 2, 2024

@martin-g Could you please take some time to review and merge it? Thank you so much.

cp -rf $SRC_DIR $PREFIX
else
cd ${SRC_DIR}/gurobi11
${PYTHON} -m pip install gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is not allowed to install dependencies with Pip like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does it install it, by the way ? It looks like it is no inside $PREFIX

Copy link
Contributor Author

@zwh82 zwh82 Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Are there any rules that restrict this installation behavior? I have already installed the artifact locally and tested it sucessfully, and PanTax is works well. Gurobipy, as a Python package, should be installed in the /path/to/env/lib/python3.10/site-packages/gurobipy. ${PREFIX} should be /path/to/env/bin,so it will certainly not be installed there.

Copy link
Contributor Author

@zwh82 zwh82 Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the question of vg version. I use vg version 1.52 instead of newer version to ensure the robustness and reproducibility of our tool. All my experiments are run on this version. In addition, I have a dependency PGGB. PGGB depends on vg version 1.40. Therefore, I cannot directly add vg to the dependency, otherwise the vg version 1.40 will be overwritten by the new version of vg.

- url: https://github.com/LuoGroup2023/PanTax/releases/download/v{{ version }}/{{ name }}.tar.gz
sha256: c14a46b41f580cf39ad502049d267cd31bbfd4dc05028215cc669c0e52f58ce7

- url: https://files.pythonhosted.org/packages/07/0f/6039cf6e22f9cbec57cdedff103949e4856c18fd2cb714bc70cdc8b52941/gurobipy-11.0.2-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use only dependencies from conda-forge and bioconda channels. You cannot install from third-party channels and/or Pip


- url: https://github.com/vgteam/vg/releases/download/v1.52.0/vg
sha256: 07776b51aa6494b45982e4134eb84005ddc743e81c67e61fa52bff211d81777c
folder: vg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://anaconda.org/bioconda/vg provides a newer version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please review & merge set to ask for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants