Add PySCENIC #50445
Add PySCENIC #50445
Conversation
|
@LiliyaBioinf I would suggest looking at other recipes in bioconda that patch code, to see how they deal with that. I would expect the patching to need to happen on a build.sh (but I might be wrong - so I suggest that you check at other recipes that do this). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
recipes/pyscenic/meta.yaml (3)
13-14: Clarify the purpose of the patch file.The recipe includes a patch file
dill_patch.patch. It's good practice to document the purpose of this patch and ensure it's necessary. Consider adding a comment explaining why this patch is needed and what it modifies in the source code.
66-68: Enhance test coverage.The current test only checks if the package can be imported. Consider adding more comprehensive tests to ensure the package functions correctly. Some suggestions:
- Add command line interface (CLI) tests if pyscenic provides any CLI tools.
- Include a simple functional test that exercises core functionality.
- Check if key submodules can be imported.
Example:
test: imports: - pyscenic - pyscenic.cli - pyscenic.utils commands: - pyscenic --help # If there's a CLI - python -c "from pyscenic import Module; assert Module('test').name == 'test'" # Simple functional test
70-74: Enhance package metadata.The current about section provides good basic information. Consider adding more metadata to improve the package's discoverability and provide more context for users. Some suggestions:
- Add a
doc_urlpointing to the package's documentation.- Include a
dev_urllinking to the GitHub repository.- Add
descriptionwith a more detailed explanation of the package's functionality.- Include
license_fileto specify the location of the license file in the source distribution.Example:
about: home: https://github.com/aertslab/pySCENIC summary: 'pySCENIC is a python implementation of the SCENIC pipeline' description: | pySCENIC (Single-Cell rEgulatory Network Inference and Clustering) is a computational method to infer Gene Regulatory Networks and cell types from single-cell RNA-seq data. license: MIT license_family: MIT license_file: LICENSE doc_url: https://pyscenic.readthedocs.io/ dev_url: https://github.com/aertslab/pySCENIC
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/pyscenic/build.sh (1 hunks)
- recipes/pyscenic/meta.yaml (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/pyscenic/build.sh
[error] 1-1: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 2-2: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 3-3: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 4-4: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 5-5: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
🪛 yamllint
recipes/pyscenic/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (2)
recipes/pyscenic/meta.yaml (2)
1-74: Overall recipe review summary.The recipe for pyscenic is generally well-structured and follows many Bioconda best practices. Here's a summary of the key points and suggestions for improvement:
- The package and source sections are correctly defined, including SHA256 checksums for security.
- The build section includes
run_exportsas requested, but the build process could be simplified.- Dependencies are comprehensive, but some version constraints may need review, and the
multiprocessing_on_dilldependency needs to be addressed.- Test coverage could be expanded to ensure package functionality.
- The about section could benefit from additional metadata to improve discoverability and user information.
Addressing these points will enhance the recipe's quality, maintainability, and user experience. Once these suggestions are implemented, the recipe should be ready for inclusion in the Bioconda channel.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
23-65:⚠️ Potential issueReview and update dependency specifications.
The Python version constraint (>=3.10,<3.11) might be too restrictive. Consider allowing a broader range of Python versions if possible, e.g.,
python >=3.10.Some dependencies have very specific version constraints. Review these and consider if they can be relaxed:
dask >=2023.4.1distributed >=2023.4.1,<2023.5.0numpy ==1.23.5The
multiprocessing_on_dillpackage is missing from the dependencies. As discussed in past comments, this package has licensing issues. Consider one of the following options:
a. Work with the upstream authors to resolve the licensing issue and add it to conda-forge.
b. Include it as a pip-installed dependency in the build script, although this is not preferred by the Bioconda team.
c. Find an alternative solution that doesn't require this package.Please choose the most appropriate solution for handling the
multiprocessing_on_dilldependency based on the options provided above.To verify the impact of these changes, you can run the following script:
recipes/pyscenic/build.sh
Outdated
| #!/bin/bash | ||
| set -e |
There was a problem hiding this comment.
Remove carriage returns and approve set -e usage.
The shebang and set -e usage are correct. However, the static analysis tool detected carriage returns which can cause issues on Unix-like systems.
Please remove the carriage returns from the entire file. You can use the following command:
tr -d '\r' < build.sh > build_unix.sh && mv build_unix.sh build.shThis will ensure compatibility across different operating systems.
🧰 Tools
🪛 Shellcheck
[error] 1-1: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
[error] 2-2: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
There was a problem hiding this comment.
@coderabbitai pause
There was a problem hiding this comment.
@LiliyaBioinf, if you'd like to pause CodeRabbit's reviews on this pull request, you can use the command @coderabbitai stop in a comment. This will prevent CodeRabbit from posting further comments on this PR.
If you want to disable CodeRabbit for this repository entirely, you can adjust the repository's settings:
- Go to the repository's Settings tab.
- Click on Integrations or Installed GitHub Apps.
- Find CodeRabbit in the list of installed apps.
- Click on Configure and unselect this repository from CodeRabbit's access.
Let me know if you need any further assistance.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the CodeRabbit FAQ to find out how to pause CodeRabbit
curl -s https://docs.coderabbit.ai/faq/
Length of output: 34416
recipes/pyscenic/build.sh
Outdated
| set -e | ||
| patch -p1 < ${RECIPE_DIR}/dill_patch.patch | ||
|
|
||
| $PYTHON -m pip install . --no-deps --ignore-installed -vv |
There was a problem hiding this comment.
Consider alternatives to using pip for installation.
While the pip installation command is correctly formatted, using pip within a Conda recipe is generally discouraged by the Bioconda/core team.
According to the PR objectives, the consensus is against using pip to install additional packages within recipes. Instead, consider the following alternatives:
- Use
$PYTHON setup.py installor$PYTHON -m buildfollowed by installation of the built wheel. - If
setup.pyis not available, useconda_build.build.build_wheel()to create a wheel, then install it.
Example:
$PYTHON -m build --wheel
$PYTHON -m pip install dist/*.whl --no-deps --ignore-installed -vvThis approach aligns better with Conda's package management philosophy and helps maintain consistency within the Bioconda ecosystem.
🧰 Tools
🪛 Shellcheck
[error] 5-5: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
recipes/pyscenic/build.sh
Outdated
| @@ -0,0 +1,5 @@ | |||
| #!/bin/bash | |||
| set -e | |||
| patch -p1 < ${RECIPE_DIR}/dill_patch.patch | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing dill_patch.patch file in RECIPE_DIR.
The build script is attempting to apply dill_patch.patch from ${RECIPE_DIR}, but the patch file was not found. Please ensure that dill_patch.patch is included in the repository and located in the appropriate directory referenced by RECIPE_DIR.
🔗 Analysis chain
Verify the contents and necessity of 'dill_patch.patch'.
The patch application looks correct. However, it's important to ensure that the patch file is necessary and contains the expected modifications.
Please verify the contents of the patch file:
Also, consider documenting the purpose of this patch in a comment above the patch application line.
'RECIPE_DIR' environment variable is not set or incorrect.
The build script failed to locate dill_patch.patch because RECIPE_DIR appears to be empty or incorrectly set. Please verify the value of the RECIPE_DIR environment variable by running the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Display the contents of the dill_patch.patch file
# Test: Check if the patch file exists and display its contents
if [ -f "${RECIPE_DIR}/dill_patch.patch" ]; then
echo "Contents of dill_patch.patch:"
cat "${RECIPE_DIR}/dill_patch.patch"
else
echo "Error: dill_patch.patch not found in ${RECIPE_DIR}"
fi
Length of output: 115
Script:
#!/bin/bash
# Description: Display the value of RECIPE_DIR
echo "RECIPE_DIR is set to: '${RECIPE_DIR}'"
Length of output: 64
Script:
#!/bin/bash
# Description: Search the codebase for assignments or usages of RECIPE_DIR
echo "Searching for assignments or usages of RECIPE_DIR in the codebase..."
rg --ignore-case 'RECIPE_DIR' .
Length of output: 41950
🧰 Tools
🪛 Shellcheck
[error] 3-3: Literal carriage return. Run script through tr -d '\r' .
(SC1017)
recipes/pyscenic/meta.yaml
Outdated
| noarch: python | ||
| run_exports: | ||
| - "{{ pin_subpackage(name, max_pin='x.x') }}" | ||
| script: bash ${RECIPE_DIR}/build.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the build process.
Consider simplifying the build process by using the standard pip install command instead of a separate build script. This approach is more common for Python packages in Bioconda and easier to maintain.
Replace:
script: bash ${RECIPE_DIR}/build.shwith:
script: "{{ PYTHON }} -m pip install . -vv"If there are specific build steps required, consider incorporating them directly in the meta.yaml file or provide a clear rationale for using a separate build script.
recipes/pyscenic/meta.yaml
Outdated
| source: | ||
| - url: "https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gz" | ||
| sha256: "{{ sha256 }}" | ||
| - path: dill_patch.patch | ||
| sha256: "{{ patch_sha256 }}" |
There was a problem hiding this comment.
It seems to be that in other recipes they handle patches like this:
source:
url: https://github.com/kcleal/gw/archive/refs/tags/v{{ version }}.tar.gz
sha256: {{ sha256 }}
patches:
- fix-linker-issues.patch # [linux]
- fix-context.patch
and they don't seem to do anything on the build.sh. Can you please try like this and removing the patching part from the build? I suspect you might not need the build if this patching mechanism works. Thanks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
recipes/pyscenic/meta.yaml (2)
65-67: Consider expanding test coverage.The current test section includes a basic import check for pyscenic, which is a good start.
To improve the robustness of the package, consider adding more comprehensive tests. For example:
- Check for the presence of key functions or classes within the pyscenic module.
- Add a simple functional test that exercises core functionality.
- Include checks for expected command-line tools, if any are provided by the package.
Example expansion:
test: imports: - pyscenic commands: - python -c "from pyscenic import cli; assert hasattr(cli, 'pyscenic')" - pyscenic --help
69-73: LGTM: About section is concise and accurate.The about section provides essential information about the package, including the home URL, summary, and license details.
Consider adding a more detailed description of the package to help users understand its purpose and capabilities. You could include this under a
descriptionkey. For example:about: home: https://github.com/aertslab/pySCENIC summary: 'pySCENIC is a python implementation of the SCENIC pipeline' description: | pySCENIC (Single-Cell rEgulatory Network Inference and Clustering) is a computational method to infer Gene Regulatory Networks and cell types from single-cell RNA-seq data. license: MIT license_family: MIT
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipes/pyscenic/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/pyscenic/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (3)
recipes/pyscenic/meta.yaml (3)
1-7: LGTM: Package metadata is well-defined.The package metadata section is correctly structured using Jinja2 variables for the name, version, and SHA256 checksum. This approach enhances maintainability and reduces the risk of inconsistencies.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
15-20: LGTM: Build configuration follows best practices.The build section is well-configured:
- Correct use of build number and noarch: python.
- Proper implementation of run_exports for dependency management.
- Standard pip installation command for Python packages.
This configuration aligns with Bioconda best practices and should ensure consistent builds.
9-13: Verify the necessity of the patch file.The source section is well-structured, with the URL correctly using Jinja2 variables. However, the inclusion of 'dill_patch.patch' warrants attention.
Could you please verify if this patch is still necessary? If possible, consider upstreaming these changes to reduce maintenance overhead. If the patch is still required, please provide a brief explanation of its purpose in a comment.
recipes/pyscenic/meta.yaml
Outdated
| requirements: | ||
| build: | ||
| - python >=3.10,<3.11 | ||
| - pip | ||
| - setuptools | ||
| - wheel | ||
|
|
||
| host: | ||
| - python >=3.10,<3.11 | ||
| - pip | ||
| - setuptools | ||
| - wheel | ||
|
|
||
| run: | ||
| - python >=3.10,<3.11 | ||
| - aiohttp | ||
| - arboreto | ||
| - attrs | ||
| - boltons | ||
| - cloudpickle | ||
| - cytoolz | ||
| - dill | ||
| - dask >=2023.4.1 | ||
| - distributed >=2023.4.1,<2023.5.0 | ||
| - frozendict | ||
| - fsspec | ||
| - llvmlite | ||
| - loompy | ||
| - networkx | ||
| - numba >=0.51.2 | ||
| - numexpr | ||
| - pandas >=1.3.5 | ||
| - pyyaml | ||
| - requests | ||
| - scikit-learn | ||
| - scipy | ||
| - tqdm | ||
| - umap-learn | ||
| - numpy ==1.23.5 | ||
| - pyarrow | ||
| - interlap | ||
| - ctxcore | ||
|
|
There was a problem hiding this comment.
Review and update dependency specifications.
The requirements section is well-structured, but there are some points to consider:
-
The Python version constraint (>=3.10,<3.11) might be too restrictive. Consider allowing a broader range of Python versions if possible.
-
Some version constraints might be unnecessarily strict:
- numpy is pinned to ==1.23.5
- distributed is constrained to >=2023.4.1,<2023.5.0
-
The
multiprocessing_on_dillpackage is missing from the dependencies. As discussed in the past comments, this package has licensing issues.
Consider the following changes:
- python >=3.10,<3.11
+ python >=3.10
- numpy ==1.23.5
+ numpy >=1.23.5
- distributed >=2023.4.1,<2023.5.0
+ distributed >=2023.4.1Regarding multiprocessing_on_dill:
- Work with the upstream authors to resolve the licensing issue and add it to conda-forge.
- If the above is not possible in the short term, consider including it as a pip-installed dependency in the build script, although this is not preferred by the Bioconda team.
- Investigate if there's an alternative solution that doesn't require this package.
Would you like assistance in implementing these changes or investigating alternatives for multiprocessing_on_dill?
Committable suggestion was skipped due to low confidence.
|
@aliciaaevans can you help me, please, by reviewing of what I have done - patching multiprocessing_of_dill. I created a patch file like in saw in bioconda recepies, but after running more than 30 min it is usually canceled. What could be the problem? How do deal with that? |
Looks like it might be running out of RAM (or possibly CPU, but between the number of dependencies and some other PRs with a similar error, I'm guessing RAM). The runners we use have |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
recipes/pyscenic/meta.yaml (1)
15-26: LGTM with a suggestion: Build configuration is well-structured.The build section is correctly configured with appropriate settings for a Python package. The inclusion of
run_exportsis excellent for maintaining compatibility.However, consider modifying the installation script:
- script: "{{ PYTHON }} -m pip install . --no-deps --no-build-isolation --no-cache-dir -vvv" + script: "{{ PYTHON }} -m pip install . --no-deps --ignore-installed --no-cache-dir -vvv"This change removes
--no-build-isolationand adds--ignore-installed, which may help prevent potential issues with existing installations while still allowing proper dependency resolution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipes/pyscenic/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/pyscenic/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (3)
recipes/pyscenic/meta.yaml (3)
1-7: LGTM: Package metadata is well-defined.The package metadata section is correctly structured using Jinja2 variables for the name, version, and SHA256 checksum. The package name is appropriately lowercased, which is a good practice.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
9-13: LGTM: Source configuration is correct.The source section is well-structured with a correctly templated URL and the inclusion of the
dill_patch.patchfile. This patch likely addresses themultiprocessing_on_dillreplacement discussed in previous comments.
66-79: LGTM: Comprehensive metadata provided.The about and extra sections are well-structured and provide comprehensive metadata for the package. The inclusion of DOI and biotools identifiers is excellent for discoverability and citation purposes.
| test: | ||
| imports: | ||
| - pyscenic |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage.
While the import check is a good start, consider adding more comprehensive tests for a package of this complexity.
Suggested additions:
test:
imports:
- pyscenic
commands:
- pyscenic --help
- db2feather --help
- csv2loom --help
- invertdb --help
- gmt2regions --helpThese additional tests would ensure that the CLI entry points are working correctly.
recipes/pyscenic/meta.yaml
Outdated
| requirements: | ||
| host: | ||
| - python >=3.6 | ||
| - pip | ||
| run: | ||
| - python >=3.6 | ||
| - aiohttp | ||
| - arboreto | ||
| - attrs | ||
| - boltons | ||
| - cloudpickle | ||
| - cytoolz | ||
| - dill | ||
| - dask-core >=2023.4.1 | ||
| - distributed >=2023.4.1,<2023.5.0 | ||
| - frozendict | ||
| - fsspec | ||
| - llvmlite | ||
| - loompy | ||
| - networkx | ||
| - numba >=0.51.2 | ||
| - numexpr | ||
| - pandas >=1.3.5 | ||
| - pyyaml | ||
| - requests | ||
| - scikit-learn | ||
| - scipy | ||
| - tqdm | ||
| - umap-learn | ||
| - numpy ==1.23.5 | ||
| - pyarrow | ||
| - interlap | ||
| - ctxcore |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating dependency specifications.
The requirements section is comprehensive, but there are some points to consider:
-
The Python version constraint (>=3.6) might be too permissive. Consider raising the minimum version to 3.8 or higher, as many of the dependencies likely require a more recent Python version.
-
Some version constraints might need updating:
- numpy is pinned to ==1.23.5, which might be too restrictive. Consider allowing a range of compatible versions.
- distributed is constrained to >=2023.4.1,<2023.5.0, which might prevent using newer versions.
-
The absence of
multiprocessing_on_dillis correct, as it's been replaced by the standardmultiprocessingmodule via the patch.
Consider the following changes:
- python >=3.6
+ python >=3.8
- numpy ==1.23.5
+ numpy >=1.23.5,<2
- distributed >=2023.4.1,<2023.5.0
+ distributed >=2023.4.1These changes would allow for more flexibility in dependency versions while still maintaining compatibility.
📝 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.
| requirements: | |
| host: | |
| - python >=3.6 | |
| - pip | |
| run: | |
| - python >=3.6 | |
| - aiohttp | |
| - arboreto | |
| - attrs | |
| - boltons | |
| - cloudpickle | |
| - cytoolz | |
| - dill | |
| - dask-core >=2023.4.1 | |
| - distributed >=2023.4.1,<2023.5.0 | |
| - frozendict | |
| - fsspec | |
| - llvmlite | |
| - loompy | |
| - networkx | |
| - numba >=0.51.2 | |
| - numexpr | |
| - pandas >=1.3.5 | |
| - pyyaml | |
| - requests | |
| - scikit-learn | |
| - scipy | |
| - tqdm | |
| - umap-learn | |
| - numpy ==1.23.5 | |
| - pyarrow | |
| - interlap | |
| - ctxcore | |
| requirements: | |
| host: | |
| - python >=3.8 | |
| - pip | |
| run: | |
| - python >=3.8 | |
| - aiohttp | |
| - arboreto | |
| - attrs | |
| - boltons | |
| - cloudpickle | |
| - cytoolz | |
| - dill | |
| - dask-core >=2023.4.1 | |
| - distributed >=2023.4.1 | |
| - frozendict | |
| - fsspec | |
| - llvmlite | |
| - loompy | |
| - networkx | |
| - numba >=0.51.2 | |
| - numexpr | |
| - pandas >=1.3.5 | |
| - pyyaml | |
| - requests | |
| - scikit-learn | |
| - scipy | |
| - tqdm | |
| - umap-learn | |
| - numpy >=1.23.5,<2 | |
| - pyarrow | |
| - interlap | |
| - ctxcore |
|
@aliciaaevans yes, i ran it locally and the package built. It was on 1 core and 16 Gb RAM. It also seemed to me that it lacked memory here, because I tried different approaches of patching. |
|
Don't worry about the patching as long as it is correctly applied, memory wise the patching should be negligible. |
|
We've switched from Azure to GitHub actions because of the memory issues. Hopefully it will complete now. |
|
The recipe looks good to me, anything else to add @aliciaaevans? |
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
|
Describe your pull request here
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add labelcommand.@bioconda/corein a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports(see here for the rationale and comprehensive explanation).Add a
run_exportssection like this:with
...being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}while replacing
"myrecipe"with eithernameif aname|lowervariable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update@BiocondaBot please add labelplease review & mergelabel.@BiocondaBot please fetch artifactsYou can use this to test packages locally.
Note that the
@BiocondaBot please mergecommand is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>and will automatically re-post them to notify the addressed<team>.Summary by CodeRabbit
pyscenic, detailing versioning and dependencies.pyscenicpackage.multiprocessinglibrary, improving compatibility and stability.