test: add nnunet pipeline tests #395
Conversation
📝 WalkthroughWalkthroughThis update modifies the way label dictionaries are constructed in the nnUNet output logic, ensuring the background label is preserved during in-place updates. Additionally, a comprehensive integration test suite for the Changes
Suggested labels
Feedback: ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
|
Added testing with RADCURE, only issue is that nnunet is not available for py13. |
|
The issue with env is nnunet install on py313(osx-64) |
jjjermiah
left a comment
jjjermiah
left a comment
There was a problem hiding this comment.
looks great to me, to address the py313 issue, I recommend having a check in the cli entry point that lets the user know
Thanks! How do I deal with the dependency issue? I need to add nnunetv2 to test as a dependency, instead should I add it only to py311 and py312? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/integration/cli/nnunet_pipeline_cli.py (1)
96-96: Consider implementing pytest-snapshot for consistent output validation.As suggested in previous reviews, using pytest-snapshot would help ensure the generated outputs remain consistent across test runs and catch any unintended changes in the pipeline output format.
This would involve capturing and comparing the generated dataset structure, JSON configurations, and file contents against known good snapshots.
🧹 Nitpick comments (2)
tests/integration/cli/nnunet_pipeline_cli.py (2)
12-13: Fix class naming and docstring accuracy.The class name should follow PEP 8 conventions, and the docstring mentions the wrong CLI command.
-class TestnnUNetCLI: - """Integration tests for the autopipeline CLI command using collections from the test data.""" +class TestNnUNetCLI: + """Integration tests for the nnunet_pipeline CLI command using collections from the test data."""
63-70: Consider moving YAML file creation to a fixture.The temporary YAML file creation could be moved to a fixture for better test organization and reusability.
+ @pytest.fixture(scope="function") + def roi_yaml_file(self, tmp_path): + """Create a temporary ROI mapping YAML file.""" + roi_dict = { + "BRAINSTEM": "Brainstem", + "SPINALCORD": "SpinalCord", + "LARYNX": "Larynx", + } + roi_yaml_path = tmp_path / "roi_match.yaml" + with roi_yaml_path.open("w") as f: + yaml.dump(roi_dict, f) + return roi_yaml_pathThen use
roi_yaml_filefixture in the test method instead of creating the file inline.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pixi.lockis excluded by!**/*.lockand included by nonepixi.tomlis excluded by none and included by none
📒 Files selected for processing (2)
src/imgtools/io/nnunet_output.py(1 hunks)tests/integration/cli/nnunet_pipeline_cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/**/*.py`: Review the Python code for compliance with PEP 8 and PEP 257 (doc...
src/**/*.py: Review the Python code for compliance with PEP 8 and PEP 257 (docstring conventions). Ensure the following: - Variables and functions follow meaningful naming conventions. - Docstrings are present, accurate, and align with the implementation. - Code is efficient and avoids redundancy while adhering to DRY principles. - Consider suggestions to enhance readability and maintainability. - Highlight any potential performance issues, edge cases, or logical errors. - Ensure all imported libraries are used and necessary.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/imgtools/io/nnunet_output.py
`tests/**/*`: Review the test code written with Pytest. Confirm: - Tests cover a...
tests/**/*: Review the test code written with Pytest. Confirm: - Tests cover all critical functionality and edge cases. - Test descriptions clearly describe their purpose. - Pytest best practices are followed, such as proper use of fixtures. - Ensure the tests are isolated and do not have external dependencies (e.g., network calls). - Verify meaningful assertions and avoidance of redundant tests. - Test code adheres to PEP 8 style guidelines.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/integration/cli/nnunet_pipeline_cli.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: jjjermiah
PR: bhklab/med-imagetools#145
File: src/imgtools/utils/nnunet.py:0-0
Timestamp: 2024-11-29T21:18:38.153Z
Learning: Suggestions to modify the `save_json` function in `src/imgtools/utils/nnunet.py` to fix type annotations or add error handling are considered out of scope.
src/imgtools/io/nnunet_output.py (1)
Learnt from: jjjermiah
PR: bhklab/med-imagetools#145
File: src/imgtools/utils/nnunet.py:0-0
Timestamp: 2024-11-29T21:18:38.153Z
Learning: Suggestions to modify the `save_json` function in `src/imgtools/utils/nnunet.py` to fix type annotations or add error handling are considered out of scope.
tests/integration/cli/nnunet_pipeline_cli.py (3)
Learnt from: jjjermiah
PR: bhklab/med-imagetools#137
File: src/imgtools/dicom/sort/parser.py:132-137
Timestamp: 2024-11-21T21:03:45.548Z
Learning: Assertions are allowed for input validation throughout the project, including in `src/imgtools/dicom/sort/parser.py`.
Learnt from: jjjermiah
PR: bhklab/med-imagetools#137
File: src/imgtools/dicom/sort/utils.py:66-67
Timestamp: 2024-11-21T16:24:04.091Z
Learning: In the `src/imgtools/dicom/sort/utils.py` file and throughout the codebase, assertions are preferred for input validation instead of explicit type checks or raising exceptions.
Learnt from: jjjermiah
PR: bhklab/med-imagetools#137
File: src/imgtools/dicom/sort/utils.py:168-169
Timestamp: 2024-11-21T16:24:13.275Z
Learning: In `src/imgtools/dicom/sort/utils.py`, it's acceptable to use `assert` statements for input validation in the `read_tags` function.
🧬 Code Graph Analysis (1)
src/imgtools/io/nnunet_output.py (1)
src/imgtools/coretypes/base_masks.py (1)
roi_keys(161-163)
🔇 Additional comments (1)
src/imgtools/io/nnunet_output.py (1)
301-303: LGTM: Preserves background label correctly.The change from reassigning to updating the labels dictionary in-place is a good fix. This ensures the background label (0) is preserved while adding ROI keys with proper incremental indexing, which aligns with nnUNet's expected label format.
| nnunet_result = subprocess.run([ | ||
| "nnUNetv2_extract_fingerprint", | ||
| "-d", "1", | ||
| "--verify_dataset_integrity", | ||
| ], | ||
| env=env, | ||
| stdout=subprocess.PIPE | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve subprocess call robustness.
The subprocess call lacks timeout and proper error handling, which could cause tests to hang or hide important error information.
nnunet_result = subprocess.run([
"nnUNetv2_extract_fingerprint",
"-d", "1",
"--verify_dataset_integrity",
],
env=env,
- stdout=subprocess.PIPE
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE,
+ timeout=300, # 5 minute timeout
+ text=True
)Also consider logging the stderr output when the command fails for better debugging:
- assert nnunet_result.returncode == 0, "nnUNetv2_extract_fingerprint failed"
+ assert nnunet_result.returncode == 0, f"nnUNetv2_extract_fingerprint failed: {nnunet_result.stderr}"📝 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.
| nnunet_result = subprocess.run([ | |
| "nnUNetv2_extract_fingerprint", | |
| "-d", "1", | |
| "--verify_dataset_integrity", | |
| ], | |
| env=env, | |
| stdout=subprocess.PIPE | |
| ) | |
| nnunet_result = subprocess.run([ | |
| "nnUNetv2_extract_fingerprint", | |
| "-d", "1", | |
| "--verify_dataset_integrity", | |
| ], | |
| env=env, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.PIPE, | |
| timeout=300, # 5 minute timeout | |
| text=True | |
| ) | |
| assert nnunet_result.returncode == 0, f"nnUNetv2_extract_fingerprint failed: {nnunet_result.stderr}" |
🤖 Prompt for AI Agents
In tests/integration/cli/nnunet_pipeline_cli.py around lines 87 to 94, the
subprocess.run call lacks a timeout and error handling, risking hangs and
obscured errors. Add a timeout parameter to prevent indefinite blocking, use
check=True to raise an exception on failure, and catch
subprocess.CalledProcessError to log stderr output for better debugging.
Summary by CodeRabbit