add r-archr
#51295
add r-archr
#51295
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive metadata configuration file for the R package The Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
recipes/r-archr/meta.yaml (3)
23-84: LGTM! Consider optimizing requirements.The build, host, and run requirements are comprehensive and well-defined. They align with the package's purpose and ensure proper compilation across platforms.
As an optimization, consider using YAML anchors to reduce duplication between host and run requirements, as they are identical. This can make future maintenance easier.
Here's an example of how you could use YAML anchors:
requirements: build: # ... (keep existing build requirements) host: &r_requirements - r-base - bioconductor-biocgenerics # ... (list all shared requirements) run: *r_requirementsThis change would make the file more maintainable and less prone to inconsistencies between host and run requirements.
86-89: LGTM! Consider adding more comprehensive tests.The test commands are correctly defined for both Windows and non-Windows environments. However, the current test only checks if the library can be loaded.
Consider adding more comprehensive tests to ensure key functionalities of the package work as expected. For example:
test: commands: - $R -e "library(ArchR); stopifnot(packageVersion('ArchR') == '{{ version }}')" # [not win] - $R -e "library(ArchR); data(examplePeakSet); stopifnot(!is.null(examplePeakSet))" # [not win] - "\"%R%\" -e \"library(ArchR); stopifnot(packageVersion('ArchR') == '{{ version }}')\"" # [win] - "\"%R%\" -e \"library(ArchR); data(examplePeakSet); stopifnot(!is.null(examplePeakSet))\"" # [win]These additional tests verify the package version and check if example data can be loaded.
102-122: LGTM! Consider updating or removing commented information.The extra section correctly lists the maintainers, including @jdblischak as mentioned in the PR objectives.
The commented-out package information at the end of the file (lines 107-122) provides useful context but doesn't affect the recipe. Consider either removing this information or updating it to match the current package version and details. If you decide to keep it, you might want to add a comment explaining why this information is retained in the recipe file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/r-archr/build.sh (1 hunks)
- recipes/r-archr/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/r-archr/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (9)
recipes/r-archr/build.sh (4)
1-1: LGTM: Correct shebang for a bash script.The shebang
#!/bin/bashis correctly specified, ensuring the script is interpreted by bash.
2-2: LGTM: Correctly disables auto-brewing.Setting
DISABLE_AUTOBREW=1is a good practice in Bioconda recipes. It prevents the use of Homebrew's auto-brewing feature, ensuring that the package is built using only the specified dependencies in the recipe. This is crucial for maintaining a controlled and reproducible build environment.
3-3: LGTM: Correct R package installation command.The R package installation command
${R} CMD INSTALL --build . ${R_ARGS}is correctly structured:
${R}refers to the R executable (set by Conda build system).CMD INSTALL --buildbuilds the package from source..specifies the current directory as the package source location.${R_ARGS}allows for additional arguments, providing flexibility.This command follows the standard format for installing R packages in Conda recipes.
1-3: LGTM: Well-structured build script for r-archr package.The
build.shscript is concise, well-structured, and follows best practices for Bioconda R package recipes. It correctly disables auto-brewing and uses the standard R installation command. The script should work as expected for building the r-archr package in a Conda environment.recipes/r-archr/meta.yaml (5)
10-12: LGTM! Source and checksum properly defined.The source URL correctly points to the specific commit archive on GitHub, and the SHA256 checksum is provided for integrity verification.
14-21: LGTM! Please clarify the build number.The build configuration looks good, including proper
rpathsandrun_exportswith pinning. However, the build number (8) seems high for a new package. Could you please clarify why it's set to 8?
91-100: LGTM! Metadata is well-defined.The about section provides comprehensive metadata, including correct URLs for the project homepage, development, and documentation. The license information is accurately specified, and the summary succinctly describes the package's purpose.
1-122: Overall, the recipe looks good with minor suggestions for improvement.The
meta.yamlfile for ther-archrpackage is well-structured and follows Bioconda guidelines. Here's a summary of the review:
- The package version and commit-specific build are correctly implemented.
- Source URL and checksum are properly defined.
- Build configuration includes necessary settings and good practices like
run_exports.- Requirements are comprehensive, though they could be optimized using YAML anchors.
- Basic tests are in place, but more comprehensive tests could be added.
- Metadata and license information are well-defined.
- Maintainers are correctly listed.
Consider addressing the following points:
- Clarify the reason for the high build number (8).
- Optimize requirements using YAML anchors.
- Add more comprehensive tests.
- Update or remove the commented package information at the end of the file.
Once these minor points are addressed, the recipe should be ready for inclusion in Bioconda.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
1-8: LGTM! Verify the specific commit used.The package name and version are correctly defined, following Bioconda conventions. The use of a specific commit (8th commit after v1.0.2) aligns with the PR objectives.
To confirm the commit is the 8th after v1.0.2, run:
The expected output should be 8.
✅ Verification successful
Verified commit is the 8th after v1.0.2.
The specific commit used aligns with the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the commit is the 8th after v1.0.2 git clone https://github.com/GreenleafLab/ArchR.git cd ArchR git log --oneline v1.0.2..d9e741c980c7c64e5348c97a74d146cc95f8ba76 | wc -lLength of output: 2685
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
recipes/r-archr/meta.yaml (4)
10-12: LGTM: Source information is correctly specified.The source URL and SHA256 checksum are properly defined. Using a specific commit hash aligns with the PR objectives.
Consider adding a comment explaining why this specific commit was chosen, for future reference. For example:
source: url: {{ github }}/archive/{{ commit }}.zip sha256: aef7bd08d55494348cac8b9e0645a71a22a41dacbb7a7843cd939f1f31ee6d87 # 8th commit after v1.0.2, includes minor bug fixes
23-84: LGTM: Requirements are well-defined and comprehensive.The requirements section is thorough, with appropriate compiler specifications for different platforms and a comprehensive list of R and Bioconductor package dependencies. The use of version constraints where necessary is good practice.
Consider using Jinja2 variables for version numbers that appear multiple times, such as the R-base version. This would make future updates easier. For example:
{% set r_base = '4.1' %} # Adjust this to the desired R version requirements: host: - r-base {{ r_base }} run: - r-base {{ r_base }}
86-89: Test commands are correct, but could be more comprehensive.The test section correctly specifies commands for both Windows and non-Windows platforms to check if the ArchR library can be loaded.
Consider adding more comprehensive tests to ensure the package's functionality. For example:
test: commands: - $R -e "library('ArchR'); stopifnot(packageVersion('ArchR') == '{{ version }}')" # [not win] - "\"%R%\" -e \"library('ArchR'); stopifnot(packageVersion('ArchR') == '{{ version }}')\"" # [win] - $R -e "ArchR:::.testArchRFunctionality()" # [not win] - "\"%R%\" -e \"ArchR:::.testArchRFunctionality()\"" # [win]This assumes that ArchR has an internal function
.testArchRFunctionality()for testing. If it doesn't, you might want to suggest adding such a function to the package.
102-124: Extra section and additional comments look good.The extra section correctly specifies linting instructions and recipe maintainers. The additional comments provide valuable information about the original R package.
Consider moving the additional comments (lines 109-124) to the top of the file, just after the initial Jinja2 variable definitions. This would make it easier for maintainers to find this information. You could also consider adding a comment explaining why the build number reset is being skipped, for future reference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- recipes/r-archr/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/r-archr/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/r-archr/meta.yaml (4)
1-8: LGTM: Package and version information looks correct.The package name and version are correctly defined. The use of Jinja2 templating for the version and the replace filter for handling potential hyphens in the version string are good practices.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
14-21: Build configuration looks good, but clarification needed on build number.The build section is well-configured with appropriate settings for Windows, correct RPaths, and a good use of run_exports for dependency management.
Could you please clarify why the build number is set to 8? For a new package, we typically start with build number 0. If there's a specific reason for this higher number, consider adding a comment explaining it.
91-100: LGTM: About section is well-defined and informative.The about section provides all necessary information, including correct URLs, license details, and a concise summary of the package's purpose. The use of environment variables for license files is a good practice.
1-1: Note on static analysis warningThe yamllint tool reports a syntax error for the '%' character at the beginning of the file. This is a false positive, as the '%' is part of the valid Jinja2 templating syntax used in Conda recipes.
To suppress this warning in future linting, you could add the following to your
.yamllintconfiguration file:rules: document-start: disableThis disables the rule that requires YAML files to start with
---, which is causing the false positive in this case.🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
|
Thanks for the feedback @martin-g! I will touch those up when I make other changes. I added some more thoughts to OP #51295 (comment), RE: versioning and upper bounding dependencies. I'm open to comments on that if you have time. |
Yes, I agree to be a recipe maintainer
I really don't like the idea of packaging the master or dev branches directly. How critical are these bug fixes? Could we create a bioconda for the latest GitHub release and then open an Issue to request another release? The changes to master since the last release are minimal. Instead of pulling from a specific commit, we could apply a single patch. GreenleafLab/ArchR@v1.0.2...master There are tons of changes on the dev branch. But who is using the dev branch?? The README in both the master and dev branches only instructs to install from master. And I'm pretty sure the pkgdown site is built from master. GreenleafLab/ArchR@v1.0.2...dev Overall this package illustrates everything that is wrong with scientific software. It has established itself as critical for advancing research, yet it is being maintained as a side project by an overworked PI. We need more funding for Research Software Engineers! Fingers crossed that they can get funding to hire a maintainer. |
This is partially what I hope we could address by generating stable builds on Bioconda. Users I know not only use the
The alternative approach suggested would be building the stable Release and patching. My worry with that is it leads to maintainers here doing guesswork as to how things upstream should be resolved - a position I'm not eager to assume. Nevertheless, I'm open to that as an initial recipe. I.e., build from Release with a one patch for the threads fix. But I would also want to continue discussion (separate Issue) about variant builds for branches that have active use. I've posted an upstream request for Releases. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
recipes/r-archr/patches/0001-cap-threads.patch (1)
1-73: Suggestion: Squash commits for clarityThe patch file contains two commits that modify the same lines of code, with the second commit correcting an issue introduced in the first. While the final state is correct, having these as separate commits might be confusing for future reviewers.
Consider squashing these two commits into a single commit that implements the correct thread capping logic. This would make the change history clearer and easier to understand.
recipes/r-archr/meta.yaml (3)
23-79: LGTM for the requirements section. Consider using Jinja2 templating for shared dependencies.The requirements are well-structured with appropriate build, host, and run sections. Pinning the R base version to 4.1 ensures consistency. The comprehensive list of dependencies aligns with the package's needs.
As an optimization, since the host and run requirements are identical, you could consider using Jinja2 templating to define these dependencies once and reuse them. This would make maintenance easier and reduce the risk of inconsistencies. For example:
{% set deps = [ "r-base =4.1", "bioconductor-biocgenerics", # ... other dependencies ... ] %} requirements: build: - cross-r-base {{ r_base }} # [build_platform != target_platform] - {{ compiler('c') }} - {{ compiler('cxx') }} host: {% for dep in deps %} - {{ dep }} {% endfor %} run: {% for dep in deps %} - {{ dep }} {% endfor %}This approach would make it easier to manage dependencies in the future.
81-83: LGTM for the test section. Consider adding more comprehensive tests.The current test command ensures that the ArchR library can be loaded in R, which is a good basic check. However, to increase confidence in the package's functionality, consider adding more comprehensive tests. For example:
- Test importing key functions or classes from the package.
- Run a simple analysis workflow using example data (if available).
- Check if specific expected outputs or objects can be created.
These additional tests would help catch potential issues early and ensure the package is working as expected in the Conda environment.
101-116: Consider moving package metadata to a separate file.The commented-out package information at the end of the file provides valuable context about the original R package. However, to improve readability and maintainability of the Conda recipe, consider moving this information to a separate file (e.g.,
original_description.txt) in the recipe directory. This would keep themeta.yamlfile focused on Conda-specific configurations while still preserving the original package metadata for reference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/r-archr/meta.yaml (1 hunks)
- recipes/r-archr/patches/0001-cap-threads.patch (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/r-archr/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (6)
recipes/r-archr/patches/0001-cap-threads.patch (1)
56-57: Approved: Correct implementation of thread cappingThis change properly implements thread capping by using
mininstead ofmax. It ensures that the number of threads used will not exceed either the number of input files or the specifiedthreadsvalue, addressing the potential issue from the previous commit.The implementation now correctly balances resource utilization with the available inputs, which is a best practice for parallel processing.
Also applies to: 69-70
recipes/r-archr/meta.yaml (5)
1-14: LGTM for package and source sections. Please clarify the patch file.The package name and version are correctly defined, and the source URL points to the appropriate GitHub tag. However, could you please provide more information about the purpose of the patch file
0001-cap-threads.patch? It would be helpful to understand what modifications are being made to the original source.🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
15-21: LGTM for the build section.The build configuration looks good. The build number is correctly set to 0 for a new package, and the RPaths are appropriately defined for R libraries. The inclusion of the
run_exportssection with proper pinning is a good practice to ensure version consistency for dependent packages.
85-94: LGTM for the about section.The about section is well-structured and contains all the necessary metadata. The home, dev_url, and doc_url provide users with easy access to important resources. The license information is correctly specified as GPL-2.0-or-later, and the appropriate license files are included. The summary, although brief, effectively describes the package's purpose.
96-99: LGTM for the extra section. Verify maintainer consent.The extra section correctly lists two recipe maintainers: jdblischak and mfansler. This is in line with the information provided in the PR objectives. However, it's important to ensure that both maintainers have given their consent to be listed.
Could you please confirm that both jdblischak and mfansler have agreed to be listed as maintainers for this recipe?
1-116: Overall, the recipe looks well-structured and ready for inclusion.The
meta.yamlfile for the r-archr package is comprehensive and follows Conda packaging best practices. It correctly defines the package metadata, source information, build configurations, and dependencies. The suggestions provided in the review comments are minor optimizations that could improve maintainability but are not critical for the functionality of the package.To summarize the main points:
- Clarify the purpose of the patch file.
- Consider using Jinja2 templating for shared dependencies.
- Add more comprehensive tests if possible.
- Verify maintainer consent.
- Consider moving the commented package metadata to a separate file.
Once these points are addressed, the recipe should be ready for inclusion in the Bioconda repository.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
Docker image(s) built:
|
There was a problem hiding this comment.
LGTM, thanks for your input @jdblischak and @mfansler for doing the heavy lifting! Feel free merge if the binaries are working as intended.
The
ArchRpackage is routinely used in the community without a canonical Conda version. This proposes to establish one in Bioconda. The recipe is mostly derived from #36855.Versioning Considerations
However, it appears the
devbranch has been solving more substantial issues, also without version releases. As an alternative to custom build strings, perhaps we should consider customizing the version string? For example, something like1.0.2_main20240226and1.0.3_dev20230825, i.e., a_{branch}{YYYYMMDD}suffix.Initially we can commit to the Release v1.0.2 plus a single patch from
master. Further discussion about variants can be postponed.Dependency Compatibility
As is frequently posted in upstream Issue responses, the release version is stable on R 4.1/Bioc 3.15. For now we propose building only for R 4.1.
CC: @jdblischak okay if I list you as a maintainer here? Any thoughts about this also welcome!
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>.