refactor: improve logging, logic, and type annotations #151
Conversation
UserWarning: This pattern is interpreted as a regular expression, and has match groups. To actually get the groups, use str.extract.
…e logging behavior
…getLogger for better logger management
…proved graph creation
…xing and graph creation
…nt and coverage reporting
|
Warning Rate limit exceeded@jjjermiah has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughThe pull request includes significant updates to configuration files and source code to enhance type checking, linting, and logging functionality. 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: 2
🧹 Outside diff range and nitpick comments (18)
src/imgtools/modules/structureset.py (1)
40-40: Add type annotation for suppress_warnings parameterThe method signature could be improved by adding a type annotation for the new parameter.
-def from_dicom_rtstruct(cls, rtstruct_path: str, suppress_warnings: bool = False) -> 'StructureSet': +def from_dicom_rtstruct(cls, rtstruct_path: str, suppress_warnings: bool = False) -> 'StructureSet':src/imgtools/io/loaders.py (1)
80-81: Add type hints to read_dicom_rtstruct functionThe function should include type hints for consistency with other functions in the module.
-def read_dicom_rtstruct(path, suppress_warnings=False): +def read_dicom_rtstruct(path: str, suppress_warnings: bool = False) -> StructureSet:src/imgtools/dicom/sort/sort_method.py (1)
Line range hint
109-127: Consider enhancing error messages with examplesThe error handling is comprehensive, but the error messages could be more helpful by including usage examples.
if not source_path.exists(): - msg = f'Source does not exist: {source_path}' + msg = f'Source does not exist: {source_path}. Ensure the file exists before proceeding.' if not overwrite and resolved_path.exists(): - msg = f'Destination already exists: {resolved_path}' + msg = f'Destination already exists: {resolved_path}. Use overwrite=True to force the operation.'src/imgtools/dicom/sort/dicomsorter.py (1)
Line range hint
99-127: Consider enhancing logging for better debuggingWhile the error handling is comprehensive, adding more detailed logging could help with debugging issues in production.
if not isinstance(action, FileAction): + self.logger.debug(f'Converting string action "{action}" to FileAction') action = FileAction.validate(action) + self.logger.debug(f'Action validated successfully: {action}')config/pytest.ini (2)
17-26: Consider using variables for coverage report pathsThe coverage report paths could be made more maintainable by using variables. This would make it easier to update all paths at once if needed.
- --cov-report=xml:coverage-report/coverage.xml - --cov-report=html:coverage-report/htmlcov + --cov-report=xml:${COVERAGE_DIR}/coverage.xml + --cov-report=html:${COVERAGE_DIR}/htmlcov
8-29: Consider adding parallel test execution configurationFor larger test suites, parallel execution can significantly reduce test runtime. Consider adding the
--numprocesses autooption if using pytest-xdist.addopts = --verbose --showlocals + --numprocesses auto --cov=imgtoolssrc/imgtools/cli/__init__.py (2)
8-12: Update docstring to include new parameterThe docstring is missing documentation for the
logger_nameparameter.def set_log_verbosity( *param_decls: str, logger_name: str = 'imgtools', quiet_decl: tuple = ('--quiet', '-q'), **kwargs: Any, # noqa ) -> Callable[[FC], FC]: """ Add a `--verbose` option to set the logging level based on verbosity count and a `--quiet` option to suppress all logging except errors. Parameters ---------- *param_decls : str Custom names for the verbosity flag. + logger_name : str, optional + Name of the logger to configure, defaults to 'imgtools'. quiet_decl : tuple Tuple containing custom names for the quiet flag. **kwargs : Any Additional keyword arguments for the click option.
Line range hint
31-50: Simplify callback logic and improve type safetyThe callback function could be simplified and made more type-safe:
- Use an Enum for logging levels
- Simplify the level comparison logic
+from enum import IntEnum + +class LogLevel(IntEnum): + ERROR = 0 + WARNING = 1 + INFO = 2 + DEBUG = 3 def callback(ctx: click.Context, param: click.Parameter, value: int) -> None: - levels = {0: 'ERROR', 1: 'WARNING', 2: 'INFO', 3: 'DEBUG'} - level = levels.get(value, 'DEBUG') # Default to DEBUG if verbosity is high + level = LogLevel(min(value, LogLevel.DEBUG)).name logger = getLogger(logger_name) if ctx.params.get('quiet', False): logger.setLevel(ERROR) return levelvalue = getLevelName(level) - env_level = logger.level - - # Override environment variable if verbosity level is passed - if levelvalue > env_level and value != 0: + if value != 0 and levelvalue > logger.level: logger.warning( - f'Environment variable {logger.name.upper()}_LOG_LEVEL is {getLevelName(env_level)} but ' + f'Environment variable {logger.name.upper()}_LOG_LEVEL is {getLevelName(logger.level)} but ' f'you are setting it to {getLevelName(levelvalue)}' ) - logger.setLevel(levelvalue) - else: - logger.setLevel(min(levelvalue, env_level)) + logger.setLevel(levelvalue)src/imgtools/dicom/index/__main__.py (3)
10-11: Consider moving constants to a configuration fileThe database path constants should be moved to a configuration file for better maintainability and flexibility.
Consider creating a
config.pyfile:from pathlib import Path class DatabaseConfig: DEFAULT_DIR = Path('.imgtools') DEFAULT_NAME = 'imgtools.db' @classmethod def get_default_path(cls) -> Path: return cls.DEFAULT_DIR / cls.DEFAULT_NAME
Line range hint
65-73: Enhance error handling and logging structureThe warning message could be improved with structured logging and better error handling.
if not dicom_files: - warningmsg = f'No DICOM files found in {directory}.' logger.warning( - warningmsg, + 'No DICOM files found in directory', directory=directory, check_header=check_header, recursive=True, extension=extension, + error_code='NO_DICOM_FILES' ) - return + raise click.UsageError('No DICOM files found. Please check the directory path and file extensions.')
88-89: Add progress tracking for long-running operationsThe index building operation could benefit from progress tracking for better user experience.
-indexer.build_index_from_files(dicom_files) +with click.progressbar(dicom_files, label='Building index') as files: + indexer.build_index_from_files(files)config/mypy.ini (1)
23-37: Consider gradual adoption of strict type checkingThe configuration enables multiple strict type checking options simultaneously. Consider implementing these checks gradually to make the transition smoother:
- Start with
disallow_untyped_defs- Then enable
check_untyped_defs- Finally add
disallow_incomplete_defsanddisallow_untyped_decoratorsThis approach would help manage the technical debt more effectively.
config/ruff.toml (2)
Line range hint
6-11: Simplify file inclusion strategyThe current approach of commenting out certain paths (
src/imgtools/cli/**/*.py,src/imgtools/utils/crawl.py) makes the configuration harder to maintain. Consider either:
- Including them with specific rule exemptions, or
- Removing the commented lines to keep the configuration clean
101-108: Document rule ignoresSome ignored rules lack documentation explaining why they're ignored:
PLR2004: Magic numbersD206: Docstring indentationN813: Camelcase importConsider adding comments explaining the rationale for ignoring these rules, similar to the existing comment for
COM812.pixi.toml (1)
116-118: Consider adding format verification stepThe
_ruff-formattask depends on_ruff-check, but there's no verification that the formatting was successful. Consider adding a post-format check to ensure the changes were applied correctly._ruff-format.cmd = ["ruff", "--config", "config/ruff.toml", "format", "src"] _ruff-format.inputs = ["config/ruff.toml", "src"] _ruff-format.depends_on = ["_ruff-check"] +_ruff-format.outputs = ["src"] _ruff-format.description = "Run ruff format, run check first" + +_verify-format.cmd = ["ruff", "--config", "config/ruff.toml", "check", "src"] +_verify-format.inputs = ["config/ruff.toml", "src"] +_verify-format.depends_on = ["_ruff-format"] +_verify-format.description = "Verify formatting was applied correctly"src/imgtools/utils/crawl.py (1)
Line range hint
119-121: Improve error handling to align with PR objectives.The current error handling could be enhanced to:
- Use the logger instead of print
- Catch specific exceptions instead of bare except
- Consider whether silently passing errors is appropriate
Consider this improvement:
- except Exception as e: - print(folder, e) - pass + except Exception as e: + logger.error(f"Error processing DICOM in {folder}: {str(e)}") + # Consider whether to continue or raise based on error severity🧰 Tools
🪛 Ruff (0.8.0)
186-186: f-string without any placeholders
Remove extraneous
fprefix(F541)
src/imgtools/ops/ops.py (2)
136-138: Remove commented-out codeThe old path construction code is commented out and should be removed since it's no longer needed and the new implementation is better.
- # self.csv_path = pathlib.Path(self.parent, ".imgtools", f"imgtools_{self.dataset_name}.csv") - # self.json_path = pathlib.Path(self.parent, ".imgtools", f'imgtools_{self.dataset}.json') - # self.edge_path = pathlib.Path(self.parent,".imgtools",f"imgtools_{self.dataset_name}_edges.csv")
163-167: Consider improving readability of the output streams constructionWhile the list comprehension is efficient, the code could be more readable with:
- Better variable names
- A brief comment explaining the purpose
- Breaking down the complex list comprehension
+ # Extract modality names from folder column names (e.g., "folder_CT" -> "CT") self.output_streams = [ - ("_").join(cols.split("_")[1:]) - for cols in self.df_combined.columns - if cols.split("_")[0] == "folder" + "_".join(column_name.split("_")[1:]) # Join all parts after "folder_" + for column_name in self.df_combined.columns + if column_name.startswith("folder_") ]
📜 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 (18)
config/mypy.ini(1 hunks)config/pytest.ini(1 hunks)config/ruff.toml(2 hunks)pixi.toml(2 hunks)src/imgtools/cli/__init__.py(2 hunks)src/imgtools/dicom/index/__init__.py(1 hunks)src/imgtools/dicom/index/__main__.py(3 hunks)src/imgtools/dicom/index/database/__init__.py(1 hunks)src/imgtools/dicom/index/models/models.py(4 hunks)src/imgtools/dicom/sort/dicomsorter.py(1 hunks)src/imgtools/dicom/sort/sort_method.py(1 hunks)src/imgtools/io/common.py(1 hunks)src/imgtools/io/loaders.py(1 hunks)src/imgtools/modules/datagraph.py(1 hunks)src/imgtools/modules/structureset.py(1 hunks)src/imgtools/ops/functional.py(2 hunks)src/imgtools/ops/ops.py(4 hunks)src/imgtools/utils/crawl.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/imgtools/dicom/index/init.py
- src/imgtools/dicom/index/database/init.py
🧰 Additional context used
🪛 Ruff (0.8.0)
src/imgtools/io/common.py
3-3: typing.Generator imported but unused
Remove unused import: typing.Generator
(F401)
🔇 Additional comments (18)
src/imgtools/modules/structureset.py (1)
48-49: LGTM! Warning suppression implementation is clean and effective
The conditional warning implementation preserves error context and includes the file path for better debugging.
src/imgtools/io/loaders.py (1)
80-81: Verify warning suppression propagation in the codebase
Let's ensure the warning suppression parameter is properly propagated through all RTSTRUCT loading paths.
✅ Verification successful
Let me analyze the propagation of the warning suppression parameter by checking the implementation of StructureSet.from_dicom_rtstruct.
Warning suppression parameter is properly propagated
The warning suppression parameter is correctly propagated through the entire call chain:
read_dicom_rtstruct(path, suppress_warnings=False)accepts the parameter and passes it directly toStructureSet.from_dicom_rtstruct(path, suppress_warnings=suppress_warnings)which uses it to control warning logging when ROI points cannot be retrieved
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to read_dicom_rtstruct and from_dicom_rtstruct
echo "Checking direct calls to read_dicom_rtstruct:"
rg "read_dicom_rtstruct\(" -A 2
echo -e "\nChecking calls through read_dicom_auto:"
rg "read_dicom_auto\(" -A 5
echo -e "\nChecking for any other RTSTRUCT loading patterns:"
rg "RTSTRUCT" -A 5
Length of output: 40571
Script:
#!/bin/bash
# Search for the implementation of from_dicom_rtstruct method
ast-grep --pattern 'def from_dicom_rtstruct($$$)'
# Also search for any other potential warning suppression handling
rg "suppress_warning" -A 3
Length of output: 1132
src/imgtools/io/common.py (1)
Line range hint 1-24: Verify the impact of removing find_dicom_paths
The removal of find_dicom_paths function could affect DICOM file discovery functionality. Let's verify if this functionality has been moved elsewhere or if it's still needed.
✅ Verification successful
Based on the search results, I can now generate the final response:
The removal of find_dicom_paths is safe as alternative DICOM discovery functionality exists
The codebase has robust DICOM file discovery functionality through:
find_dicoms()insrc/imgtools/dicom/utils.py- A comprehensive function that handles DICOM file discovery with support for recursion, header validation, and filteringcrawl_one()insrc/imgtools/utils/crawl.py- Provides DICOM file discovery within folder hierarchiesread_dicom_auto()insrc/imgtools/io/loaders.py- Handles both single DICOM files and directories containing DICOM files
The removed find_dicom_paths function's functionality has been effectively replaced by these implementations, particularly the more feature-rich find_dicoms().
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to find_dicom_paths
rg "find_dicom_paths"
# Search for alternative DICOM path discovery implementations
ast-grep --pattern 'def $func($$$) {
$$$
is_dicom($$$)
$$$
}'
Length of output: 91
Script:
#!/bin/bash
# Search for any references to find_dicom_paths in git history
git log -p | rg "find_dicom_paths"
# Search for functions that might handle DICOM file discovery
rg -A 5 "\.dcm|is_dicom|pydicom"
# Look for Python files that might contain DICOM-related functionality
fd -e py | xargs rg "dicom" -l
Length of output: 41170
Script:
#!/bin/bash
# Search for alternative DICOM file discovery implementations
rg -A 10 "def find_dicoms|def crawl_one|def read_dicom"
# Look for imports or usage of find_dicom_paths in the codebase
rg "find_dicom_paths"
# Check if there are any other functions that handle DICOM file discovery
ast-grep --pattern 'def $func($$$) {
$$$
glob.glob($$$dcm$$$)
$$$
}'
Length of output: 4568
🧰 Tools
🪛 Ruff (0.8.0)
1-1: os imported but unused
Remove unused import: os
(F401)
2-2: pathlib imported but unused
Remove unused import: pathlib
(F401)
3-3: typing.Generator imported but unused
Remove unused import: typing.Generator
(F401)
5-5: pydicom.misc.is_dicom imported but unused
Remove unused import: pydicom.misc.is_dicom
(F401)
src/imgtools/dicom/sort/sort_method.py (1)
108-108: LGTM! Type annotation enhancement improves flexibility
The union type FileAction | str allows for more flexible usage while maintaining type safety through validation.
src/imgtools/dicom/sort/dicomsorter.py (2)
98-98: LGTM! Consistent type enhancement
The type annotation change aligns with the updates in sort_method.py, maintaining consistency across the codebase.
Line range hint 128-150: LGTM! Robust parallel processing implementation
The implementation of parallel processing with proper progress tracking and error handling is well done.
config/pytest.ini (1)
28-29: Verify the coverage configuration file extension
The path config/coverage.toml is unusual as coverage.py typically uses .ini, .cfg, or .coveragerc file extensions.
✅ Verification successful
Let me verify the content of the TOML file to ensure it's a valid coverage configuration file.
The .toml extension for coverage configuration is valid and correctly used
The coverage configuration file uses the TOML format which is supported by coverage.py through its integration with pyproject.toml standards. The file contains valid coverage configuration sections ([tool.coverage.run] and [tool.coverage.report]) following the expected schema. The usage of TOML format is actually a modern and recommended approach for Python tools configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the coverage config file exists with the specified extension
if [ -f "config/coverage.toml" ]; then
echo "Coverage config file exists with .toml extension"
else
echo "Coverage config file not found with .toml extension"
# Check for other common extensions
for ext in ini cfg coveragerc; do
if [ -f "config/coverage.$ext" ]; then
echo "Found coverage config with .$ext extension"
fi
done
fi
Length of output: 140
Script:
#!/bin/bash
# Check the content of the coverage.toml file
if [ -f "config/coverage.toml" ]; then
echo "Content of coverage.toml:"
cat "config/coverage.toml"
# Also check if coverage.py is listed in requirements
echo -e "\nChecking for coverage.py in requirements:"
rg "coverage" "*requirements*.txt" || true
fi
Length of output: 439
config/mypy.ini (1)
13-14: Verify Python version compatibility
The mypy configuration specifies Python 3.10, but the project's default Python version in pixi.toml is 3.12. Consider updating this to match the project's default Python version.
pixi.toml (1)
63-65: Verify test configuration changes
The test command has been simplified to use pytest.ini. Ensure that all necessary pytest options are properly configured in the referenced config file.
✅ Verification successful
Test configuration is properly set up and comprehensive
The pytest.ini configuration file contains all necessary settings:
- Coverage reporting with multiple formats (terminal, XML, HTML)
- Proper test discovery paths and patterns
- Comprehensive test markers for categorization
- Verbose output and debugging options
- Coverage configuration linked correctly to coverage.toml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if pytest.ini exists and contains necessary configurations
if [ -f config/pytest.ini ]; then
echo "pytest.ini configurations:"
cat config/pytest.ini
else
echo "Warning: config/pytest.ini not found"
fi
Length of output: 1910
src/imgtools/dicom/index/models/models.py (2)
294-298: LGTM! Improved code readability.
The refactoring of nested attribute access into a more readable format with proper indentation and parentheses makes the code easier to understand while maintaining the same functionality.
Also applies to: 301-301
Line range hint 304-307: LGTM! Robust error handling implementation.
The error handling follows best practices by:
- Logging errors with context (file path)
- Preserving the original exception stack trace
src/imgtools/utils/crawl.py (1)
184-184: LGTM! Enhanced progress reporting.
The addition of a descriptive label and cleanup behavior to the progress bar improves user feedback during the crawling process.
src/imgtools/ops/functional.py (3)
2-7: LGTM! Enhanced type annotations and import organization.
The changes improve code quality by:
- Adding comprehensive type hints
- Organizing imports logically
25-65: LGTM! Improved docstring formatting and consistency.
The docstring improvements follow NumPy style guidelines and maintain consistent formatting throughout the file, enhancing code documentation.
Also applies to: 118-149, 178-212
67-71: LGTM! Enhanced error handling with proper exception chaining.
The error handling improvements:
- Use proper exception chaining with 'raise from'
- Provide clear error messages with available options
- Preserve the original exception context
src/imgtools/modules/datagraph.py (1)
430-433: Improved regex pattern construction!
The changes enhance the regex pattern by:
- Using non-capturing groups for better performance
- Explicitly setting
regex=Truefor clarity - Using f-string for better readability
src/imgtools/ops/ops.py (2)
122-122: Good use of class-level configuration!
Defining imgtools_dir as a class variable follows the DRY principle and makes the configuration more maintainable.
132-134: Excellent use of pathlib.Path!
Using pathlib.Path for path handling is a better practice as it:
- Provides cross-platform compatibility
- Offers cleaner path manipulation with the / operator
- Makes the code more maintainable
…ons version 24.10.0 for improved compatibility
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 43.46% 43.90% +0.44%
==========================================
Files 48 48
Lines 3640 3603 -37
==========================================
Hits 1582 1582
+ Misses 2058 2021 -37 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style