Poetry >uv #144
Poetry >uv #144
Conversation
WalkthroughThe pull request introduces a comprehensive migration from Poetry to alternative tools like uv and Hatch for dependency management, building, and project configuration. The changes span multiple configuration files, including Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Makefile (2)
11-12: Consider adding error handling to setupThe setup target should verify the installation success and Python version compatibility.
setup: - pip install uv + python3 -c 'import sys; assert sys.version_info >= (3,9), "Python 3.9+ required"' + pip install uv || { echo "Failed to install uv"; exit 1; }
32-32: Consider adding test coverage reportingThe test command could benefit from including coverage reporting.
- PYTHONPATH=$(PYTHONPATH) uv run pytest -c pyproject.toml + PYTHONPATH=$(PYTHONPATH) uv run pytest -c pyproject.toml --cov=lighter --cov-report=term-missingpyproject.toml (1)
31-44: Consider relaxing some version constraintsSome dependency version constraints might be unnecessarily strict:
torch>=2.1.2,<3: Major version constraint might be too permissivenumpy< 2.0.0: Inconsistent spacing in version constrainttorchvision>=0.20.0,<0.21: Overly strict minor version constraint- "torch>=2.1.2,<3", + "torch>=2.1.2,<2.2", - "numpy< 2.0.0", + "numpy<2.0.0", - "torchvision>=0.20.0,<0.21", + "torchvision>=0.20.0,<0.21.0",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/release-drafter.yml(0 hunks).github/workflows/auto-publish.yml(2 hunks).github/workflows/ci.yml(0 hunks).github/workflows/release-drafter.yml(0 hunks)Makefile(1 hunks)pyproject.toml(2 hunks)
💤 Files with no reviewable changes (3)
- .github/release-drafter.yml
- .github/workflows/ci.yml
- .github/workflows/release-drafter.yml
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/auto-publish.yml
[error] 19-19: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
.github/workflows/auto-publish.yml (2)
17-17: LGTM: Installation step simplifiedThe change to use
make setupaligns with the migration to uv and provides a centralized setup process.
35-35: LGTM: Build step simplifiedThe change to
uv buildaligns with the new build system configuration.Makefile (2)
22-24: LGTM: Consistent formatting tools configurationThe formatting commands are properly configured using uvx with appropriate configuration paths.
50-50: LGTM: Comprehensive lint targetThe lint target properly combines all necessary checks.
pyproject.toml (2)
2-3: LGTM: Build system properly configuredThe build system configuration is correctly set up for Hatch.
67-71: LGTM: Default groups properly configuredThe uv configuration correctly includes both test and dev groups by default.
.github/workflows/auto-publish.yml
Outdated
| - name: Bump version | ||
| run: | | ||
| poetry version prerelease | ||
| uvx poetry version prerelease |
There was a problem hiding this comment.
💡 Codebase verification
Confirmed: Version bump command needs updating
The project uses Hatch as the build backend and UV for dependency management. Using Poetry's version command is inconsistent with this setup. Consider these alternatives:
- Use
uvx hatch versionif you want to leverage Hatch's version management - Use direct file manipulation with
sedor similar tools to update version in pyproject.toml
🔗 Analysis chain
Verify the version bump command compatibility
The command uvx poetry version prerelease seems to still rely on Poetry's versioning system. This appears inconsistent with the migration away from Poetry.
Consider using Hatch's versioning system instead, as it's now the build backend:
- uvx poetry version prerelease
+ uvx hatch version prerelease🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check build system configuration and version bump commands
echo "=== pyproject.toml content ==="
cat pyproject.toml 2>/dev/null || echo "No pyproject.toml found"
echo -e "\n=== Looking for Poetry/Hatch config files ==="
fd -H "poetry.lock|poetry.toml|hatch.toml" .
echo -e "\n=== Searching for version commands in workflows ==="
rg "version" .github/workflows/ -A 2 -B 2
Length of output: 7109
pyproject.toml
Outdated
| { name = "Suraj Pai", email = "[email protected]" }, | ||
| { name = "Keno Bressem", email = "[email protected]" }, | ||
| ] | ||
| requires-python = "~=3.9" |
There was a problem hiding this comment.
Resolve Python version compatibility mismatch
The requires-python = "~=3.9" constraint conflicts with the Python 3.8 classifier and configuration in other tools (black, isort, mypy).
Either update the classifiers to match the Python requirement or adjust the requirement:
- requires-python = "~=3.9"
+ requires-python = ">=3.8"Also applies to: 25-28
| Homepage = "https://github.com/lighter/lighter" | ||
| Repository = "https://github.com/lighter/lighter" |
There was a problem hiding this comment.
Fix repository URLs
The repository URLs appear to be incorrect, using "lighter/lighter" instead of "project-lighter/lighter".
- Homepage = "https://github.com/lighter/lighter"
- Repository = "https://github.com/lighter/lighter"
+ Homepage = "https://github.com/project-lighter/lighter"
+ Repository = "https://github.com/project-lighter/lighter"📝 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.
| Homepage = "https://github.com/lighter/lighter" | |
| Repository = "https://github.com/lighter/lighter" | |
| Homepage = "https://github.com/project-lighter/lighter" | |
| Repository = "https://github.com/project-lighter/lighter" |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Makefile (1)
9-10: Consider enhancing the setup targetThe setup target has been simplified to only install uv. Consider adding pre-commit hooks setup and other development dependencies installation.
setup: pip install uv + pip install pre-commit + pre-commit installpyproject.toml (1)
73-77: Consider including additional files in build targetsThe build targets only include the 'lighter' package. Consider including additional files like README.md, LICENSE, etc.
[tool.hatch.build.targets.sdist] -include = ["lighter"] +include = ["lighter", "README.md", "LICENSE", "CHANGELOG.md"] [tool.hatch.build.targets.wheel] -include = ["lighter"] +include = ["lighter"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(1 hunks)pyproject.toml(2 hunks)
🔇 Additional comments (5)
Makefile (2)
18-25: LGTM! Well-structured formatting targetsGood separation between codestyle and formatting targets. The implementation correctly uses uvx for running the formatting tools.
32-45: LGTM! Comprehensive linting setupGood separation of linting concerns into distinct targets with appropriate tool configurations.
pyproject.toml (3)
14-14: Resolve Python version compatibility mismatchThe
requires-python = "~=3.9"constraint conflicts with the Python 3.8 classifier.-requires-python = "~=3.9" +requires-python = ">=3.8"Also applies to: 25-28
48-49: Fix repository URLsThe repository URLs are incorrect, using "lighter/lighter" instead of "project-lighter/lighter".
-Homepage = "https://github.com/lighter/lighter" -Repository = "https://github.com/lighter/lighter" +Homepage = "https://github.com/project-lighter/lighter" +Repository = "https://github.com/project-lighter/lighter"
54-71: LGTM! Well-organized dependency groupsGood separation of test and development dependencies, with appropriate UV configuration to include both groups by default.
Makefile
Outdated
| #* Linting | ||
| .PHONY: test | ||
| test: | ||
| PYTHONPATH=$(PYTHONPATH) poetry run pytest -c pyproject.toml --cov-report=html --cov=lighter tests/ | ||
| PYTHONPATH=$(PYTHONPATH) uv run pytest -c pyproject.toml |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restore coverage reporting in test target
The coverage report generation has been removed from the test target. This might affect CI/CD pipeline expectations.
test:
- PYTHONPATH=$(PYTHONPATH) uv run pytest -c pyproject.toml
+ PYTHONPATH=$(PYTHONPATH) uv run pytest -c pyproject.toml --cov=lighter --cov-report=term-missing📝 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.
| #* Linting | |
| .PHONY: test | |
| test: | |
| PYTHONPATH=$(PYTHONPATH) poetry run pytest -c pyproject.toml --cov-report=html --cov=lighter tests/ | |
| PYTHONPATH=$(PYTHONPATH) uv run pytest -c pyproject.toml | |
| #* Linting | |
| .PHONY: test | |
| test: | |
| PYTHONPATH=$(PYTHONPATH) uv run pytest -c pyproject.toml --cov=lighter --cov-report=term-missing |
pyproject.toml
Outdated
| "torch>=2.1.2,<3", | ||
| "pandas>=1.5.3,<2", | ||
| "numpy< 2.0.0", | ||
| "pytorch-lightning>=2.1.3,<3", | ||
| "pydantic>=2.9.2,<3", | ||
| "fire>=0.5.0,<0.6", | ||
| "loguru>=0.6.0,<0.7", | ||
| "torchmetrics>=1.2.0,<2", | ||
| "tensorboard>=2.11.2,<3", | ||
| "requests>=2.31.0,<3", | ||
| "monai>=1.3.0,<2", | ||
| "rich>=13.7.0,<14", | ||
| "torchvision>=0.20.0,<0.21", | ||
| "lightly>=1.5.13,<2", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review dependency version constraints
Some dependency version constraints need attention:
- The numpy constraint
< 2.0.0is too loose and should specify a minimum version - Ensure torch and torchvision versions are compatible
- "numpy< 2.0.0",
+ "numpy>=1.24.0,<2.0.0",📝 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.
| "torch>=2.1.2,<3", | |
| "pandas>=1.5.3,<2", | |
| "numpy< 2.0.0", | |
| "pytorch-lightning>=2.1.3,<3", | |
| "pydantic>=2.9.2,<3", | |
| "fire>=0.5.0,<0.6", | |
| "loguru>=0.6.0,<0.7", | |
| "torchmetrics>=1.2.0,<2", | |
| "tensorboard>=2.11.2,<3", | |
| "requests>=2.31.0,<3", | |
| "monai>=1.3.0,<2", | |
| "rich>=13.7.0,<14", | |
| "torchvision>=0.20.0,<0.21", | |
| "lightly>=1.5.13,<2", | |
| "torch>=2.1.2,<3", | |
| "pandas>=1.5.3,<2", | |
| "numpy>=1.24.0,<2.0.0", | |
| "pytorch-lightning>=2.1.3,<3", | |
| "pydantic>=2.9.2,<3", | |
| "fire>=0.5.0,<0.6", | |
| "loguru>=0.6.0,<0.7", | |
| "torchmetrics>=1.2.0,<2", | |
| "tensorboard>=2.11.2,<3", | |
| "requests>=2.31.0,<3", | |
| "monai>=1.3.0,<2", | |
| "rich>=13.7.0,<14", | |
| "torchvision>=0.20.0,<0.21", | |
| "lightly>=1.5.13,<2", |
dosubot
bot
added
the
size:L
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
10-10: Remove trailing whitespace.There are trailing spaces on this line that should be removed.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
1-1: Document the migration from Poetry to uv.Given this significant change in dependency management:
- Please update the PR description to explain the motivation and impact of migrating from Poetry to uv
- Consider adding comments in the workflow file to document the new setup process
- Update the repository's documentation to reflect this change in the development workflow
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)lighter/utils/dynamic_imports.py(0 hunks)
💤 Files with no reviewable changes (1)
- lighter/utils/dynamic_imports.py
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
19-19: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/ci.yml
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 19-19: wrong indentation: expected 6 but found 4
(indentation)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
16-16: Consider adding Python 3.12 support.Python 3.12 is now stable. Consider verifying if your dependencies are compatible with Python 3.12 and adding it to the matrix if possible.
Run this script to check dependency compatibility with Python 3.12:
| uses: actions/cache@v2.1.6 | ||
| with: | ||
| path: .venv | ||
| key: venv-${{ matrix.python-version }}-${{ hashFiles('**/poetry.lock') }} | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| poetry config virtualenvs.in-project true | ||
| poetry install | ||
| key: venv-${{ matrix.python-version }}-${{ hashFiles('pyproject.toml') }}-${{ hashFiles('poetry.lock') }} | ||
|
|
There was a problem hiding this comment.
Update cache configuration for uv migration.
The cache configuration needs updates:
- The
actions/cacheaction is using an outdated version - The cache key includes
poetry.lockwhich is inconsistent with the migration to uv
- uses: actions/[email protected]
+ uses: actions/cache@v4
with:
path: .venv
- key: venv-${{ matrix.python-version }}-${{ hashFiles('pyproject.toml') }}-${{ hashFiles('poetry.lock') }}
+ key: venv-${{ matrix.python-version }}-${{ hashFiles('pyproject.toml') }}-${{ hashFiles('requirements.txt') }}📝 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.
| uses: actions/[email protected] | |
| with: | |
| path: .venv | |
| key: venv-${{ matrix.python-version }}-${{ hashFiles('**/poetry.lock') }} | |
| - name: Install dependencies | |
| run: | | |
| poetry config virtualenvs.in-project true | |
| poetry install | |
| key: venv-${{ matrix.python-version }}-${{ hashFiles('pyproject.toml') }}-${{ hashFiles('poetry.lock') }} | |
| uses: actions/cache@v4 | |
| with: | |
| path: .venv | |
| key: venv-${{ matrix.python-version }}-${{ hashFiles('pyproject.toml') }}-${{ hashFiles('requirements.txt') }} | |
| - uses: actions/checkout@v2 | ||
| - name: Set up Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v4 | ||
| uses: actions/setup-python@v2.2.2 |
There was a problem hiding this comment.
Update GitHub Actions to latest versions.
Both actions/checkout and actions/setup-python are using outdated versions. Update them to the latest versions to ensure security and feature improvements:
- - uses: actions/checkout@v2
- uses: actions/[email protected]
+ - uses: actions/checkout@v4
+ uses: actions/setup-python@v5📝 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.
| - uses: actions/checkout@v2 | |
| - name: Set up Python ${{ matrix.python-version }} | |
| uses: actions/setup-python@v4 | |
| uses: actions/[email protected] | |
| - uses: actions/checkout@v4 | |
| - name: Set up Python ${{ matrix.python-version }} | |
| uses: actions/setup-python@v5 |
🧰 Tools
🪛 actionlint (1.7.4)
19-19: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 19-19: wrong indentation: expected 6 but found 4
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lighter/__init__.py (1)
5-5: Add LighterSystem to allThe import is marked as unused by the static analyzer.
+__all__ = ["LighterSystem"]🧰 Tools
🪛 Ruff (0.8.2)
5-5:
.system.LighterSystemimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/auto-publish.yml(1 hunks).github/workflows/ci.yml(1 hunks)Makefile(1 hunks)lighter/__init__.py(1 hunks)pyproject.toml(4 hunks)tests/unit/test_system.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/test_system.py
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/auto-publish.yml
11-11: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/ci.yml
19-19: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/auto-publish.yml
[warning] 11-11: wrong indentation: expected 6 but found 8
(indentation)
[error] 19-19: trailing spaces
(trailing-spaces)
[error] 36-36: trailing spaces
(trailing-spaces)
.github/workflows/ci.yml
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 19-19: wrong indentation: expected 6 but found 4
(indentation)
🪛 Ruff (0.8.2)
lighter/__init__.py
5-5: .system.LighterSystem imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (8)
pyproject.toml (3)
73-77: LGTM: Build targets configurationThe build targets configuration for Hatch looks correct, properly including the "lighter" package for both sdist and wheel.
30-43: 🛠️ Refactor suggestionReview dependency version constraints
Some dependency version constraints need attention:
- The numpy constraint
< 2.0.0is too loose and should specify a minimum version- Ensure torch and torchvision versions are compatible
- "numpy< 2.0.0", + "numpy>=1.24.0,<2.0.0",Likely invalid or redundant comment.
47-48:⚠️ Potential issueFix repository URLs
The repository URLs are incorrect, using "lighter/lighter" instead of "project-lighter/lighter".
-Homepage = "https://github.com/lighter/lighter" -Repository = "https://github.com/lighter/lighter" +Homepage = "https://github.com/project-lighter/lighter" +Repository = "https://github.com/project-lighter/lighter"Likely invalid or redundant comment.
Makefile (2)
58-61: LGTM: Safety checks configurationThe safety checks using UV look good, properly configured to run both safety and bandit checks.
29-30: 🛠️ Refactor suggestionRestore coverage reporting in test target
The coverage report generation has been removed from the test target.
- uv run pytest -c pyproject.toml + uv run pytest -c pyproject.toml --cov=lighter --cov-report=term-missingLikely invalid or redundant comment.
.github/workflows/ci.yml (2)
19-21:⚠️ Potential issueUpdate GitHub Actions to latest versions
Both
actions/checkoutandactions/setup-pythonare using outdated versions.- - uses: actions/checkout@v2 - uses: actions/[email protected] + - uses: actions/checkout@v4 + uses: actions/setup-python@v5Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
19-19: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 19-19: wrong indentation: expected 6 but found 4
(indentation)
29-32:⚠️ Potential issueUpdate cache configuration for UV migration
The cache configuration needs updates:
- The
actions/cacheaction is using an outdated version- The cache key includes
poetry.lockwhich is inconsistent with the migration to UV- uses: actions/[email protected] + uses: actions/cache@v4 with: path: .venv - key: venv-${{ matrix.python-version }}-${{ hashFiles('pyproject.toml') }}-${{ hashFiles('poetry.lock') }} + key: venv-${{ matrix.python-version }}-${{ hashFiles('pyproject.toml') }}-${{ hashFiles('requirements.txt') }}Likely invalid or redundant comment.
.github/workflows/auto-publish.yml (1)
24-32: Review direct commits to main branchThe workflow commits and pushes directly to the main branch, which might bypass branch protection rules.
Consider creating a separate branch for version bumps and opening a PR instead.
| requires-python = "~=3.10" | ||
| readme = "README.md" | ||
| authors = ["Ibrahim Hadzic <[email protected]>" , | ||
| "Suraj Pai <[email protected]>", | ||
| "Keno Bressem <[email protected]>"] | ||
| license = "MIT" | ||
| repository = "https://github.com/lighter/lighter" | ||
| homepage = "https://github.com/lighter/lighter" | ||
|
|
||
| # Keywords description https://python-poetry.org/docs/pyproject/#keywords | ||
| keywords = [] #! Update me | ||
| packages = [ | ||
| { include = "lighter" }, | ||
| keywords = [] | ||
| classifiers = [ | ||
| "Development Status :: 3 - Alpha", | ||
| "Intended Audience :: Developers", | ||
| "Operating System :: OS Independent", | ||
| "Topic :: Software Development :: Libraries :: Python Modules", | ||
| "License :: OSI Approved :: MIT License", | ||
| "Programming Language :: Python :: 3", | ||
| "Programming Language :: Python :: 3.10", | ||
| "Programming Language :: Python :: 3.11", | ||
| "Programming Language :: Python :: 3.12", |
There was a problem hiding this comment.
💡 Codebase verification
Confirmed: Python version constraint is too restrictive
The CI workflow tests against Python 3.10, 3.11, and 3.12, but requires-python = "~=3.10" only allows 3.10.x versions. The suggested change to requires-python = ">=3.10,<4.0" will correctly reflect the actual Python version compatibility being tested.
🔗 Analysis chain
Resolve Python version compatibility mismatch
The requires-python = "~=3.10" constraint conflicts with the Python version matrix in CI (which includes 3.12).
Either update the Python requirement or adjust the CI matrix:
-requires-python = "~=3.10"
+requires-python = ">=3.10,<4.0"🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find GitHub Actions workflow files
fd -e yml -e yaml . .github/workflows/
# Search for Python version configurations in workflow files
fd -e yml -e yaml . .github/workflows/ --exec-batch rg -l "python-version|python_version"
# Also check for any other Python version constraints in the project
rg "python_version|requires-python" --type yaml --type toml
Length of output: 463
Script:
#!/bin/bash
# Check Python version configurations in workflow files
echo "=== CI Workflow ==="
rg "python-version" .github/workflows/ci.yml -A 3 -B 1
echo -e "\n=== Auto Publish Workflow ==="
rg "python-version" .github/workflows/auto-publish.yml -A 3 -B 1
echo -e "\n=== Docs Publish Workflow ==="
rg "python-version" .github/workflows/docs-publish.yml -A 3 -B 1
Length of output: 1134
| .PHONY: bump-prerelease | ||
| bump-prerelease: | ||
| uvx --with poetry-bumpversion poetry version prerelease | ||
|
|
||
| # Run tests with coverage report | ||
| .PHONY: test | ||
| test: | ||
| PYTHONPATH=$(PYTHONPATH) poetry run pytest -c pyproject.toml --cov-report=html --cov=lighter tests/ | ||
| .PHONY: bump-patch | ||
| bump-patch: | ||
| uvx --with poetry-bumpversion poetry version patch | ||
|
|
||
| # Generate coverage badge | ||
| .PHONY: coverage | ||
| coverage: | ||
| poetry run coverage-badge -o assets/images/coverage.svg -f | ||
| .PHONY: bump-minor | ||
| bump-minor: | ||
| uvx --with poetry-bumpversion poetry version minor | ||
|
|
||
| ### Dependency Management | ||
| # Update development dependencies | ||
| .PHONY: update-dev-deps | ||
| update-dev-deps: | ||
| poetry add -G dev bandit@latest "isort[colors]@latest" mypy@latest pre-commit@latest pydocstyle@latest \ | ||
| pylint@latest pytest@latest pyupgrade@latest safety@latest coverage@latest coverage-badge@latest \ | ||
| pytest-html@latest pytest-cov@latest | ||
| poetry add -G dev --allow-prereleases black@latest | ||
| .PHONY: bump-major | ||
| bump-major: | ||
| uvx --with poetry-bumpversion poetry version major |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update version bump commands for Hatch
The version bump commands still use Poetry's version command through poetry-bumpversion, which is inconsistent with the migration to Hatch.
- uvx --with poetry-bumpversion poetry version prerelease
+ uvx hatch version preSimilar changes needed for patch, minor, and major version bumps.
📝 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.
| .PHONY: bump-prerelease | |
| bump-prerelease: | |
| uvx --with poetry-bumpversion poetry version prerelease | |
| # Run tests with coverage report | |
| .PHONY: test | |
| test: | |
| PYTHONPATH=$(PYTHONPATH) poetry run pytest -c pyproject.toml --cov-report=html --cov=lighter tests/ | |
| .PHONY: bump-patch | |
| bump-patch: | |
| uvx --with poetry-bumpversion poetry version patch | |
| # Generate coverage badge | |
| .PHONY: coverage | |
| coverage: | |
| poetry run coverage-badge -o assets/images/coverage.svg -f | |
| .PHONY: bump-minor | |
| bump-minor: | |
| uvx --with poetry-bumpversion poetry version minor | |
| ### Dependency Management | |
| # Update development dependencies | |
| .PHONY: update-dev-deps | |
| update-dev-deps: | |
| poetry add -G dev bandit@latest "isort[colors]@latest" mypy@latest pre-commit@latest pydocstyle@latest \ | |
| pylint@latest pytest@latest pyupgrade@latest safety@latest coverage@latest coverage-badge@latest \ | |
| pytest-html@latest pytest-cov@latest | |
| poetry add -G dev --allow-prereleases black@latest | |
| .PHONY: bump-major | |
| bump-major: | |
| uvx --with poetry-bumpversion poetry version major | |
| .PHONY: bump-prerelease | |
| bump-prerelease: | |
| uvx hatch version pre | |
| .PHONY: bump-patch | |
| bump-patch: | |
| uvx hatch version patch | |
| .PHONY: bump-minor | |
| bump-minor: | |
| uvx hatch version minor | |
| .PHONY: bump-major | |
| bump-major: | |
| uvx hatch version major |
| - uses: actions/checkout@v2 | ||
| - name: Set up Python | ||
| uses: actions/[email protected] | ||
| with: | ||
| python-version: 3.11 |
There was a problem hiding this comment.
Update GitHub Actions versions
The workflow is using outdated versions of GitHub Actions.
- - uses: actions/checkout@v2
- uses: actions/[email protected]
+ - uses: actions/checkout@v4
+ uses: actions/setup-python@v5📝 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.
| - uses: actions/checkout@v2 | |
| - name: Set up Python | |
| uses: actions/setup-python@v2.2.2 | |
| with: | |
| python-version: 3.11 | |
| - uses: actions/checkout@v4 | |
| - name: Set up Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: 3.11 |
🧰 Tools
🪛 actionlint (1.7.4)
11-11: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 11-11: wrong indentation: expected 6 but found 8
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/unit/test_callbacks_writer_file.py (1)
Line range hint
1-9: Fix import statement issues.
- Remove the unused
tempfileimport as it's been replaced by pytest'stmp_pathfixture.- Fix the import order to comply with the style guidelines.
Apply this diff to fix the issues:
+import shutil +from pathlib import Path + -import shutil -from pathlib import Path - import monai import numpy as np import pytest import torch from PIL import Image -import tempfile🧰 Tools
🪛 Ruff (0.8.2)
9-9:
tempfileimported but unusedRemove unused import:
tempfile(F401)
🧹 Nitpick comments (1)
tests/unit/test_callbacks_writer_file.py (1)
Line range hint
14-143: Consider expanding test coverage.The test suite is comprehensive, but consider adding these test cases:
- Different file extensions (e.g.,
.jpgfor images)- Various tensor shapes and data types
- Concurrent write operations to test thread safety
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/test_callbacks_writer_file.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit/test_callbacks_writer_file.py
9-9: tempfile imported but unused
Remove unused import: tempfile
(F401)
🪛 GitHub Actions: Lighter CI
tests/unit/test_callbacks_writer_file.py
[error] 1-9: Imports are incorrectly sorted and/or formatted. The 'tempfile' import should be moved before the 'from pathlib import Path' statement.
🔇 Additional comments (1)
tests/unit/test_callbacks_writer_file.py (1)
Line range hint
14-143: Great improvements to test infrastructure!The migration to pytest's
tmp_pathfixture is a significant improvement:
- Better test isolation
- Automatic cleanup of test artifacts
- Prevention of test interference
- Alignment with pytest best practices
The comprehensive docstrings and test structure are well maintained.
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/test_callbacks_writer_file.py (3)
71-74: Consider additional image property verificationsWhile the basic dimension check is good, consider adding verifications for:
- Image mode (should be RGB)
- Pixel value range (0-255)
- File format properties
# Verify image contents image = Image.open(saved_path) + assert image.mode == "RGB" image_array = np.array(image) assert image_array.shape == (64, 64, 3) + assert np.all((image_array >= 0) & (image_array <= 255))
91-93: Enhance video file verificationThe tests only verify file existence. Consider adding checks for:
- Video duration
- Frame rate
- Resolution
- Color space
Consider using a library like
cv2ormoviepyto verify video properties:from moviepy.editor import VideoFileClip def verify_video_properties(file_path, expected_frames, expected_size): with VideoFileClip(str(file_path)) as clip: assert clip.duration == expected_frames / 30 # Assuming 30fps assert clip.size == expected_sizeAlso applies to: 112-114
Line range hint
143-165: Consider using tmp_path for consistencyFor consistency with other tests, consider updating this test to use the
tmp_pathfixture instead of creating temporary files in the current directory.-def test_file_writer_invalid_directory(): +def test_file_writer_invalid_directory(tmp_path): """Test error handling for invalid directory paths in LighterFileWriter.""" - test_file = Path("test_file.txt") + test_file = tmp_path / "test_file.txt" test_file.touch() try: with pytest.raises(RuntimeError, match="LighterFileWriter expects a directory path"): writer = LighterFileWriter(path=test_file, writer="tensor") writer.write(torch.tensor([1, 2, 3]), id=1) finally: test_file.unlink() with pytest.raises(RuntimeError): - writer = LighterFileWriter(path=Path("invalid_dir"), writer="tensor") + writer = LighterFileWriter(path=tmp_path / "invalid_dir", writer="tensor") writer.write(torch.tensor([1, 2, 3]), id=1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/test_callbacks_writer_file.py(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (3.12)
🔇 Additional comments (3)
tests/unit/test_callbacks_writer_file.py (3)
Line range hint
12-25: Great use of pytest's tmp_path fixture!The change improves test isolation and reduces complexity by eliminating manual directory cleanup. The docstring effectively documents the test's objectives and verification steps.
Line range hint
117-141: Well-structured test with good error handling!The test effectively covers both error and success cases, properly validates MONAI MetaTensor requirements, and includes appropriate assertions.
48-48: Verify security implications of torch.loadThe
nosec B614comment suppresses a security warning fortorch.load. While this might be acceptable in tests, it's worth documenting why this is safe in this context, astorch.loadcan execute arbitrary code.Consider adding a comment explaining the security implications or using
torch.loadwithpickle_module=Noneif possible:- loaded_tensor = torch.load(saved_path) # nosec B614 + # Safe in tests as we control the saved tensor + loaded_tensor = torch.load(saved_path, pickle_module=None) # Safer alternative✅ Verification successful
Security suppression is acceptable in this test context
The
nosec B614suppression is justified here as:
- The tensor being loaded is created and saved within the same test
- No external or untrusted data is involved
- The test environment is controlled
The current implementation is safe for testing purposes, and no changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find all instances of torch.load rg "torch\.load" -B 2 -A 2 # Look for security-related documentation about torch.load rg -i "security|pickle|torch\.load" "**/*.md" # Find any pickle-related configurations rg "pickle_module" -B 2 -A 2Length of output: 1227
|
@ibro45 Migration to UV complete! No more poetry 👋🏽 |
|
wdy say about making monai>=1.4.0, the bundle error messages are much better from that version, as you very well know 😁 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pyproject.toml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (4)
pyproject.toml (4)
2-3: LGTM: Build system configuration updated for HatchThe build system configuration has been correctly updated to use Hatch as the build backend.
54-77: LGTM: Development configuration is well-structuredThe development dependencies, UV configuration, and Hatch build targets are properly configured:
- Comprehensive set of development dependencies
- UV default groups correctly set
- Build targets properly specify the package inclusion
47-48:⚠️ Potential issueFix repository URLs
The repository URLs are incorrect, using "lighter/lighter" instead of "project-lighter/lighter".
-Homepage = "https://github.com/lighter/lighter" -Repository = "https://github.com/lighter/lighter" +Homepage = "https://github.com/project-lighter/lighter" +Repository = "https://github.com/project-lighter/lighter"Likely invalid or redundant comment.
29-43: Review dependency version constraintsSome dependency version constraints need attention:
- The numpy constraint
< 2.0.0should specify a minimum version- Verify compatibility between torch 2.1.2 and torchvision 0.20.0
Apply this change for numpy:
- "numpy< 2.0.0", + "numpy>=1.24.0,<2.0.0",
| { name = "Suraj Pai", email = "[email protected]" }, | ||
| { name = "Keno Bressem", email = "[email protected]" }, | ||
| ] | ||
| requires-python = "~=3.10" |
There was a problem hiding this comment.
Update Python version constraint to match classifiers
The requires-python = "~=3.10" constraint only allows 3.10.x versions, but the classifiers include Python 3.10, 3.11, and 3.12.
Apply this change:
-requires-python = "~=3.10"
+requires-python = ">=3.10,<4.0"Also applies to: 18-28
|
@ibro45 Any other considerations? If not, let's merge. I also want to test if the new publish works as expected. |
|
https://tsak.dev/posts/python-enum/ Not sure if it's worth it, but if we drop 3.10, i will use StrEnum instead of (str, Enum). Not a big deal at all. Otherwise good to go! |
|
No reason to drop 3.10 yet. EOL is somewhere late 26 and afaik its the default now for most systems. |
Description
Related Issue
Type of Change
Checklist
CODE_OF_CONDUCT.mddocument.CONTRIBUTING.mdguide.make codestyle.Summary by CodeRabbit
Chores
New Features
Refactor
uv.