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 rna-seqc python utilities and qtl package #51344

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

Conversation

lrvdijk
Copy link
Contributor

@lrvdijk lrvdijk commented Oct 11, 2024

Used grayskull to generate PyPI skeleton recipes.

Copy link
Contributor

coderabbitai bot commented Oct 11, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces new metadata files, meta.yaml, for two packages: qtl and rna-seqc-python. The meta.yaml for the qtl package specifies its name as "qtl" and version "0.1.8", along with the source URL from PyPI and a SHA256 checksum for verification. It outlines the build process as architecture-independent and includes a run_exports directive to pin the package itself. The requirements section lists dependencies for both host and runtime environments, including several essential Python packages. Testing commands are provided to ensure package integrity.

Similarly, the meta.yaml for the rna-seqc-python package sets its name and version to "rna-seqc-python" and "0.0.3," respectively, and includes a source URL and SHA256 checksum. It also defines its build as architecture-independent and specifies dependencies, including qtl. The testing section includes commands for verifying the installation. Both files include an about section detailing the package's functionality and licensing information, as well as an extra section listing recipe maintainers.

Possibly related PRs

  • add pantax #50983: The rna-seqc-python package's meta.yaml includes qtl as a runtime requirement, indicating a direct dependency on the qtl package introduced in the main PR.
  • Update tbtamr dependencies #51140: The tbtamr package's meta.yaml has been updated to include mutamr as a dependency, which is relevant as both mutamr and qtl are part of the bioinformatics domain, although this is a more indirect relationship.

Suggested labels

please review & merge


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
Contributor

@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: 1

🧹 Outside diff range and nitpick comments (7)
recipes/rna-seqc-python/meta.yaml (2)

12-17: Consider removing the run_exports directive

The run_exports directive is typically used for compiled libraries to ensure ABI compatibility. For a pure Python package, this might be unnecessary. Consider removing it unless there's a specific reason for its inclusion.

If you decide to keep it, please add a comment explaining the rationale.


32-38: Consider adding more specific tests

While the current tests cover basic functionality and dependency checks, consider adding more specific tests to ensure the package works as expected. For example, you could include a simple function call or data processing test.

Would you like assistance in generating additional test commands?

recipes/qtl/meta.yaml (5)

8-10: LGTM: Source URL and checksum are correctly specified.

The source URL is properly constructed using Jinja2 templating, following the standard PyPI format. The SHA256 checksum is provided, which is crucial for verifying the integrity of the downloaded package.

Consider adding a comment above the sha256 line to indicate the version of the package this checksum corresponds to. This can be helpful when updating the package in the future.

  # sha256 for version 0.1.8
  sha256: 8fdb99cda1ceff578a233db6c15a944fa57b43a2826af41c292e36848906117b

19-31: LGTM: Requirements are well-defined, but consider version pinning for critical dependencies.

The host and run requirements are appropriately specified for a Python package focused on QTL analysis.

Consider adding version constraints to critical dependencies to ensure compatibility. For example:

  run:
    - python >=3.6
    - numpy >=1.18
    - pandas >=1.0
    # Add version constraints for other critical dependencies

This can help prevent potential issues with incompatible package versions while still allowing for some flexibility.


33-39: LGTM: Basic tests are in place, but consider adding more comprehensive tests.

The current test configuration provides a basic check for package importability and dependency compatibility.

Consider enhancing the test suite with more comprehensive checks:

  1. Add tests for key functionalities of the qtl package.
  2. Include a test that runs a simple QTL analysis with sample data.
  3. Verify that all submodules can be imported correctly.

Example of additional tests:

test:
  imports:
    - qtl
    - qtl.core  # Add other important submodules
  commands:
    - pip check
    - python -c "import qtl; qtl.run_sample_analysis()"  # Add a sample analysis function
  requires:
    - pip

These additional tests would provide greater confidence in the package's functionality.


41-45: LGTM: About section is informative and well-structured.

The about section provides essential metadata for the package, including the home page, a concise summary, and proper license information.

Consider adding a doc_url field to point users to more detailed documentation if available. For example:

about:
  home: https://github.com/broadinstitute/pyqtl
  summary: Utilities for analyzing and visualizing QTL data
  license: BSD-3-Clause
  license_file: LICENSE
  doc_url: https://github.com/broadinstitute/pyqtl#readme  # Add this line if documentation is available

This would make it easier for users to find additional information about the package.


47-49: LGTM: Maintainer information is provided.

The extra section correctly lists the recipe maintainer, which is crucial for package maintenance.

Consider adding more maintainers to the list if possible. This can help ensure continued maintenance of the package even if the primary maintainer becomes unavailable. For example:

extra:
  recipe-maintainers:
    - lrvdijk
    # - additional_maintainer1
    # - additional_maintainer2

Having multiple maintainers can improve the long-term sustainability of the package in the Bioconda ecosystem.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4f7fa12 and 324d35f.

📒 Files selected for processing (2)
  • recipes/qtl/meta.yaml (1 hunks)
  • recipes/rna-seqc-python/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/qtl/meta.yaml

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

(syntax)

recipes/rna-seqc-python/meta.yaml

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

(syntax)

🔇 Additional comments (7)
recipes/rna-seqc-python/meta.yaml (5)

8-10: Source section looks good

The source URL and SHA256 checksum are correctly specified, ensuring the integrity of the package source.


19-30: Requirements section is well-defined

The host and run requirements are appropriately specified. The inclusion of the qtl package as a dependency aligns with the PR objectives.


40-44: About section is well-defined

The about section provides concise and relevant information about the package, including the home URL, summary, and license details.


46-48: Extra section is correctly specified

The recipe maintainer is properly listed, and it aligns with the PR author mentioned in the PR objectives.


1-6: Verify package name consistency

The package name "rna-seqc-python" differs from the PyPI name "rnaseqc". This might cause confusion or issues during installation. Consider aligning the package name with the PyPI name or provide a clear reason for the difference.

To check if this discrepancy is intentional or documented, run:

🧰 Tools
🪛 yamllint

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

(syntax)

recipes/qtl/meta.yaml (2)

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

The package name and version are properly set using Jinja2 templating, which is a best practice for Conda recipes. This allows for easy updates and maintenance of the package metadata.

🧰 Tools
🪛 yamllint

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

(syntax)


1-49: Overall, the meta.yaml file is well-structured and follows Conda recipe best practices.

Great job on creating this recipe for the qtl package. The file is generally well-organized and includes all necessary sections. Here's a summary of the key points to address:

  1. Verify the necessity of the run_exports section for the rna-seqc-python subpackage.
  2. Consider adding version constraints to critical dependencies in the run requirements.
  3. Enhance the test suite with more comprehensive checks of the package's functionality.
  4. Add a doc_url field in the about section if documentation is available.
  5. Consider adding more maintainers to ensure long-term sustainability.

Addressing these points will further improve the recipe and make it more robust and maintainable.

🧰 Tools
🪛 yamllint

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

(syntax)

Comment on lines 12 to 17
build:
noarch: python
script: {{ PYTHON }} -m pip install . -vv --no-deps --no-build-isolation
number: 0
run_exports:
- {{ pin_subpackage("rna-seqc-python", max_pin="x.x") }}
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

The run_exports section appears unnecessary as no dependencies on rna-seqc-python were found. Please remove it to simplify the build configuration.

  • No imports or references to rna-seqc-python detected in the qtl package.
  • rna-seqc-python is not listed in setup.py or pyproject.toml.
🔗 Analysis chain

Build configuration looks good, but please verify the run_exports section.

The build configuration is well-structured:

  • noarch: python is correctly set for a pure Python package.
  • The pip install script uses the recommended flags for Conda builds.
  • The build number is correctly set to 0 for a new package.

However, the run_exports section is unusual for a Python package. Please verify if this is intentional and necessary. If it is, consider adding a comment explaining why this pinning is required.

To verify the relationship between qtl and rna-seqc-python, you can run the following script:

This script will help determine if there's a strong dependency between qtl and rna-seqc-python that justifies the run_exports pinning.


[/run_scripts]

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for dependencies between qtl and rna-seqc-python

# Search for imports of rna-seqc-python in qtl package
echo "Searching for imports of rna-seqc-python in qtl package:"
rg --type python 'import.*rna[-_]seqc' || echo "No direct imports found"

# Search for other references to rna-seqc-python
echo "\nSearching for other references to rna-seqc-python:"
rg --type python 'rna[-_]seqc' || echo "No other references found"

# Check if rna-seqc-python is listed in setup.py or pyproject.toml
echo "\nChecking for rna-seqc-python in setup.py or pyproject.toml:"
rg --type python 'rna[-_]seqc' -g 'setup.py' -g 'pyproject.toml' || echo "Not found in setup files"

Length of output: 777


Script:

#!/bin/bash
# Description: Check for dependencies between qtl and rna-seqc-python

# Search for imports of rna-seqc-python in qtl package
echo "Searching for imports of rna-seqc-python in qtl package:"
rg 'import.*rna[-_]seqc' --glob '*.py' || echo "No direct imports found"

# Search for other references to rna-seqc-python
echo "\nSearching for other references to rna-seqc-python:"
rg 'rna[-_]seqc' --glob '*.py' || echo "No other references found"

# Check if rna-seqc-python is listed in setup.py or pyproject.toml
echo "\nChecking for rna-seqc-python in setup.py or pyproject.toml:"
rg 'rna[-_]seqc' --glob 'setup.py' --glob 'pyproject.toml' || echo "Not found in setup files"

Length of output: 678

Copy link
Contributor

@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: 1

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

19-31: Consider adding version constraints to critical dependencies.

The requirements section looks good overall, with appropriate host and run dependencies listed. However, to ensure long-term stability and reproducibility, consider adding version constraints to critical dependencies.

Here's an example of how you might add version constraints:

requirements:
  host:
    - python >=3.6
    - pip
  run:
    - python >=3.6
    - numpy >=1.18
    - pandas >=1.0
    - scipy >=1.4
    - matplotlib-base >=3.2
    - seaborn >=0.10
    - pybigwig >=0.3
    - bx-python >=0.8

Please adjust the version numbers based on the minimum versions required by your package. This will help prevent potential compatibility issues in the future.


33-39: LGTM: Test section is well-defined. Consider adding more comprehensive tests.

The test section includes appropriate checks:

  • Import test for the qtl module.
  • pip check command to verify package integrity.
  • Correct inclusion of pip as a test requirement.

To further improve the test coverage, consider adding more specific tests if possible. For example:

  • Check for the presence of important functions or classes within the qtl module.
  • Add a simple functional test that exercises core functionality of the package.

Example:

test:
  imports:
    - qtl
  commands:
    - pip check
    - python -c "from qtl import some_important_function; assert some_important_function()"
  requires:
    - pip
recipes/rna-seqc-python/meta.yaml (3)

12-17: LGTM: Build configuration is well-defined

The build configuration is appropriate for a pure Python package. The noarch: python setting and custom build script using pip are correct. The run_exports directive is a good practice for version pinning.

Consider adding a comment explaining the purpose of the --no-deps --no-build-isolation flags in the build script for better maintainability.


19-30: LGTM: Requirements are well-specified

The host and run requirements are appropriately defined. The inclusion of the qtl package aligns with the PR objectives.

Consider specifying minimum versions for critical dependencies to ensure compatibility and reproducibility. For example:

run:
  - python >=3.6
  - numpy >=1.18
  - pandas >=1.0

32-38: LGTM: Test configuration is adequate

The test configuration includes basic checks for package integrity, which is good. The import check and pip check are essential.

Consider enhancing the test suite by adding a simple functional test. For example:

commands:
  - python -c "import rnaseqc; assert rnaseqc.__version__ == '{{ version }}'"
  - pip check

This addition would verify that the installed version matches the expected version.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 324d35f and 624351d.

📒 Files selected for processing (2)
  • recipes/qtl/meta.yaml (1 hunks)
  • recipes/rna-seqc-python/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/qtl/meta.yaml

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

(syntax)

recipes/rna-seqc-python/meta.yaml

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

(syntax)

🔇 Additional comments (9)
recipes/qtl/meta.yaml (4)

1-10: LGTM: Package and source information are correctly defined.

The package name, version, source URL, and SHA256 checksum are all properly specified. This ensures that the correct version of the package will be downloaded and verified during the build process.

🧰 Tools
🪛 yamllint

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

(syntax)


41-45: LGTM: About section is complete and accurate.

The about section provides all necessary metadata:

  • Home URL is correctly specified.
  • A concise summary of the package functionality is provided.
  • The license (BSD-3-Clause) is clearly stated.
  • The license file is correctly referenced.

This information is crucial for users to understand the package's purpose and terms of use.


47-49: LGTM: Recipe maintainer is correctly specified.

The extra section correctly lists the recipe maintainer (lrvdijk), which matches the PR author mentioned in the PR objectives. This is important for future maintenance and updates of the package in the Bioconda ecosystem.


1-2: Ignore the yamllint syntax error for Jinja2 templating.

The static analysis tool (yamllint) reports a syntax error for the '%' characters at the beginning of the file. This is a false positive due to the use of Jinja2 templating in the YAML file.

The '%' characters are part of the Jinja2 syntax and are valid in this context. You can safely ignore this warning, as it does not indicate an actual problem with your configuration.

🧰 Tools
🪛 yamllint

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

(syntax)

recipes/rna-seqc-python/meta.yaml (5)

1-6: LGTM: Package information is well-defined

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

🧰 Tools
🪛 yamllint

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

(syntax)


8-10: LGTM: Source information is correctly specified

The source URL is properly formatted for PyPI downloads, and the inclusion of a SHA256 checksum is a good security practice to ensure the integrity of the downloaded package.


40-44: LGTM: About section is informative and concise

The about section provides essential metadata, including the home page, a brief summary, and clear license information. This is helpful for users and maintainers.


46-48: LGTM: Maintainer information is correctly provided

The extra section correctly lists the recipe maintainer, which matches the PR author mentioned in the PR objectives.


1-1: Note on static analysis warning

The yamllint tool reported a syntax error for the '%' character. This is a false positive due to the use of Jinja2 templating in the meta.yaml file. The '%' character is valid in this context and does not require any changes.

🧰 Tools
🪛 yamllint

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

(syntax)

Comment on lines +12 to +17
build:
noarch: python
script: {{ PYTHON }} -m pip install . -vv --no-deps --no-build-isolation
number: 0
run_exports:
- {{ pin_subpackage("qtl", max_pin="x.x") }}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider removing the run_exports section.

The build configuration looks good overall:

  • noarch: python is correctly set for a pure Python package.
  • The pip install script uses the recommended flags for Conda builds.

However, the run_exports section is unusual for a Python package and may not be necessary. Unless there's a specific reason for pinning the qtl package version in downstream dependencies, consider removing this section to simplify the build configuration.

If you decide to keep the run_exports section, please add a comment explaining why this pinning is required. Otherwise, you can remove it by applying this diff:

 build:
   noarch: python
   script: {{ PYTHON }} -m pip install . -vv --no-deps --no-build-isolation
   number: 0
-  run_exports:
-    - {{ pin_subpackage("qtl", max_pin="x.x") }}
📝 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
build:
noarch: python
script: {{ PYTHON }} -m pip install . -vv --no-deps --no-build-isolation
number: 0
run_exports:
- {{ pin_subpackage("qtl", max_pin="x.x") }}
build:
noarch: python
script: {{ PYTHON }} -m pip install . -vv --no-deps --no-build-isolation
number: 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant