tests: allow for parallel test execution #180
Conversation
|
Warning Rate limit exceeded@jjjermiah has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 42 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 selected for processing (2)
WalkthroughThis pull request introduces several configuration and code refinements across multiple files. The changes focus on improving test configuration in Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Nitpick comments (1)
tests/dicom/sort/test_sort_utils.py (1)
54-54: Use context manager for temporary file creation.Using a context manager ensures proper resource cleanup.
- temp_file = Path(tempfile.NamedTemporaryFile(suffix=".dcm", delete=False).name) + with tempfile.NamedTemporaryFile(suffix=".dcm", delete=False) as tf: + temp_file = Path(tf.name)🧰 Tools
🪛 Ruff (0.8.2)
54-54: Use a context manager for opening files
(SIM115)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/pytest.ini(2 hunks)src/imgtools/ops/__init__.py(1 hunks)tests/dicom/sort/test_sort_utils.py(2 hunks)tests/test_ops.py(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI-CD
config/pytest.ini
[error] Test execution was canceled before completion. The pipeline operation was terminated unexpectedly.
🪛 Ruff (0.8.2)
tests/test_ops.py
3-3: shutil imported but unused
Remove unused import: shutil
(F401)
tests/dicom/sort/test_sort_utils.py
54-54: Use a context manager for opening files
(SIM115)
🔇 Additional comments (5)
src/imgtools/ops/__init__.py (1)
1-41: Well-structured module interface definition!The explicit imports and
__all__list definition improve code clarity and provide better control over the module's public interface. This change follows Python best practices.tests/test_ops.py (1)
26-32: Great use of pytest's tmp_path fixture!The replacement of the custom
output_pathfixture with pytest's built-intmp_pathfixture is an excellent change that:
- Improves test isolation
- Enables parallel test execution
- Eliminates the need for manual cleanup
Also applies to: 48-53
config/pytest.ini (2)
30-38: Well-configured parallel test execution!The addition of:
--cov-appendensures proper coverage data collection during parallel execution--numprocesses=autowith--maxprocesses=8provides a good balance between performance and resource usageThe pipeline failure might be related to resource constraints. Consider:
✅ Verification successful
Parallel test configuration is appropriate for CI environment
The current configuration with
--numprocesses=autoand--maxprocesses=8is well-balanced:
- Auto-detection ensures optimal resource utilization on CI runners
- Max 8 processes prevents excessive parallelization
- Coverage append is correctly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check available resources in the CI environment nproc free -hLength of output: 208
Script:
#!/bin/bash # Search for CI configuration files fd -e yml -e yaml -e json . .github/workflows # Look for any resource-related documentation rg -i "resource|memory|cpu|processor" README.md docs/ 2>/dev/null # Check for any test-related environment variables in CI configs rg -i "test.*env|env.*test|resource|memory|cpu|processor" .github/workflows/Length of output: 413
🧰 Tools
🪛 GitHub Actions: CI-CD
[error] Test execution was canceled before completion. The pipeline operation was terminated unexpectedly.
17-17: Verify coverage target configuration.The removal of the explicit coverage target (
--cov=imgtools→--cov) requires the target to be specified elsewhere. Please ensure it's properly configured inconfig/coverage.toml.✅ Verification successful
Coverage configuration is properly set up ✓
The removal of
--cov=imgtoolsis safe as the coverage configuration is correctly defined incoverage.toml. The package will be automatically discovered since it follows the src layout pattern (src/imgtools).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if coverage target is specified in coverage.toml cat config/coverage.toml | grep -A 5 '\[paths\]'Length of output: 50
Script:
#!/bin/bash # Find coverage configuration files fd -e toml -e cfg -e ini -e coveragerc --max-depth 2 # Check pyproject.toml for coverage settings echo "=== pyproject.toml coverage settings ===" cat pyproject.toml 2>/dev/null | grep -A 10 '\[tool\.coverage' || true # Check setup.cfg for coverage settings echo -e "\n=== setup.cfg coverage settings ===" cat setup.cfg 2>/dev/null | grep -A 10 '\[coverage:' || true # Check .coveragerc echo -e "\n=== .coveragerc settings ===" cat .coveragerc 2>/dev/null || trueLength of output: 547
Script:
#!/bin/bash # Display full content of coverage.toml echo "=== Full content of coverage.toml ===" cat config/coverage.toml || true # Search for coverage target patterns in all config files echo -e "\n=== Coverage patterns in config files ===" rg -i "source|coverage.*path|imgtools" config/coverage.toml config/pytest.ini pyproject.toml 2>/dev/null || trueLength of output: 858
🧰 Tools
🪛 GitHub Actions: CI-CD
[error] Test execution was canceled before completion. The pipeline operation was terminated unexpectedly.
tests/dicom/sort/test_sort_utils.py (1)
144-150: Good use of tmp_path fixture!The addition of the
tmp_pathfixture improves test isolation and enables parallel execution.
| @@ -1,5 +1,6 @@ | |||
| import copy | |||
| import pathlib | |||
| import shutil | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import.
The shutil import is no longer needed after removing the manual cleanup code.
-import shutil🧰 Tools
🪛 Ruff (0.8.2)
3-3: shutil imported but unused
Remove unused import: shutil
(F401)
…onftest.py and test_modalities.py
…p for serial execution
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pixi.toml (1)
67-69: Consider improvements to the clean_tests task implementation.While the task serves a good purpose for cleaning up test artifacts, there are several improvements to consider:
- The task should be a dependency for the test task to ensure a clean state before testing:
[feature.test.tasks.test] cmd = "pytest -c config/pytest.ini --rootdir ." inputs = ["src", "tests", "config/pytest.ini", "config/coverage.toml"] +depends-on = ["clean_tests"] description = "Run pytest (make sure to run in root directory)"
- Consider using pytest's built-in temporary directory management (
tmp_pathfixture) instead of manually cleaning up./tests/temp, as this would better support parallel test execution.tests/conftest.py (1)
42-45: Consider consolidating repetitive path construction logic.The path construction for imgtools files follows a pattern that could be made more maintainable.
+ def _make_imgtools_path(base_path, suffix=""): + return pathlib.Path(base_path, f"imgtools_{dataset_name}{suffix}").as_posix() + - crawl_path = pathlib.Path(imgtools_path, f"imgtools_{dataset_name}.csv").as_posix() - edge_path = pathlib.Path( - imgtools_path, f"imgtools_{dataset_name}_edges.csv" - ).as_posix() + crawl_path = _make_imgtools_path(imgtools_path, ".csv") + edge_path = _make_imgtools_path(imgtools_path, "_edges.csv")tests/test_modalities.py (3)
16-36: Consider making the modality paths more configurable.While the fixture effectively organizes modality paths, the hard-coded paths could be made more maintainable and configurable.
Consider using a configuration file or environment variables for these paths:
@pytest.fixture(scope="module") def modalities_path(curr_path, dataset_path): config = { "CT": "3.000000-Merged-06362", "RTSTRUCT": "Pinnacle POI-41418", "RTDOSE": "11376", "PT": "532790.000000-LOR-RAMLA-44600" } qc_path = pathlib.Path(curr_path, "data", "Head-Neck-PET-CT", "HN-CHUS-052") assert qc_path.exists(), "Dataset not found" return { modality: str(pathlib.Path(qc_path, parent_dir, file_name)) for modality, file_name in config.items() for parent_dir in [next(p for p in qc_path.glob("*") if file_name in str(p))] }
39-39: Consider adding descriptive test IDs to the parameterization.Adding test IDs would make test output more readable and help identify which modality is being tested.
-@pytest.mark.parametrize("modalities", ["CT", "RTSTRUCT", "RTDOSE", "PT"]) +@pytest.mark.parametrize("modalities", ["CT", "RTSTRUCT", "RTDOSE", "PT"], + ids=lambda x: f"testing_{x}_modality")
68-70: Consider parameterizing ROI names for better test flexibility.The hard-coded ROI names make the test less flexible and harder to maintain.
+@pytest.fixture(params=[ + {"names": ["GTV.?", "LARYNX"], "expected_regions": 2}, + {"names": ["GTV.?"], "expected_regions": 1}, +]) +def roi_config(request): + return request.param - make_binary_mask = StructureSetToSegmentation( - roi_names=["GTV.?", "LARYNX"], continuous=False - ) + make_binary_mask = StructureSetToSegmentation( + roi_names=roi_config["names"], continuous=False + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
config/coverage.toml(1 hunks)pixi.toml(1 hunks)tests/conftest.py(1 hunks)tests/test_components.py(2 hunks)tests/test_modalities.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_components.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Unit-Tests (macos-14, py312)
- GitHub Check: Unit-Tests (macos-14, py311)
- GitHub Check: Unit-Tests (macos-14, py310)
- GitHub Check: Unit-Tests (macos-latest, py311)
- GitHub Check: Unit-Tests (macos-latest, py310)
- GitHub Check: Unit-Tests (ubuntu-latest, py312)
- GitHub Check: Unit-Tests (ubuntu-latest, py311)
- GitHub Check: Unit-Tests (ubuntu-latest, py310)
🔇 Additional comments (5)
config/coverage.toml (2)
6-6: LGTM! Good practice to exclude test files from coverage reports.The addition of
"tests/**/*.py"to the omit list is correct. This ensures coverage reports focus on the actual codebase rather than test files, which is the standard practice.
9-9: LGTM!pixi.toml (1)
68-68: Verify the impact of cleanup during parallel test execution.The
rm -rfcommand on shared directories (.pytest_cache,./data,./tests/temp) could potentially interfere with parallel test execution if tests are running simultaneously in different processes.Let's check if any tests are explicitly using these paths:
✅ Verification successful
The cleanup task is safe for parallel test execution
The codebase follows best practices for test directory management:
- Uses pytest's process-safe temporary directory fixtures (tmp_path)
- Leverages Python's TemporaryDirectory for automatic cleanup
- Custom fixtures properly scope test data directories
The directories in the cleanup task (.pytest_cache, ./data, ./tests/temp) are not directly referenced in tests and can be safely removed between test runs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for explicit usage of these paths in test files echo "Checking for explicit path usage in tests..." rg -g '!pixi.toml' "pytest_cache|/data/|/tests/temp" ./testsLength of output: 161
Script:
#!/bin/bash # Search for pytest fixtures and temporary path handling echo "Checking for pytest fixtures and tmp paths..." rg -g "tests/**" "(fixture|tmp_path|tmpdir|tempfile|mkdir|os\.path)" -A 2 # Search for test setup/teardown patterns echo -e "\nChecking for test setup/teardown..." ast-grep --pattern 'def setup$$$' ast-grep --pattern 'def teardown$$$'Length of output: 9642
tests/conftest.py (1)
16-16: LGTM! Good improvement for test parallelization.Changing the fixture scope from 'session' to 'package' is a positive change that better supports parallel test execution by creating separate fixture instances for each test package.
tests/test_modalities.py (1)
38-38: LGTM! Good use of xdist grouping for test coordination.The
@pytest.mark.xdist_group("serial")marker correctly ensures that these tests run serially when needed, which is important for resource management.
|
|
||
| output_path = pathlib.Path(curr_path, 'tests', 'temp').as_posix() | ||
| output_path = pathlib.Path(curr_path, "tests", "temp").as_posix() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use pytest's built-in temporary directory fixtures for better test isolation.
Instead of using a fixed 'temp' directory, consider using pytest's tmp_path or tmp_path_factory fixtures. This would provide better isolation for parallel test execution and automatic cleanup.
Example refactor:
- output_path = pathlib.Path(curr_path, "tests", "temp").as_posix()
+ @pytest.fixture(scope="package")
+ def output_path(tmp_path_factory):
+ return tmp_path_factory.mktemp("output").as_posix()Committable suggestion skipped: line range outside the PR's diff.
tests/conftest.py
Outdated
| zipfile.extractall(quebec_path) | ||
| os.remove(quebec_zip_path) | ||
| # os.remove(quebec_zip_path) |
There was a problem hiding this comment.
💡 Codebase verification
Remove the commented cleanup code for better code maintainability
The commented cleanup (os.remove(quebec_zip_path)) is not critical since:
- The dataset fixture has package scope, meaning data persistence is intentional
- The zip file is downloaded only once per test session
- The disk space impact is minimal
Simply remove the commented line to maintain code cleanliness.
🔗 Analysis chain
Consider removing or justifying the commented cleanup code.
The commented-out cleanup code (os.remove(quebec_zip_path)) might lead to unnecessary disk space usage, especially in CI environments. If there's a specific reason for keeping the zip file, please document it; otherwise, consider removing the commented line.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this cleanup is handled elsewhere in the codebase
rg "quebec.*\.zip.*remove" --type py
Length of output: 38
Script:
#!/bin/bash
# Get broader context around the Quebec zip handling
echo "=== Quebec zip file references ==="
rg "quebec.*\.zip" --type py -B 2 -A 2
echo -e "\n=== General zip cleanup patterns in conftest ==="
rg "\.zip.*remove|remove.*\.zip" tests/conftest.py
echo -e "\n=== Full context of the fixture ==="
cat tests/conftest.py
Length of output: 3062
…ove test parameterization
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)config/pytest.ini(2 hunks)tests/conftest.py(1 hunks)tests/test_modalities.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI-CD
tests/conftest.py
[error] 7-7: Missing required dependency: Module 'filelock' is not installed but is imported in the file
🔇 Additional comments (7)
config/pytest.ini (2)
17-17: Coverage configuration looks good for parallel execution.The removal of hardcoded coverage target and addition of
--cov-appendensures proper coverage reporting when tests run in parallel.Also applies to: 30-32
33-40: Well-configured parallel execution settings.The parallel execution configuration is well thought out:
--numprocesses=autowith--maxprocesses=8provides good scalability--dist=loadgroupensures proper test isolationtests/conftest.py (2)
17-52: Well-implemented dataset preparation with proper locking.The implementation of
prepare_datasetwith FileLock is excellent for parallel test execution:
- Prevents concurrent downloads when using xdist
- Properly cleans up lock file
- Uses appropriate logging
55-70: Consider using pytest's tmp_path fixture.The previous review comment about using pytest's built-in temporary directory fixtures still applies. This would improve test isolation in parallel execution.
tests/test_modalities.py (2)
15-39: Well-structured modalities fixture with proper assertions.The modalities_path fixture is well implemented:
- Appropriate scope and location in test file
- Good path existence checks
- Clear path organization
44-44: Improved test parameterization.The change from 'modalities' to 'imaging_modality' makes the test intention clearer.
Also applies to: 46-46
.gitignore (1)
232-232: Appropriate addition of lock files to .gitignore.Adding
*.lockpattern is necessary to prevent committing temporary lock files used in parallel test execution.
…ust dataset lock path in tests
… and modifying test parameters
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/conftest.py (2)
17-52: LGTM! Well-implemented parallel execution handling.The fixture implementation is robust:
- Uses FileLock to prevent concurrent downloads
- Includes proper cleanup
- Has appropriate logging
Consider moving the dataset URL to a configuration file.
The hard-coded URL at line 37 should be moved to a configuration file for better maintainability.
+# config/test_config.py +TEST_DATASET_URL = "https://github.com/bhklab/tcia_samples/blob/main/Head-Neck-PET-CT.zip?raw=true" - quebec_data_url = "https://github.com/bhklab/tcia_samples/blob/main/Head-Neck-PET-CT.zip?raw=true" + quebec_data_url = TEST_DATASET_URL
35-44: Enhance error handling for dataset download.The dataset download process could benefit from improved error handling:
- Add timeout for download
- Handle network errors explicitly
- Verify file integrity after download
+from urllib.error import URLError +import hashlib +EXPECTED_CHECKSUM = "..." # Add the expected SHA256 checksum - request.urlretrieve(quebec_data_url, quebec_zip_path) + try: + # Add 5-minute timeout + request.urlretrieve(quebec_data_url, quebec_zip_path, timeout=300) + + # Verify checksum + with open(quebec_zip_path, "rb") as f: + if hashlib.sha256(f.read()).hexdigest() != EXPECTED_CHECKSUM: + raise ValueError("Downloaded file has incorrect checksum") + + with ZipFile(quebec_zip_path, "r") as zipfile: + zipfile.extractall(quebec_path) + except (URLError, TimeoutError) as e: + logger.error(f"Failed to download dataset: {e}") + raise + except ValueError as e: + logger.error(f"File verification failed: {e}") + raise + finally: + if os.path.exists(quebec_zip_path): + os.remove(quebec_zip_path) - with ZipFile(quebec_zip_path, "r") as zipfile: - zipfile.extractall(quebec_path) - os.remove(quebec_zip_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pixi.toml(2 hunks)tests/conftest.py(1 hunks)tests/test_components.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pixi.toml
- tests/test_components.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Unit-Tests (macos-14, py312)
- GitHub Check: Unit-Tests (macos-14, py311)
- GitHub Check: Unit-Tests (macos-14, py310)
- GitHub Check: Unit-Tests (macos-latest, py311)
- GitHub Check: Unit-Tests (ubuntu-latest, py312)
- GitHub Check: Unit-Tests (ubuntu-latest, py311)
- GitHub Check: Unit-Tests (ubuntu-latest, py310)
🔇 Additional comments (3)
tests/conftest.py (3)
7-7: Add filelock to project dependencies.The filelock module is required for parallel test execution but is not installed.
55-70: Use pytest's built-in temporary directory fixtures for better test isolation.Instead of using a fixed 'temp' directory, consider using pytest's
tmp_pathortmp_path_factoryfixtures for better isolation during parallel test execution.
12-14: LGTM! Verify the package scope impact.The changes look good:
- Package scope is appropriate for parallel execution
- Path resolution is more direct using
cwd()✅ Verification successful
✅ Package scope change is safe
The
curr_pathfixture is only used within the root test package and not in any sub-packages, making the scope change from 'session' to 'package' safe and appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other fixtures depending on curr_path that might be affected by the scope change rg "@pytest\.fixture.*\bcurr_path\b" tests/Length of output: 45
Script:
#!/bin/bash # Find all usages of curr_path in test files rg "\bcurr_path\b" tests/ -l # List test directory structure to understand package organization fd . tests/ --type dLength of output: 182
|
addressed in #202 |
Explicitly define exported members in the module to improve clarity. Update test configurations for better coverage reporting and parallel execution. Refactor tests to utilize temporary paths for output handling.
Summary by CodeRabbit
Configuration
Module Improvements
Testing Enhancements
tmp_pathfor better test isolation.Code Style
Chores
.lockfiles to.gitignoreto prevent tracking.