-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
base: master
Are you sure you want to change the base?
Conversation
Using PyPI skeleton recipes.
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces new metadata files, Similarly, the 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 (
|
There was a problem hiding this 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 directiveThe
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 testsWhile 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 dependenciesThis 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:
- Add tests for key functionalities of the
qtl
package.- Include a test that runs a simple QTL analysis with sample data.
- 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: - pipThese 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 availableThis 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_maintainer2Having multiple maintainers can improve the long-term sustainability of the package in the Bioconda ecosystem.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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 goodThe source URL and SHA256 checksum are correctly specified, ensuring the integrity of the package source.
19-30
: Requirements section is well-definedThe 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-definedThe about section provides concise and relevant information about the package, including the home URL, summary, and license details.
46-48
: Extra section is correctly specifiedThe recipe maintainer is properly listed, and it aligns with the PR author mentioned in the PR objectives.
1-6
: Verify package name consistencyThe 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:
- Verify the necessity of the
run_exports
section for therna-seqc-python
subpackage.- Consider adding version constraints to critical dependencies in the
run
requirements.- Enhance the test suite with more comprehensive checks of the package's functionality.
- Add a
doc_url
field in theabout
section if documentation is available.- 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)
recipes/qtl/meta.yaml
Outdated
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") }} |
There was a problem hiding this comment.
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 theqtl
package. rna-seqc-python
is not listed insetup.py
orpyproject.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
There was a problem hiding this 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.8Please 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: - piprecipes/rna-seqc-python/meta.yaml (3)
12-17
: LGTM: Build configuration is well-definedThe 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-specifiedThe 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 adequateThe 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 checkThis addition would verify that the installed version matches the expected version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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-definedThe 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 specifiedThe 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 conciseThe 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 providedThe extra section correctly lists the recipe maintainer, which matches the PR author mentioned in the PR objectives.
1-1
: Note on static analysis warningThe 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)
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") }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
Used
grayskull
to generate PyPI skeleton recipes.