feat: Enhance CI/CD Workflow with TestPyPI Publishing, Linting Improvements, and minor docs update #42
Conversation
…yle to numpy, and enhance loader type annotations
…ts and comment out previous push configuration
…r linting and formatting tasks
… and improving module documentation for DICOM and RTSTRUCT functions
… SHA for dependency consistency
…o test PyPI and add description for build task
…w environment variables for authentication
…lation steps for verifying package deployment
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #42 +/- ##
==========================================
+ Coverage 63.54% 63.96% +0.41%
==========================================
Files 9 9
Lines 524 530 +6
==========================================
+ Hits 333 339 +6
Misses 191 191 ☔ View full report in Codecov by Sentry. |
…t-Tests job before deployment to PyPI
WalkthroughThe pull request introduces significant updates to the CI-CD workflow, including new jobs for linting, publishing, and testing installations. The Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (7)
src/readii/loaders.py (7)
1-9: Update module docstring to comply with NumPy styleThe module docstring provides a good overview, but to maintain consistency with the updated linting rules and the NumPy docstring style guide, consider reformatting it accordingly.
37-39: Redundant conversion toPathSince
imgDirPathis converted to aPathobject, ensure that this conversion is necessary and that all usages ofimgDirPathrequire it. IfimgDirPathis already aPath, the conversion is harmless but redundant.
68-71: Redundant conversion toPathEnsure that converting
rtstructPathandbaseImageDirPathtoPathobjects is necessary. If they are alreadyPathinstances, the conversion is redundant but not harmful.
Line range hint
75-85: Silent exception handling without loggingIn the exception block, the
ValueErroris caught, and an empty dictionary is returned without any logging. This could make debugging difficult if issues arise.Consider logging the exception before returning:
except ValueError as e: + logger.error(f"Failed to create ROI masks: {e}") return {}
111-138: Update function docstrings to comply with NumPy styleThe function docstrings should be reformatted to adhere to the NumPy docstring conventions, ensuring consistency and improving readability across the codebase.
146-151: Normalizemodalityinput for case-insensitive comparisonTo make the modality checks more robust, consider converting the
modalityparameter to lowercase before comparison.Apply this change:
+ modality = modality.lower() if modality in ["seg"]: # SEG handling code elif modality in ["rtstruct"]: # RTSTRUCT handling code
154-158: Ensure consistent indentationThe indentation for the
raise ValueErrorstatement and the subsequentelseblock should be consistent to maintain code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
- .github/workflows/ci-cd.yml (4 hunks)
- config/ruff.toml (3 hunks)
- pyproject.toml (3 hunks)
- src/readii/loaders.py (3 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/ci-cd.yml
[error] 198-198: trailing spaces
(trailing-spaces)
[error] 220-220: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (5)
config/ruff.toml (3)
65-65: Verify quote style migration.The change from single to double quotes will require updating all string literals in the codebase. Ensure that the auto-formatter is run across all Python files to maintain consistency.
#!/bin/bash # Search for single quotes that might need updating rg "'" --type py --glob '!tests/*'
61-61: Verify docstring migration from Google to NumPy style.Changing the docstring convention from Google to NumPy style is a significant change that requires updating all existing docstrings to maintain consistency.
✅ Verification successful
Let me try a different approach to search for docstrings using
ast-grepwith a more specific pattern.This will help us identify if there are Google-style docstrings that need updating and if any NumPy-style docstrings already exist, giving us a better picture of the migration status.
Based on the search results, I can now generate the final response:
No migration needed - codebase already uses NumPy docstring style
The codebase is already consistently using NumPy-style docstrings across multiple files (loaders.py, negative_controls.py, metadata.py, image_processing.py, feature_extraction.py) as evidenced by the NumPy-style section headers:
Parametersfollowed by----------Returnsfollowed by-------Raisesfollowed by------No Google-style docstrings (with
Args:,Returns:,Raises:) were found. The change in ruff.toml is actually aligning the configuration with the existing codebase convention.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential Google-style docstrings that need updating ast-grep --pattern 'def $_($$) { """ Args: $$$ """ $$$ }'Length of output: 6232
Script:
#!/bin/bash # Search for Google-style docstrings with Args, Returns, Raises sections rg "^\s+Args:|^\s+Returns:|^\s+Raises:" --type py --multiline # Also search for NumPy-style docstrings to see if migration is in progress rg "^\s+Parameters\n\s+-+|^\s+Returns\n\s+-+|^\s+Raises\n\s+-+" --type py --multilineLength of output: 5797
30-36: Verify the impact of disabled linting rules.While adding pydocstyle (
D) is beneficial, disablingpep8-naming(N) andflake8-logging-format(G) might lead to:
- Inconsistent naming conventions across the codebase
- Inconsistent logging format patterns
Consider keeping these rules enabled to maintain code quality standards.
pyproject.toml (1)
116-126:⚠️ Potential issueEnhance publish-test task with authentication and validation
The TestPyPI publishing task needs additional safeguards:
- Add required environment variables for authentication:
publish-test = { cmd = [ "hatch", "publish", "--yes", "--repo", "test", ], inputs = [ "dist/*", ], depends-on = [ "build", - ], env = { HATCH_CONFIG = "config/hatch.toml" }, description = "Publish to test PyPI" } + ], env = { + "HATCH_CONFIG" = "config/hatch.toml", + "HATCH_INDEX_AUTH" = "${HATCH_INDEX_AUTH}", + }, description = "Publish to test PyPI" }
- Add a validation step to ensure the build artifacts exist and are valid:
publish-test = { cmd = [ + "sh", + "-c", + """ + if [ ! -f "dist/*.whl" ] || [ ! -f "dist/*.tar.gz" ]; then + echo "Error: Build artifacts missing" >&2 + exit 1 + fi "hatch", "publish",.github/workflows/ci-cd.yml (1)
5-9: Review workflow trigger configurationThe current trigger configuration might prevent automatic deployments:
- The commented-out push trigger means the workflow won't run on direct pushes to any branch
- This could prevent automatic releases when merging to main, as the
Semantic-Releasejob specifically checks forrefs/heads/mainConsider keeping the push trigger for the main branch:
on: push: branches: ["main"] pull_request: branches: ["*"]
…ed modalities in loadSegmentation function
…espace and ensuring proper formatting at end of file
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
pyproject.toml (2)
88-91: Update format task description to match actual capabilitiesThe description mentions using
--diffbut since Pixi doesn't support command options (as per previous discussion), consider updating the description to be more accurate.-description = "Run ruff formatter, use `--diff` to see changes only" +description = "Run ruff formatter"
111-112: Consider using more precise branch patternsWhile better than "*", the current pattern could be more precise. Consider using start/end anchors to prevent partial matches.
-match = "(main|development)" +match = "^(main|development)$"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
- .github/workflows/ci-cd.yml (4 hunks)
- pyproject.toml (2 hunks)
- src/readii/loaders.py (3 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/ci-cd.yml (1)
Learnt from: jjjermiah PR: bhklab/readii#42 File: .github/workflows/ci-cd.yml:145-152 Timestamp: 2024-10-22T17:19:25.869Z Learning: Do not suggest optimizing cache configuration in the Semantic-Release job in `.github/workflows/ci-cd.yml`.
🔇 Additional comments (7)
pyproject.toml (1)
135-145: LGTM! Well-structured TestPyPI publishing configurationThe publish-test task is properly configured with:
- Correct dependency on the build task
- Appropriate repository specification
- Proper environment configuration
src/readii/loaders.py (5)
1-9: Well-structured module documentation!The added module docstring provides clear and comprehensive documentation about the module's purpose and capabilities.
25-38: Type annotation compatibility issue persistsThe use of
str | Pathtype annotation requires Python 3.10+.
72-74: Good logging and path resolution implementation!The addition of debug logging and proper path resolution using
.resolve()improves the robustness of the function.
140-143: Improved error message clarity!The error message now includes the actual modality value, making it more helpful for debugging.
145-147: Verify path traversal protectionThe code handles user-provided paths. While using
Pathobjects is good, let's verify that the codebase has proper path traversal protection.✅ Verification successful
Let me check the actual implementation of path handling in
loaders.pyto make a more informed decision.The initial results show path-related operations, but I need to see the actual implementation to verify if there's proper path validation. This will help determine if there are any security concerns with path handling.
Based on the code inspection, I can now provide a final response:
Path handling implementation is secure
The code implements proper path handling through:
- Consistent conversion of input paths to
Pathobjects- Use of
resolve()to get absolute paths before file operations- Proper validation of input types and modalities
- No direct path concatenation or manipulation that could lead to traversal
The use of
Path.resolve()before file operations ensures that symbolic links are resolved and absolute paths are used, which is a good security practice. The parent directory access (Path.parent) is used safely within the context of already resolved paths.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for path validation or sanitization in the codebase echo "Searching for path validation patterns..." rg -l "resolve\(\)" --type py rg -l "absolute\(\)" --type py rg -l "Path\.parent" --type py rg -l "pathlib\.Path" --type py echo "Checking for potential path traversal vulnerabilities..." rg -l "\.\./" --type py rg -l "\.\." --type pyLength of output: 569
Script:
#!/bin/bash # Check path handling implementation in loaders.py echo "Checking path handling in loaders.py..." rg "resolve\(\)" -A 5 -B 5 src/readii/loaders.py rg "Path\.parent" -A 5 -B 5 src/readii/loaders.py # Check how the paths are used echo "\nChecking path usage patterns..." rg "Path\(" -A 5 -B 5 src/readii/loaders.pyLength of output: 3446
.github/workflows/ci-cd.yml (1)
106-106: LGTM: Semantic Release job changesThe addition of the Ruff job as a dependency ensures linting passes before releases.
Also applies to: 145-152
…TRUCT validation and simplifying modality checks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pyproject.toml (1)
82-91: Clean up style tasks configurationMinor improvements needed:
- Remove extra space in lint command
- Clarify how to use the
--diffoption in format task description[tool.pixi.feature.style.tasks.lint] -cmd = "ruff check --config=config/ruff.toml" +cmd = "ruff check --config=config/ruff.toml" [tool.pixi.feature.style.tasks.format] cmd = "ruff format --config=config/ruff.toml" inputs = ["src", "config/ruff.toml"] -description = "Run ruff formatter, use `--diff` to see changes only" +description = "Run ruff formatter. Add '--diff' to preview changes without applying them"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
- pyproject.toml (2 hunks)
- src/readii/loaders.py (3 hunks)
🧰 Additional context used
🔇 Additional comments (6)
pyproject.toml (1)
135-145: Enhance publish-test task configurationThe task configuration looks good but could benefit from additional documentation and error handling considerations.
Consider updating the description to be more informative:
-], env = { HATCH_CONFIG = "config/hatch.toml" }, description = "Publish to test PyPI" } +], env = { HATCH_CONFIG = "config/hatch.toml" }, description = "Build and publish package to TestPyPI for pre-release testing" }Ensure TestPyPI credentials are properly configured in CI:
#!/bin/bash # Check if TestPyPI token is configured in GitHub Actions gh api /repos/bhklab/readii/actions/secrets | jq -r '.secrets[] | select(.name | contains("PYPI")) | .name'src/readii/loaders.py (5)
1-23: Well-structured module documentation and imports!The comprehensive module docstring and organized imports provide clear context and maintainable structure.
Line range hint
25-44: Update type annotation for Python <3.10 compatibilityThe function implementation is solid with good path handling and logging. However, as noted in previous reviews, the type annotation needs updating for broader Python version compatibility.
Apply this change:
+from typing import Union -def loadDicomSITK(imgDirPath: str | Path) -> sitk.Image: +def loadDicomSITK(imgDirPath: Union[str, Path]) -> sitk.Image:
Line range hint
48-103: Update type annotations for Python <3.10 compatibilityThe function implementation is well-structured with proper path handling and logging. However, as noted in previous reviews, the type annotations need updating for broader Python version compatibility.
Apply this change:
+from typing import Union -def loadRTSTRUCTSITK( - rtstructPath: str | Path, baseImageDirPath: str | Path, roiNames: Optional[str] = None -) -> Dict[str, sitk.Image]: +def loadRTSTRUCTSITK( + rtstructPath: Union[str, Path], + baseImageDirPath: Union[str, Path], + roiNames: Optional[str] = None +) -> Dict[str, sitk.Image]:
140-166: Great improvements in error handling and path management!The changes show excellent attention to detail:
- More informative error messages with the actual modality value
- Consistent path handling with explicit conversions
- Proper case-insensitive modality checking
106-110:⚠️ Potential issueUpdate type annotations for Python <3.10 compatibility
The function signature needs updating for broader Python version compatibility.
Apply this change:
+from typing import Union -def loadSegmentation( - segImagePath: str | Path, - modality: str, - baseImageDirPath: Optional[str | Path] = None, - roiNames: Optional[str] = None, -) -> Dict[str, sitk.Image]: +def loadSegmentation( + segImagePath: Union[str, Path], + modality: str, + baseImageDirPath: Optional[Union[str, Path]] = None, + roiNames: Optional[str] = None, +) -> Dict[str, sitk.Image]:
Summary by CodeRabbit
New Features
Ruff) to enhance code quality.ruffin project configuration.Bug Fixes
Documentation
Chores
1.11.0and improved configuration file organization.