Update hictk to v2.0.0 #50881
Update hictk to v2.0.0 #50881
Conversation
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
Docker image(s) built:
|
|
@BiocondaBot please add label Supersedes #51500. |
📝 WalkthroughWalkthroughThe pull request introduces several modifications to enhance the functionality of the Possibly related PRs
Suggested labels
Suggested reviewers
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 (6)
recipes/hictk/conan_profiles/clang (1)
1-8: LGTM with suggestions for improved compatibility and portability.The Conan profile configuration for Clang looks good overall and is well-structured. It sets up a modern C++17 build environment for x86_64 Linux systems using Clang. However, there are a couple of points to consider:
Compiler version: The specified Clang version (18) is very recent. This might cause compatibility issues on systems with older Clang versions. Consider using a more widely available version or providing fallback options.
Portability: The configuration is specific to Linux and x86_64 architecture. If cross-platform compatibility is a goal, you might want to create additional profiles for other operating systems and architectures.
To improve compatibility and portability, consider the following suggestions:
- Use a more conservative Clang version or make it configurable:
-compiler.version=18 +compiler.version=${CLANG_VERSION:-10}
- Create separate profiles for different operating systems and architectures, or use variables:
-arch=x86_64 -os=Linux +arch=${CONAN_ARCH:-x86_64} +os=${CONAN_OS:-Linux}These changes would make the profile more flexible and easier to adapt to different environments.
recipes/hictk/run_test.sh (5)
8-15: Excellent addition of command checks and dependency display!The new code segment effectively verifies the existence of the
hictkcommand and displays its shared library dependencies. This is crucial for debugging and ensuring proper installation.A minor suggestion for improvement:
Consider adding error handling for the
which hictkcommand. Ifhictkis not found, the script will continue, potentially causing issues later. You could add:if ! which hictk &>/dev/null; then echo "Error: hictk command not found" exit 1 fiThis will ensure the script exits early if
hictkis not available.
22-26: Good update to use .zst format and appropriate extraction method!The change to use a .zst file format and the corresponding update to the extraction method using
zstdcatis a good improvement. It maintains data integrity checks while potentially improving decompression speed.A suggestion for improvement:
Consider adding error handling for the curl download and zstdcat extraction. For example:
if ! curl -L "$url" -o hictk_test_dataset.tar.zst; then echo "Error: Failed to download test dataset" exit 1 fi if ! zstdcat hictk_test_dataset.tar.zst | tar -xf -; then echo "Error: Failed to extract test dataset" exit 1 fiThis will help catch and report any issues during the download or extraction process.
28-29: Good practice to use pip for installing the test suite!Using pip to install the test suite from a local directory is a clean and standard approach. This ensures that the correct version of the test suite is always used.
A suggestion for improvement:
Consider pinning the version of the test suite or using a requirements file to ensure reproducibility. For example:
pip install test/integration==1.0.0 # Replace with actual versionOr create a
requirements.txtfile in thetest/integrationdirectory and install using:pip install -r test/integration/requirements.txtThis will help maintain consistency across different environments and prevent potential issues from unexpected updates.
32-38: Excellent update to the integration test command!The new
hictk_integration_suitecommand is more comprehensive and flexible. The use of${CPU_COUNT}for threading and the--do-not-copy-binaryflag are particularly good optimizations.A suggestion for improvement:
Consider adding error handling for the integration test command. For example:
if ! hictk_integration_suite \ "$(which hictk)" \ test/integration/config.toml \ --data-dir test/data \ --do-not-copy-binary \ --threads "${CPU_COUNT}" \ --result-file results.json; then echo "Error: Integration tests failed" exit 1 fiThis will help catch and report any issues during the integration test execution.
40-41: Good addition of result output!The separator and the display of the results.json contents provide clear and immediate feedback on the test execution.
A suggestion for improvement:
Consider adding a simple analysis of the results. For example:
echo "Test Results Summary:" jq '.summary' results.json echo "Failed Tests (if any):" jq '.failed_tests[]?' results.jsonThis assumes that results.json has a structure with 'summary' and 'failed_tests' fields. Adjust according to the actual structure of your results file. This will provide a quick overview of the test results, making it easier to spot any issues at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- recipes/hictk/build.sh (4 hunks)
- recipes/hictk/conan_profiles/clang (1 hunks)
- recipes/hictk/conanfile.Dockerfile.py.patch (1 hunks)
- recipes/hictk/conanfile.py.patch (0 hunks)
- recipes/hictk/conda_build_config.yaml (1 hunks)
- recipes/hictk/meta.yaml (3 hunks)
- recipes/hictk/run_test.sh (1 hunks)
💤 Files with no reviewable changes (1)
- recipes/hictk/conanfile.py.patch
🧰 Additional context used
🔇 Additional comments (18)
recipes/hictk/conda_build_config.yaml (1)
1-8: LGTM! Compiler configuration looks good.The compiler configuration for Linux is well-defined and uses up-to-date versions of clang/clangxx (version 18). This is consistent with modern C/C++ development practices and should provide good support for recent language features and optimizations.
To ensure this change aligns with the project's overall configuration and doesn't introduce conflicts, please run the following verification:
This script will help identify any potential conflicts or inconsistencies with other parts of the project.
recipes/hictk/run_test.sh (1)
Line range hint
1-41: Overall excellent improvements to the test script!The changes to this script significantly enhance its functionality, robustness, and output clarity. Key improvements include:
- Better verification of the
hictkcommand and its dependencies.- Updated file format (.zst) for potentially faster decompression.
- More comprehensive integration testing with optimized resource usage.
- Clearer output of test results.
These changes align well with the update to hictk v2.0.0 mentioned in the PR title and contribute to a more reliable testing process.
The script is approved for merging, subject to addressing the minor suggestions provided in the previous comments.
recipes/hictk/meta.yaml (7)
10-10: Build number reset is correct.Resetting the build number to 0 for a new version release is the proper practice.
44-46: Run dependencies correctly updated to match host dependencies.The run dependencies have been properly updated to match the host dependencies, ensuring correct runtime behavior.
59-60: Test requirements update looks good. Verify the test suite.The addition of Python, pip, and zstd as test requirements suggests new Python-based tests and compression functionality testing. This is a positive change that likely improves the test coverage.
Please confirm that the test suite has been updated to use these new requirements:
#!/bin/bash # Check for Python usage in the test scripts grep -R 'python' recipes/hictk/test* # Check for zstd usage in the test scripts grep -R 'zstd' recipes/hictk/test*Also applies to: 63-63
66-66: Test source files update simplifies the recipe. Verify directory contents.Updating the test source files to include the entire test/integration/ directory simplifies the recipe and makes it easier to maintain as new test files are added.
Please confirm that all necessary test files are included in the test/integration/ directory:
#!/bin/bash # List contents of the test/integration directory ls -R test/integration/
55-55: Documentation URL update looks good. Verify the link.The change to a ReadTheDocs URL suggests improved and more comprehensive documentation, which is beneficial for users.
Please confirm that the new documentation URL is active and contains up-to-date information:
37-39: Host dependency updates look good. Verify impact on build process.The addition of libarchive >=3 and the update to libdeflate >=1 suggest changes in the package's compression or archiving capabilities. These changes may affect the build process.
Please confirm that these new host dependencies are correctly linked during the build process:
2-3: Version update looks good. Verify the SHA256 checksum.The version bump from 1.0.0 to 2.0.0 indicates a major release. Ensure that all breaking changes are documented and that downstream dependencies are updated accordingly.
Please confirm that the SHA256 checksum is correct for the new version:
recipes/hictk/build.sh (7)
5-5: Set parallel build levels usingCPU_COUNTvariableThe build now utilizes the available CPU cores by setting
CMAKE_BUILD_PARALLEL_LEVELandCTEST_PARALLEL_LEVELto${CPU_COUNT}, improving build efficiency.
22-23: AddCXXFLAGSto support newer C++ features on macOSAdding
-D_LIBCPP_DISABLE_AVAILABILITYtoCXXFLAGSenables the use of newer C++ features with older SDKs on macOS, which is appropriate in this context.
35-35: Confirm compatibility with Clang compiler on LinuxSetting
conan_profile='clang'for non-Darwin hosts changes the default compiler from GCC to Clang on Linux. Since GCC is typically the default compiler on Linux in conda environments, please verify that using Clang is intentional and ensure that Clang is available and compatible in the build environment.
42-43: Update patch target toconanfile.Dockerfile.pyThe script now patches
conanfile.Dockerfile.pyinstead ofconanfile.py, aligning with the changes in the dependency management approach. Please confirm that the patch fileconanfile.Dockerfile.py.patchcorresponds to the new target.
46-46: Updateconan installto useconanfile.Dockerfile.pyThe
conan installcommand correctly referencesconanfile.Dockerfile.py, consistent with the updated patch target and new dependency management.
63-63: Disable fuzzy testing in CMake configurationAdding
-DHICTK_ENABLE_FUZZY_TESTING=OFFdisables fuzzy testing during the build, which may reduce build time or resolve compatibility issues. Ensure that disabling fuzzy testing does not omit important tests necessary for verifying the software's correctness.
80-80: Increasectesttimeout to 180 secondsExtending the test timeout from
100to180seconds allows longer tests to complete without timing out, improving test reliability on slower systems or with more extensive test suites.recipes/hictk/conanfile.Dockerfile.py.patch (2)
8-29: Verify that removed dependencies are no longer requiredThe removal of multiple dependencies from the
requirementsmethod simplifies the dependency list. Please ensure that these dependencies are not used elsewhere in the codebase to prevent potential build or runtime errors.You can run the following script to search for references to the removed dependencies:
#!/bin/bash # Description: Search the codebase for references to the removed dependencies. dependencies=( "bzip2" "cli11" "fast_float" "fmt" "hdf5" "highfive" "libarchive" "libdeflate" "lz4" "lzo" "nlohmann_json" "spdlog" "tomlplusplus" "xz_utils" "zstd" "zlib" ) for dep in "${dependencies[@]}"; do echo "Searching for references to $dep..." rg "$dep" || true done
33-66: Ensure default configurations meet project requirementsBy removing custom configuration options in the
configuremethod, the project now relies on the default settings of the dependencies. Verify that the default configurations align with the project's requirements and that no necessary functionality is lost.You can inspect the default options of the affected dependencies using the following script:
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>.