-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: transfer project management to Pixi #133
Conversation
…s and updating dependencies
…vel branches, and refresh dependencies in pixi.lock
…rsion__ in __init__.py
WalkthroughThe changes encompass updates to various configuration files and workflows for the project "med-imagetools." Key modifications include the introduction of a new Changes
Poem
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: 6
🧹 Outside diff range and nitpick comments (9)
pyproject.toml (2)
21-36
: Consider relaxing some version constraintsWhile the major version pins are good for stability, some minor version constraints might be too restrictive. For example:
pandas>=2.2.2,<3
- Consider allowing earlier 2.x versions if compatiblescikit-image>=0.23.2,<1
- The upper bound<1
might be too restrictive as the package is approaching 1.0
54-57
: Clarify the relationship between CLI commandsThe presence of both
autopipeline
andbetapipeline
might confuse users. Consider:
- Adding deprecation warnings in the original version
- Documenting the differences and migration path
- Adding version tags to indicate stability
pixi.toml (4)
4-4
: Update placeholder descriptionThe current description appears to be a placeholder. Please replace it with a meaningful description of the project's purpose and functionality.
45-58
: Consider enabling parallel testingThe
--numprocesses=auto
option is commented out. If there's no specific reason to disable parallel testing, enabling it could significantly improve test execution time."pytest", - # "--numprocesses=auto", + "--numprocesses=auto", "-s",
51-53
: Consider simplifying coverage configuration pathThe coverage configuration and report paths could be simplified by using a consistent directory structure:
- "--cov-report=xml:coverage-report/coverage.xml", - "--cov-config=config/coverage.toml", + "--cov-report=xml:.coverage/coverage.xml", + "--cov-config=.coverage/config.toml",
90-90
: Improve semantic version commandThe current command uses echo with a subshell, which could be simplified. Also, consider adding error handling.
- semver = 'echo "Next Version is: $(semantic-release version --print)"' + semver = "semantic-release version --print".github/workflows/main.yml (3)
1-8
: Consider optimizing workflow triggersRunning the workflow on all branches (
"*"
) might consume unnecessary CI resources. Consider limiting the triggers to:
- Main/development branches for pushes
- Pull request branches that could potentially be merged
on: push: - branches: ["*"] + branches: ["main", "devel"] pull_request: - branches: ["*"] + branches: ["main", "devel"]
Line range hint
140-144
: Update PyPI authentication configurationThe current PyPI authentication setup uses both
user
andpassword
. The recommended approach is to use just the token.uses: pypa/gh-action-pypi-publish@release/v1 with: verbose: true - user: ${{ secrets.pypi_token }} - password: ${{ secrets.pypi_api }} + password: ${{ secrets.pypi_token }}
147-167
: Remove commented out job configurationIf the
does-the-pip-squeak
job is no longer needed, it should be removed rather than left as commented code. This helps maintain a cleaner configuration file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
pixi.lock
is excluded by!**/*.lock
poetry.lock
is excluded by!**/*.lock
📒 Files selected for processing (11)
.gitattributes
(1 hunks).github/workflows/main.yml
(5 hunks).gitignore
(1 hunks)config/coverage.toml
(1 hunks)config/ruff.toml
(1 hunks)pixi.toml
(1 hunks)pyproject.toml
(1 hunks)requirements.txt
(0 hunks)ruff.toml
(0 hunks)setup.py
(0 hunks)src/imgtools/__init__.py
(1 hunks)
💤 Files with no reviewable changes (3)
- requirements.txt
- ruff.toml
- setup.py
✅ Files skipped from review due to trivial changes (4)
- .gitattributes
- config/coverage.toml
- config/ruff.toml
- src/imgtools/init.py
🧰 Additional context used
🪛 actionlint
.github/workflows/main.yml
102-102: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (9)
pyproject.toml (3)
59-69
: LGTM! Build system configuration looks good
The Hatch build configuration is well-structured with appropriate include patterns and explicit build directory setting.
71-86
: LGTM! Semantic release configuration is well-structured
The configuration properly handles versioning for both main and development branches, with appropriate prerelease token handling.
1-18
: Verify the license change implications
The change from GPL-3.0 to MIT license is a significant modification that requires careful consideration. This change makes the software more permissive but could have implications for existing users and contributors.
✅ Verification successful
License change from GPL-3.0 to MIT has been properly implemented
The license change has been properly documented and implemented across the codebase:
- LICENSE file contains the complete MIT license text
- CHANGELOG.md documents the license update in PR fix: License updated to MIT #119
- README.md correctly references the MIT license
- No remaining GPL references found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for GPL-3.0 license references and verify license files
# Check for LICENSE file changes
git diff HEAD~1 LICENSE
# Look for any remaining GPL references
rg -i "GPL|GNU" --type txt --type md
# Check if NOTICE or COPYING files exist (common for GPL)
fd -g "NOTICE*" -g "COPYING*"
Length of output: 192
Script:
#!/bin/bash
# Let's try a different approach to verify license-related changes
# Check if LICENSE file exists and its content
if [ -f LICENSE ]; then
echo "=== Current LICENSE content ==="
cat LICENSE
fi
# Check commit history for license-related changes
git log --grep="license\|GPL\|MIT" --pretty=format:"%h %s" -i
# Search for any license declarations in documentation
rg -i "licen[cs]e" --type md
Length of output: 1632
pixi.toml (3)
8-9
: LGTM: Appropriate development setup
The editable installation of the package is correctly configured for development purposes.
11-20
: LGTM: Well-structured environment configuration
The environment setup supports multiple Python versions and has a comprehensive default environment with all necessary features.
#!/bin/bash
# Verify Python version compatibility in the codebase
# Check for version-specific imports or syntax
rg -l "(?i)(python_version|sys\.version_info|typing\.TYPE_CHECKING|__future__)" src/
# Check for potential compatibility issues in dependencies
if [ -f "pyproject.toml" ]; then
grep -A 5 "python_requires" pyproject.toml
fi
6-6
: Consider adding Windows platform support
The platforms list currently only includes Linux and macOS (ARM). Consider adding Windows support (win-64
) if the project is intended to be cross-platform.
.gitignore (1)
219-219
: LGTM!
The blank line separator improves readability by grouping related ignore patterns.
.github/workflows/main.yml (2)
55-60
: Verify coverage artifact paths
Ensure the coverage report paths are consistent between upload in Unit-Tests job and download in Code-Coverage job.
✅ Verification successful
Let me check the workflow file to verify the artifact paths between jobs.
Let me check the complete artifact configuration in both jobs.
Artifact paths are consistent between jobs
The coverage artifact configuration is correctly set up:
- Upload job: Uses name
coverage-report
without specific path (defaults to repository root) - Download job: Uses name
coverage-report
with pathcoverage-report
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for coverage path configurations
if [ -f "pyproject.toml" ]; then
echo "Coverage paths in pyproject.toml:"
cat pyproject.toml | grep -A 5 "\[tool.coverage"
fi
if [ -f "config/coverage.toml" ]; then
echo "Coverage paths in coverage.toml:"
cat config/coverage.toml
fi
Length of output: 347
Script:
#!/bin/bash
# Check the workflow file for coverage artifact paths
echo "Checking coverage artifact paths in workflow:"
rg "coverage.*artifact" .github/workflows/main.yml -B2 -A2
Length of output: 567
Script:
#!/bin/bash
# Get the complete artifact configuration context
echo "Checking complete artifact configurations in workflow:"
rg "Upload coverage|Download coverage" .github/workflows/main.yml -B2 -A5
Length of output: 752
19-20
: Verify Windows compatibility status
Windows support is currently commented out. This might limit the package's cross-platform compatibility.
✅ Verification successful
Windows support is intentionally disabled with known compatibility considerations
The codebase shows evidence of Windows compatibility being a known consideration:
- Windows support was previously included in CI but intentionally removed (commented out with "removed for now" note)
- The codebase includes Windows-specific handling:
- Path character sanitization for Windows in
src/imgtools/io/writers.py
- Windows-specific dependency (colorama) in
pixi.lock
- Path character sanitization for Windows in
- No Windows-specific test skips or platform-specific code blocks that would indicate incompatibility
The temporary removal from CI suggests this is a known state rather than an oversight, likely pending specific Windows-related improvements before re-enabling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any OS-specific code paths or dependencies
# Search for OS-specific imports or conditions
rg -l 'import (os|sys|platform)' || rg -l 'sys\.platform|os\.name'
# Check pixi.toml for OS-specific dependencies
if [ -f "pixi.toml" ]; then
cat pixi.toml | grep -i "platform.*=.*\"windows\""
fi
Length of output: 466
Script:
#!/bin/bash
# Let's check for Windows-specific code paths or potential compatibility issues
# Search for Windows-specific path handling or system calls
rg -i 'win(32|64|dows)' -A 2 -B 2
# Check for path separators that might cause issues on Windows
rg -i '(\\\\|//)' --type py
# Look for any GitHub issues or discussions about Windows support
rg -i 'windows' .github/ || true
# Check test files for any Windows-specific test skips or conditions
rg -i '(skipif|pytest\.mark).*(windows|platform)' tests/
Length of output: 2926
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
==========================================
+ Coverage 45.73% 45.75% +0.01%
==========================================
Files 30 30
Lines 2871 2872 +1
==========================================
+ Hits 1313 1314 +1
Misses 1558 1558 ☔ View full report in Codecov by Sentry. |
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: 3
🧹 Outside diff range and nitpick comments (8)
pyproject.toml (3)
18-18
: Consider narrowing Python version rangeThe current Python requirement
>=3.10,<4
is very broad. Consider setting an upper bound to a specific minor version based on your testing matrix (e.g.,>=3.10,<3.13
).
35-35
: Add upper bound for attrs dependencyAll other dependencies have upper version bounds except
attrs
. Consider adding an upper bound to prevent potential breaking changes:- "attrs>=23.2.0", + "attrs>=23.2.0,<24",
54-56
: Consider consolidating autopipeline implementationsThe presence of both
autopipeline
andbetapipeline
(refactored) suggests an incomplete migration. Consider:
- Documenting the differences between these implementations
- Creating a migration plan to consolidate into a single implementation
- Marking the legacy implementation as deprecated if needed
.github/workflows/main.yml (5)
1-8
: Optimize workflow triggersThe current configuration runs the full workflow on any branch push or PR, which could waste CI resources. Consider:
- Limiting pushes to main/development branches
- Using path filters to run only when relevant files change
name: Test on: push: - branches: ["*"] + branches: ["main", "devel"] + paths: + - "**/*.py" + - "pyproject.toml" + - "pixi.toml" pull_request: - branches: ["*"] + branches: ["main", "devel"] + paths: + - "**/*.py" + - "pyproject.toml" + - "pixi.toml"
61-68
: Enhance Codecov upload reliabilityConsider adding these improvements to the Codecov step:
- Retry mechanism for flaky uploads
- Validation step to ensure coverage file exists
+ - name: Verify coverage file exists + run: | + if [ ! -f ./coverage-report/coverage.xml ]; then + echo "Coverage file not found!" + exit 1 + fi - name: Use Codecov to track coverage uses: codecov/codecov-action@v3 with: token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage-report/coverage.xml fail_ci_if_error: true + retry_on_error: true + retry_count: 3 verbose: true name: codecov-umbrella
Line range hint
73-80
: Review permission scopesThe job requests broad permissions. Consider limiting them to only what's needed:
- Remove
actions: read
as it's not used- Consider if
issues
andpull-requests
write access is necessarypermissions: id-token: write - actions: read contents: write packages: write - issues: write - pull-requests: write
Line range hint
140-144
: Security: Streamline PyPI authenticationThe current configuration uses both token and password which is redundant and less secure.
- name: Publish to PyPI uses: pypa/gh-action-pypi-publish@release/v1 with: verbose: true - user: ${{ secrets.pypi_token }} - password: ${{ secrets.pypi_api }} + password: ${{ secrets.PYPI_TOKEN }}
147-167
: Remove or document commented codeThe
does-the-pip-squeak
job is commented out without explanation. Either:
- Remove it if it's no longer needed
- Document why it's disabled and when it will be re-enabled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pixi.lock
is excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/main.yml
(5 hunks).gitignore
(1 hunks)pixi.toml
(1 hunks)pyproject.toml
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- pixi.toml
🔇 Additional comments (2)
pyproject.toml (2)
70-85
: LGTM! Well-structured semantic release configuration
The semantic release configuration is well-defined with:
- Clear version variable mappings
- Appropriate branch patterns for main and development
- Proper prerelease token configuration
16-16
: Verify license change from GPL-3.0 to MIT
The license has been changed from GPL-3.0 to MIT. This is a significant change that requires careful consideration.
✅ Verification successful
License change from GPL-3.0 to MIT is properly implemented
The LICENSE file has been updated with the standard MIT license text, and no references to GPL/GNU were found in the codebase. The change is consistently reflected in both pyproject.toml and LICENSE file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify license change implications
# Check if LICENSE file exists and contains MIT license text
if [ -f LICENSE ]; then
echo "LICENSE file content:"
cat LICENSE
fi
# Check for any GPL-3.0 references in the codebase
echo "Checking for GPL-3.0 references:"
rg -i "GPL|GNU" --type txt --type md
Length of output: 1279
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.
the future of med-imagetools is bright!
* feat: started port to pixi * feat: updated test suite for pixi and started refactor of cicd * build: added coverage to gha * fix: explicitly direct location of package code and update pixi in pipeline * fix: added linux to pixi * chore: update GitHub Actions workflow for improved deployment process * chore: clean up GitHub Actions workflow by removing commented-out jobs and updating dependencies * chore: update GitHub Actions workflow to deploy from both main and devel branches, and refresh dependencies in pixi.lock * chore(sem-ver): 1.6.0-rc.1 * fix: update version_toml path in pyproject.toml for semantic release * chore(sem-ver): 1.6.0-rc.2 * chore: remove requirements.txt file and poetry.lock * chore: update authors and maintainers in pyproject.toml, and add __version__ in __init__.py * chore: remove setup.py file as part of project restructuring * chore: update pixi.lock and add ruff configuration for linting * chore: fix some recommendations from coderabbit --------- Co-authored-by: semantic-release <semantic-release>
Summary by CodeRabbit
New Features
__version__
set to "1.6.0-rc.2".Changes
Bug Fixes