-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adding Temporary Directory Routine #9029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
a95e44e
d9cd2d1
5e7e430
101064f
8592172
a96b14e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # Copyright (c) MONAI Consortium | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| import tempfile | ||
| import unittest | ||
| from unittest.mock import patch | ||
|
|
||
| from monai.apps import create_temp_dir | ||
| from monai.utils import MONAIEnvVars | ||
|
|
||
| MONAI_DATA_DIRECTORY = "MONAI_DATA_DIRECTORY" | ||
|
|
||
|
|
||
| class TestCreateTempDir(unittest.TestCase): | ||
| def test_basic_use(self): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| """Test basic usage which should create a new random temporary directory.""" | ||
| try: | ||
| data_dir = os.environ.pop(MONAI_DATA_DIRECTORY, None) # ignore the environment variable if present | ||
|
|
||
| test_dir = create_temp_dir() | ||
|
|
||
| self.assertTrue(os.path.isdir(test_dir)) | ||
|
|
||
| finally: | ||
| if data_dir is not None: | ||
| os.environ[MONAI_DATA_DIRECTORY] = data_dir | ||
|
|
||
| def test_data_dir(self): | ||
| """Test using a mocked MONAI_DATA_DIRECTORY, which should be returned by the function.""" | ||
| with patch("monai.utils.MONAIEnvVars.data_dir") as data_dir, tempfile.TemporaryDirectory() as fake_data_dir: | ||
| data_dir.return_value = fake_data_dir | ||
|
|
||
| self.assertEqual(fake_data_dir, MONAIEnvVars.data_dir()) | ||
|
|
||
| test_dir = create_temp_dir() | ||
|
|
||
| self.assertTrue(os.path.isdir(test_dir)) | ||
| self.assertEqual(test_dir, fake_data_dir) | ||
|
|
||
| def test_given_dir(self): | ||
| """Test giving a directory to the function, ensuring it creates the directory.""" | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| selected_dir = f"{temp_dir}/test_inner_dir" | ||
| test_dir = create_temp_dir(selected_dir) | ||
|
|
||
| self.assertTrue(os.path.isdir(selected_dir)) | ||
| self.assertEqual(test_dir, selected_dir) | ||
|
|
||
| def test_finalisation(self): | ||
| """Test the temporary directory is deleted by finalisation using a subprocess.""" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This docstring still says "using a subprocess," but the earlier subprocess-based check was replaced with a monkeypatched atexit.register that the test then calls manually (see lines 64-65, 77-78). That proves shutil.rmtree works, but not that create_temp_dir is actually wired up correctly with the real atexit module or that cleanup fires on process exit. Could you fix the docstring to describe what's actually tested, and/or restore a lightweight subprocess smoke test (guarded with skip_if_quick like before) so there's still one real end-to-end check? |
||
| self.finaliser = None | ||
|
|
||
| def _register(func, /, *args, **kwargs): | ||
| self.finaliser = (func, args, kwargs) | ||
|
|
||
| with patch("atexit.register", new=_register), tempfile.TemporaryDirectory() as temp_dir: | ||
| selected_dir = f"{temp_dir}/test_inner_dir" | ||
| test_dir = create_temp_dir(selected_dir, True) | ||
|
|
||
| self.assertTrue(os.path.isdir(selected_dir)) | ||
| self.assertIsNotNone(self.finaliser) | ||
|
|
||
| with open(test_dir + "/test_file", "w") as o: | ||
| o.write("Test file data") | ||
|
|
||
| func, args, kwargs = self.finaliser | ||
| func(*args, **kwargs) | ||
|
|
||
| self.assertFalse(os.path.exists(selected_dir)) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.