add meta #51349
add meta #51349
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several significant changes across multiple files related to the packaging and installation of a software package named "meta." A new shell script, Additionally, 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
recipes/meta/conda_build_config.yaml (1)
9-9: Add a newline at the end of the file.To adhere to best practices and resolve the static analysis warning, please add a newline character at the end of the file.
Apply this change:
python: - 3.8 - 3.9 - 3.10 - - 3.11 + - 3.11 +🧰 Tools
🪛 yamllint
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
recipes/meta/build.sh (3)
7-11: Directory setup looks good, with a minor suggestion.The directory structure creation is well-organized. However, there's a small improvement we can make:
Consider adding error handling to the
cdcommand on line 11:-cd $SRC_DIR/build +cd $SRC_DIR/build || exit 1This change ensures the script exits if the directory change fails, preventing potential issues in subsequent steps.
🧰 Tools
🪛 Shellcheck
[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
22-24: Build and install steps look good, with a suggestion for improvement.The build, install, and cleanup process is concise and effective. However, we can enhance it with error checking:
Consider adding error checking to the build and install steps:
-cmake --build . --config Release -cmake --install . +cmake --build . --config Release || exit 1 +cmake --install . || exit 1This change ensures the script exits if either the build or install step fails, preventing potential issues with incomplete or failed installations.
26-28: MeTA package installation looks good, with a minor suggestion.The package installation step is well-structured, using the correct Python interpreter and appropriate pip options. However, there's a small improvement we can make:
Consider adding error handling to the
cdcommand on line 27:-cd ${SRC_DIR} +cd ${SRC_DIR} || exit 1This change ensures the script exits if the directory change fails, preventing potential issues with the pip installation step.
Additionally, it might be beneficial to add error checking to the pip install command:
-${PYTHON} -m pip install . --no-deps -vv +${PYTHON} -m pip install . --no-deps -vv || exit 1This ensures the script exits if the pip installation fails.
🧰 Tools
🪛 Shellcheck
[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/meta/meta.yaml (4)
13-15: Document reason for skipping Windows buildsThe build configuration looks good. However, it would be helpful to document the reason for skipping Windows builds. Consider adding a comment explaining why the package is not compatible with Windows, if that's the case.
build: number: 0 - skip: true # [win] + skip: true # [win] # Package is not compatible with Windows due to [reason]
81-92: LGTM with a minor suggestionThe about section is well-structured and informative. It provides clear information about the package's purpose and licensing.
As a minor suggestion, consider adding a
doc_urlfield pointing to any available documentation, if it exists. This can help users find more detailed information about using the package.about: home: https://github.com/bagari/meta license: BSD-3-Clause license_file: LICENSE.txt summary: 'Medial Tractography Analysis (MeTA)' description: | MeTA is a workflow implemented to minimize microstructural heterogeneity in diffusion MRI (dMRI) metrics by extracting and parcellating the core volume along the bundle length in the voxel-space directly while effectively preserving bundle shape and efficiently capturing the regional variation within and along white matter (WM) bundles. dev_url: https://github.com/bagari/meta + doc_url: https://github.com/bagari/meta/wiki # If documentation exists🧰 Tools
🪛 yamllint
[error] 88-88: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 90-90: trailing spaces
(trailing-spaces)
94-96: Consider adding additional maintainersThe extra section correctly lists a recipe maintainer, which is crucial for package management. However, to improve the bus factor and ensure long-term maintenance:
- Consider adding additional maintainers if there are other contributors to the project who could help maintain the package.
- If this is part of a larger project or organization, you might want to add the organization as a maintainer as well.
extra: recipe-maintainers: - bagari + # Consider adding additional maintainers here + # - organization-name
87-91: Remove trailing spacesThere are trailing spaces at the end of lines in the description. While these don't affect functionality, removing them improves code cleanliness.
- MeTA is a workflow implemented to minimize microstructural heterogeneity - in diffusion MRI (dMRI) metrics by extracting and parcellating the core - volume along the bundle length in the voxel-space directly while effectively - preserving bundle shape and efficiently capturing the regional variation - within and along white matter (WM) bundles. + MeTA is a workflow implemented to minimize microstructural heterogeneity + in diffusion MRI (dMRI) metrics by extracting and parcellating the core + volume along the bundle length in the voxel-space directly while effectively + preserving bundle shape and efficiently capturing the regional variation + within and along white matter (WM) bundles.🧰 Tools
🪛 yamllint
[error] 88-88: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 90-90: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/meta/build.sh (1 hunks)
- recipes/meta/conda_build_config.yaml (1 hunks)
- recipes/meta/meta.yaml (1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/meta/build.sh
[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🪛 yamllint
recipes/meta/conda_build_config.yaml
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
recipes/meta/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
[error] 88-88: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 90-90: trailing spaces
(trailing-spaces)
🔇 Additional comments (5)
recipes/meta/conda_build_config.yaml (1)
1-9: LGTM! The conda build configuration looks good.The
build_variantsection correctly specifies different build options for Linux (osmesa) and macOS (qt). Thepythonsection includes all the required versions (3.8 to 3.11) as mentioned in the PR objectives. This configuration will allow for flexible building across different operating systems and Python versions.🧰 Tools
🪛 yamllint
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
recipes/meta/build.sh (3)
1-6: LGTM: Environment setup looks good.The initial environment setup is well-structured. Unsetting potentially conflicting variables and setting up CMAKE_INSTALL_RPATH and CMAKE_BUILD_WITH_INSTALL_RPATH are good practices for ensuring a clean and portable build.
13-20: CMake configuration is well-structured and comprehensive.The CMake configuration covers all necessary aspects:
- Correct build type (Release) for a package
- Proper installation prefix
- C++ standard specification
- Appropriate module and prefix paths
- Consistent use of RPATH settings
This configuration should result in a well-organized and portable build.
1-28: Overall, the build script is well-structured and follows good practices.The script effectively sets up the environment, configures CMake, builds the project, and installs the package. It's designed to work within a conda build system and includes necessary steps for a clean and portable build.
A few minor suggestions have been made to improve error handling, which would make the script more robust. These include adding checks after directory changes and critical operations like building and installing.
Great job on creating a comprehensive build script for the MeTA package!
🧰 Tools
🪛 Shellcheck
[warning] 11-11: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/meta/meta.yaml (1)
17-70: 🛠️ Refactor suggestionConsider optimizing dependency specifications and clarify build variants
The requirements section is comprehensive, which is good for reproducibility. However, consider the following suggestions:
- Some dependencies are repeated across build, host, and run sections. Consider using the
{{ pin_compatible() }}function to reduce repetition and ensure consistency. For example:host: - boost {{ boost }} run: - {{ pin_compatible('boost', max_pin='x.x') }}
The use of
{{ build_variant }}suggests multiple build configurations. Please clarify what these variants are and consider documenting them in a comment.The
libexpatdependency is specified twice with different version constraints. This might lead to conflicts. Consider consolidating these into a single, more specific constraint.✅ Verification successful
Build variant is correctly defined
The
build_variantis properly defined inconda_build_config.yaml, ensuring that the placeholders inmeta.yamlare correctly resolved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if build_variant is defined elsewhere in the recipe rg "build_variant:" conda_build_config.yaml || echo "build_variant not found in conda_build_config.yaml"Length of output: 218
Script:
#!/bin/bash # Search for all occurrences of 'build_variant' in the codebase rg "build_variant"Length of output: 571
Script:
#!/bin/bash # Display the contents of 'conda_build_config.yaml' to verify 'build_variant' definition cat recipes/meta/conda_build_config.yamlLength of output: 146
recipes/meta/meta.yaml
Outdated
| test: | ||
| imports: | ||
| - meta | ||
| commands: | ||
| - set -x | ||
| - meta --help | ||
| - cmrep_vskel -h || true | ||
| - vtklevelset -h || true | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness and clarity
The test section includes both import and command-line tests, which is good. However, consider the following improvements:
-
Remove
set -xunless debugging is specifically needed, as it can clutter test output. -
The use of
|| trueforcmrep_vskelandvtklevelsetcommands might hide actual failures. Instead, consider checking for specific expected output or return codes. For example:
- cmrep_vskel -h 2>&1 | grep -q "Usage:" || exit 1
- vtklevelset -h 2>&1 | grep -q "Usage:" || exit 1- Add more comprehensive import tests to ensure all main modules are importable.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
recipes/meta/meta.yaml (1)
90-92: Consider adding more maintainersCurrently, there's only one recipe maintainer listed. While this is acceptable, it's generally a good practice to have multiple maintainers to ensure continued support and maintenance of the package.
Consider reaching out to other contributors or team members who are familiar with this package and adding them as co-maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/meta/build.sh (1 hunks)
- recipes/meta/conda_build_config.yaml (1 hunks)
- recipes/meta/meta.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- recipes/meta/build.sh
- recipes/meta/conda_build_config.yaml
🧰 Additional context used
🪛 yamllint
recipes/meta/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (3)
recipes/meta/meta.yaml (3)
13-15: Build configuration looks goodThe build number is correctly set to 0 for a new package, and the Windows build is skipped with a clear explanation in the comment.
80-88: About section is well-documentedThe about section provides comprehensive information about the package, including its purpose, license, and relevant URLs. This is excellent for user understanding and package discoverability.
1-1: Ignore yamllint error for Jinja2 syntaxThe 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 Jinja2 templating syntax commonly used in conda recipes.
No action is needed. The syntax is correct for a conda recipe.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
| requirements: | ||
| build: | ||
| - {{ compiler('cxx') }} | ||
| - {{ stdlib("c") }} | ||
| - cmake | ||
| - make # [not win] | ||
| - python {{ python }} | ||
| - pip | ||
| - git | ||
| - setuptools | ||
| - boost =1.82 | ||
| - itk ==5.3.0 | ||
| - libitk ==5.3.0 | ||
| - libitk-devel ==5.3.0 | ||
| - hdf5 >=1.14.2,<1.14.3.0a0 | ||
| - vtk-base >=9.2.6,<9.3.0 build=*{{ build_variant }}* | ||
| - libexpat <2.6 | ||
| - libexpat >=2.5.0,<3.0a0 | ||
| - vtk-io-ffmpeg >=9.2.6,<9.3.0 build=*{{ build_variant }}* | ||
|
|
||
| host: | ||
| - qhull | ||
| - boost =1.82 | ||
| - itk ==5.3.0 | ||
| - libitk ==5.3.0 | ||
| - libitk-devel ==5.3.0 | ||
| - hdf5 >=1.14.2,<1.14.3.0a0 | ||
| - vtk-base >=9.2.6,<9.3.0 build=*{{ build_variant }}* | ||
| - python {{ python }} | ||
| - libexpat <2.6 | ||
| - libexpat >=2.5.0,<3.0a0 | ||
| - vtk-io-ffmpeg >=9.2.6,<9.3.0 build=*{{ build_variant }}* | ||
|
|
||
| run: | ||
| - qhull | ||
| - boost =1.82 | ||
| - itk ==5.3.0 | ||
| - libitk ==5.3.0 | ||
| - libitk-devel ==5.3.0 | ||
| - hdf5 >=1.14.2,<1.14.3.0a0 | ||
| - vtk-base >=9.2.6,<9.3.0 build=*{{ build_variant }}* | ||
| - python {{ python }} | ||
| - numpy | ||
| - setuptools | ||
| - nibabel | ||
| - pandas | ||
| - pyvista | ||
| - scipy | ||
| - tqdm | ||
| - tslearn | ||
| - dipy | ||
| - libexpat <2.6 | ||
| - libexpat >=2.5.0,<3.0a0 | ||
| - vtk-io-ffmpeg >=9.2.6,<9.3.0 build=*{{ build_variant }}* |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing dependency management
The requirements section is comprehensive, but there might be room for optimization:
-
Some dependencies (e.g., boost, itk, hdf5, vtk-base) are repeated across build, host, and run sections with the same version constraints. Consider using conda-forge's
run_exportsfeature to reduce redundancy. -
The
libexpatdependency has two entries with overlapping version ranges. These could potentially be combined into a single entry. -
Ensure that all listed dependencies are necessary for each stage (build, host, run). For example, does the package really need
gitat build time?
Review each dependency and its presence in different sections. If you need assistance in optimizing this section, please let me know.
| test: | ||
| imports: | ||
| - meta | ||
| commands: | ||
| - meta --help | ||
| - cmrep_vskel --version | ||
| - vtklevelset --version |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness
The test section includes both import and command-line tests, which is good. However, consider the following improvements based on past review suggestions:
- For command-line tests, instead of just running with
--helpor--version, check for specific expected output:
commands:
- meta --help 2>&1 | grep -q "Usage:" || exit 1
- cmrep_vskel --version 2>&1 | grep -q "version" || exit 1
- vtklevelset --version 2>&1 | grep -q "version" || exit 1-
Add more comprehensive import tests to ensure all main modules are importable.
-
Consider adding functional tests that exercise core features of the package.
Would you like assistance in implementing these test improvements?
MeTA, which stands for Medial Tractography Analysis, is a workflow designed to minimize brain microstructural heterogeneity in diffusion MRI (dMRI) metrics by extracting and parcellating the core volume along the length of white matter bundles in voxel-space, directly preserving bundle shape while efficiently capturing regional variation within and along the bundles. MeTA is applied to the study of human brain diseases and the genetic architecture of white matter, providing valuable insights into both neurodegenerative conditions and the structural components of brain connectivity.
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>.