-
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
Update yacht to 1.3.0 #51918
Update yacht to 1.3.0 #51918
Conversation
📝 WalkthroughWalkthroughThis pull request involves an update to the Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (1)
recipes/yacht/meta.yaml (1)
Line range hint
55-57
: Consider enhancing test coverageThe test section only verifies that the CLI is accessible. Consider adding more comprehensive tests.
Suggested additional test commands:
test: commands: - yacht --help - yacht --version - python -c "import yacht; print(yacht.__version__)"🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/yacht/meta.yaml
(2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/yacht/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/yacht/meta.yaml (5)
1-1
: Version update looks good!
Version bump from 1.2.3 to 1.3.0 matches the PR objective.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
9-9
: Verify SHA256 checksum
Let's verify that the SHA256 checksum matches the source tarball.
#!/bin/bash
# Download the tarball and verify its checksum
wget -q https://github.com/KoslickiLab/YACHT/releases/download/v1.3.0/yacht-1.3.0.tar.gz -O yacht.tar.gz
echo "04a1fabffb6767619f84e08f74126ba2be02d146169bd76980ee80ffe3cb0039 yacht.tar.gz" | sha256sum -c -
19-22
: New build requirements added
Addition of C/C++ compilers and make suggests native code compilation. This is a significant change that could affect build reproducibility.
#!/bin/bash
# Check for C/C++ source files in the package
rg -t cpp -t c -l . || echo "No C/C++ files found"
Consider documenting why these compiler requirements were added and what components require native compilation.
37-38
:
Review dependency version constraints
The Python requirement has been simplified and sourmash version constraints have been removed. This could potentially lead to compatibility issues.
#!/bin/bash
# Check for sourmash usage patterns that might depend on specific versions
rg -A 3 "from sourmash|import sourmash"
Consider maintaining version constraints for sourmash to ensure compatibility. The previous constraints (>=4.8.3,<5
) provided better guarantees against breaking changes.
13-13
: Verify Python version constraints
The build is now skipped for Python versions < 3.7 or > 3.11. This is more restrictive than before.
Consider documenting the reason for the Python version constraints in a comment, especially if there are specific dependencies or features requiring this range.
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/yacht/meta.yaml
(2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/yacht/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/yacht/meta.yaml (4)
1-1
: Version update and checksum look correct.
The version bump to 1.3.0 and corresponding SHA256 checksum update are properly implemented.
Also applies to: 9-9
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
39-39
: Verify sourmash version compatibility.
The version constraints for sourmash have been removed. This could potentially lead to compatibility issues if there are breaking changes in sourmash.
#!/bin/bash
# Check upstream sourmash dependency specifications
curl -s https://raw.githubusercontent.com/KoslickiLab/YACHT/v1.3.0/pyproject.toml | grep -A 5 'dependencies'
19-22
: Verify new compiler requirements.
The addition of C/C++ compilers and make suggests new compiled components. Let's verify this change against upstream.
#!/bin/bash
# Check for C/C++ source files and Makefile in the upstream repository
echo "Checking for C/C++ files and Makefile in v1.3.0..."
curl -s https://api.github.com/repos/KoslickiLab/YACHT/git/trees/v1.3.0?recursive=1 | \
jq -r '.tree[].path' | \
grep -E '\.(cpp|cc|cxx|c|h|hpp)$|Makefile'
13-13
: Verify Python version constraints with upstream.
The build now explicitly skips Python versions below 3.7 and above 3.11. Please verify if:
- The upper bound of Python 3.11 aligns with upstream compatibility
- There are specific reasons to exclude Python 3.12+ support
✅ Verification successful
Let me try another approach to verify the Python version requirements.
Let me try one more approach to find Python version requirements.
Python version constraint appears reasonable
The upper bound of Python 3.11 in the build configuration is a conservative choice that:
- Ensures compatibility with stable Python releases
- Allows for proper testing before supporting newer Python versions
- Follows common packaging practices of incrementally validating support for new Python versions
The lower bound of Python 3.7 is also appropriate as it's an older version that has reached end-of-life.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check upstream Python version requirements
curl -s https://raw.githubusercontent.com/KoslickiLab/YACHT/v1.3.0/pyproject.toml | grep -A 5 'requires-python'
Length of output: 330
Script:
#!/bin/bash
# Check setup.py or other configuration files for Python version requirements
fd "setup.py|setup.cfg|pyproject.toml" --type f --exec cat {}
Length of output: 42700
Script:
#!/bin/bash
# Check README or other documentation files for Python version info
rg -i "python|requires" README.md
Length of output: 35
host: | ||
- python >3.6 | ||
- python |
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.
Fix inconsistent Python version constraints.
The host requirement python
lacks version constraints, which is inconsistent with the build skip conditions (py < 37 or py > 311
). This could lead to installation issues.
Apply this change to align with build constraints:
- - python
+ - python >=3.7,<3.12
📝 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.
- python | |
- python >=3.7,<3.12 |
- python >3.6 | ||
- sourmash >=4.8.3,<5 | ||
- rust | ||
- python |
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.
Fix inconsistent Python version constraints in run requirements.
Similar to host requirements, the run requirement for Python should be constrained to match build skip conditions.
Apply this change:
- - python
+ - python >=3.7,<3.12
📝 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.
- python | |
- python >=3.7,<3.12 |
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/yacht/meta.yaml
(2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/yacht/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/yacht/meta.yaml (5)
11-16
: LGTM! Build configuration is appropriate.
The build configuration is well-structured with:
- Correct build number for a new version
- Appropriate Python version constraints
- Proper pip installation flags
19-22
: LGTM! Build requirements are appropriate.
The addition of C/C++ compilers and make is appropriate for packages with native code compilation.
24-24
: Fix inconsistent Python version constraints.
The Python requirements in host and run sections should match the build skip conditions.
Also applies to: 38-38
Line range hint 56-77
: LGTM! Test and metadata sections are well-defined.
The test command, license information, and maintainer details are properly configured.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
1-9
: Verify source tarball integrity.
The version update and URL structure look correct. Let's verify the SHA256 checksum matches the source tarball.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
- sourmash >=4.8.3,<5 | ||
- rust | ||
- python | ||
- sourmash |
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.
Restore version constraints for sourmash dependency.
Removing version constraints from sourmash could lead to compatibility issues. Consider maintaining version constraints to ensure compatibility.
Apply this change:
- - sourmash
+ - sourmash >=4.8.3,<5
📝 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.
- sourmash | |
- sourmash >=4.8.3,<5 |
Closing in favor of #51947. |
Update
yacht
: 1.2.3 → 1.3.0recipes/yacht
(click to view/edit other files)@KoslickiLab
This pull request was automatically generated (see docs).