Adding Temporary Directory Routine - #9029
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the exported Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
…ply.github.com> I, Eric Kerfoot <17726042+ericspod@users.noreply.github.com>, hereby add my Signed-off-by to this commit: a95e44e I, Eric Kerfoot <17726042+ericspod@users.noreply.github.com>, hereby add my Signed-off-by to this commit: d9cd2d1 I, Eric Kerfoot <17726042+ericspod@users.noreply.github.com>, hereby add my Signed-off-by to this commit: 5e7e430 Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
![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 (2)
monai/apps/utils.py (2)
46-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort
__all__to satisfy RUF022.Ruff reports this export list is unsorted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/apps/utils.py` around lines 46 - 54, Sort the entries in the __all__ export list alphabetically to satisfy RUF022, preserving all existing exported symbols and their values.Source: Linters/SAST tools
436-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument filesystem exceptions.
Add a Google-style
Raisessection forOSErrorfrom temporary-directory creation oros.makedirs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/apps/utils.py` around lines 436 - 452, Update the create_temp_dir docstring to include a Google-style Raises section documenting OSError from temporary-directory creation and os.makedirs, without changing the function’s behavior or existing documentation.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/apps/utils.py`:
- Around line 461-462: Update the delete_on_finalise cleanup registration in
create_temp_dir to make shutil.rmtree ignore missing directories, ensuring
repeated callbacks are idempotent and exit cleanup does not raise
FileNotFoundError.
---
Nitpick comments:
In `@monai/apps/utils.py`:
- Around line 46-54: Sort the entries in the __all__ export list alphabetically
to satisfy RUF022, preserving all existing exported symbols and their values.
- Around line 436-452: Update the create_temp_dir docstring to include a
Google-style Raises section documenting OSError from temporary-directory
creation and os.makedirs, without changing the function’s behavior or existing
documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f394f467-bd08-4818-9576-ff8eeb031547
📒 Files selected for processing (3)
monai/apps/__init__.pymonai/apps/utils.pytests/apps/test_create_temp_dir.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/apps/test_create_temp_dir.py (1)
60-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a descriptive file-handle name.
Rename
otofile_handleor similar.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/apps/test_create_temp_dir.py` around lines 60 - 80, In test_finalisation, rename the file handle variable o to a descriptive name such as file_handle, and update its write call accordingly.Source: Path instructions
monai/apps/utils.py (2)
46-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__to satisfy Ruff.
create_temp_dirshould be placed alphabetically with the other exports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/apps/utils.py` around lines 46 - 54, Sort the export names in __all__ alphabetically, moving create_temp_dir into its correct position while preserving all existing exports.Source: Linters/SAST tools
437-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument propagated filesystem errors.
Add a Google-style
Raisessection forOSErrorfrom directory creation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/apps/utils.py` around lines 437 - 452, Update the docstring for the temporary-directory function to add a Google-style Raises section documenting that directory creation can raise OSError. Keep the existing arguments and return documentation unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@monai/apps/utils.py`:
- Around line 46-54: Sort the export names in __all__ alphabetically, moving
create_temp_dir into its correct position while preserving all existing exports.
- Around line 437-452: Update the docstring for the temporary-directory function
to add a Google-style Raises section documenting that directory creation can
raise OSError. Keep the existing arguments and return documentation unchanged.
In `@tests/apps/test_create_temp_dir.py`:
- Around line 60-80: In test_finalisation, rename the file handle variable o to
a descriptive name such as file_handle, and update its write call accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3aa5766-d56d-47e5-b8be-dea4293006fa
📒 Files selected for processing (3)
monai/apps/__init__.pymonai/apps/utils.pytests/apps/test_create_temp_dir.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/apps/init.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
|
|
||
|
|
||
| class TestCreateTempDir(unittest.TestCase): | ||
| def test_basic_use(self): |
There was a problem hiding this comment.
test_basic_use exercises the implicit auto-cleanup path (no directory, no MONAI_DATA_DIRECTORY), where delete_on_finalise gets forced to True in monai/apps/utils.py. That's the exact path every notebook migrating to this helper will hit, but this test never asserts atexit.register was called, so a regression here would go unnoticed. Could you patch atexit.register in this test too and assert it gets called when no directory or env var is provided?
| extractall(filepath=filename, output_dir=output_dir, file_type=file_type, has_base=has_base) | ||
|
|
||
|
|
||
| def create_temp_dir(directory: str | None = None, delete_on_finalise: bool = False) -> str: |
There was a problem hiding this comment.
Every other path-like parameter in this file (check_hash, download_and_extract, etc.) is typed PathLike, but this one is str | None. Minor consistency nit: directory: PathLike | None = None.
garciadias
left a comment
garciadias
left a comment
There was a problem hiding this comment.
create_temp_dir isn't added to docs/source/apps.rst alongside check_hash, download_url, extractall, and download_and_extract, so it won't show up in the built API docs. Consider adding an autofunction entry for it.
garciadias
left a comment
garciadias
left a comment
There was a problem hiding this comment.
Two Major findings around test rigor for the new create_temp_dir utility: the finalisation test's docstring is stale (still claims subprocess-based verification, but now just mocks atexit.register and calls it manually), and the more common implicit auto-cleanup path isn't asserted at all. The implementation itself looks correct and low-risk. Details in the inline comments.
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/apps/test_create_temp_dir.py (1)
80-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename and document the callback-capture helper.
At Line 80,
_registercapturesatexit.registerarguments; it does not register a callback. Rename it to a name such as_capture_registered_callback, add a Google-style docstring forfunc,args,kwargs, and theNonereturn, and update the patch at Line 83.As per path instructions, all Python definitions must have Google-style docstrings that describe variables, return values, and raised exceptions in the appropriate sections.
Proposed helper update
- def _register(func, /, *args, **kwargs): + def _capture_registered_callback(func, /, *args, **kwargs): + """Capture the callback registered by create_temp_dir. + + Args: + func: Cleanup callback. + args: Positional callback arguments. + kwargs: Keyword callback arguments. + + Returns: + None. + """ self.finaliser = (func, args, kwargs) - with patch("atexit.register", new=_register), tempfile.TemporaryDirectory() as temp_dir: + with patch("atexit.register", new=_capture_registered_callback), tempfile.TemporaryDirectory() as temp_dir:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/apps/test_create_temp_dir.py` around lines 80 - 83, Rename the local helper _register to _capture_registered_callback and update the atexit.register patch to use the new name. Add a Google-style docstring documenting func, args, kwargs, the None return value, and any applicable raised exceptions, while preserving its callback-argument capture behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/apps/test_create_temp_dir.py`:
- Around line 31-38: Update the test around create_temp_dir() to explicitly
invoke the captured shutil.rmtree cleanup callback immediately after asserting
the registration, ensuring the randomly created test_dir is removed even though
atexit.register is patched.
---
Nitpick comments:
In `@tests/apps/test_create_temp_dir.py`:
- Around line 80-83: Rename the local helper _register to
_capture_registered_callback and update the atexit.register patch to use the new
name. Add a Google-style docstring documenting func, args, kwargs, the None
return value, and any applicable raised exceptions, while preserving its
callback-argument capture behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ea471a9-7db8-42d5-bf68-23cc3a89e47d
📒 Files selected for processing (2)
monai/apps/utils.pytests/apps/test_create_temp_dir.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/apps/utils.py
2 participants
Description
This adds a temporary directory creation routine to replace the following cell in many tutorial notebooks:
This should be functionally equivalent, but can also add a finaliser function to delete the directory when the notebook closes. The notebooks can then be updated to reduce the amount of code.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.