-
Notifications
You must be signed in to change notification settings - Fork 261
FEAT - Pruning skrub-data folder to remove old data ops execution reports #1657
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
Changes from 9 commits
f560b23
4d08cc2
fa955f8
7cf9bc3
3a76e20
f612dc4
3e4a145
9b14f78
01be627
7474b72
fe088af
4aeb179
a0987e6
dadb13b
282128e
d135246
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 |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| import enum | ||
| import os | ||
| import shutil | ||
| import time | ||
| import traceback | ||
| import warnings | ||
|
|
||
| from joblib.externals import cloudpickle | ||
|
|
||
|
|
@@ -62,3 +66,25 @@ def format_exception(e): | |
| def format_exception_only(e): | ||
| """compatibility for python < 3.10""" | ||
| return traceback.format_exception_only(type(e), e) | ||
|
|
||
|
|
||
| def prune_folder(path: str): | ||
| if not os.path.exists(path): | ||
| return | ||
| time_threshold = time.time() - 7 * 24 * 3600 # 7 days ago | ||
|
|
||
| folders = ( | ||
| (f, os.path.getmtime(os.path.join(path, f))) | ||
|
Contributor
Author
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. All tests are passing except the codecov (not enough test coverage). In the test cov report these lines (line 73-77) are not covered. Maybe because assignment of the variable is only done on line 73. |
||
| for f in os.listdir(path) | ||
| if os.path.isdir(os.path.join(path, f)) and f.startswith("full_data_op_report_") | ||
| ) | ||
| for dir_path, dir_time in folders: | ||
| try: | ||
| if dir_time < time_threshold: | ||
|
Member
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. i guess the test could be outside the try block |
||
| shutil.rmtree(os.path.join(path, dir_path)) | ||
|
Member
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. I wonder if we should have an even stricter check of the name (eg check with a regex there is a timestamp) just to be extra sure we don't erase something we shouldn't ... but I guess "full_data_op_report_" is already very specific 🤔 |
||
| except Exception as e: | ||
| warnings.warn( | ||
| "Skrub wants to delete an old folder in the skrub data folder: " | ||
| f"Could not delete {dir_path}:\n" | ||
| + "".join(format_exception_only(e)) | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import os | ||
| import tempfile | ||
| import time | ||
|
|
||
| import pytest | ||
|
|
||
| from skrub._data_ops import _utils | ||
|
|
||
|
|
||
| def test_prune_folder_with_standard_name_dirs(): | ||
| eight_days_ago = time.time() - 8 * 24 * 3600 | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| for i in range(3): | ||
| dirname = os.path.join(tmpdir, f"full_data_op_report_{i}") | ||
| os.mkdir(dirname) | ||
| if i < 2: | ||
|
dierickxsimon marked this conversation as resolved.
Outdated
|
||
| os.utime(dirname, (eight_days_ago, eight_days_ago)) | ||
|
|
||
| assert len(os.listdir(tmpdir)) == 3 | ||
|
|
||
| _utils.prune_folder(tmpdir) | ||
|
|
||
| remaining_items = os.listdir(tmpdir) | ||
| assert len(remaining_items) == 1 | ||
|
|
||
|
|
||
| def test_prune_folder_with_nonstandard_name_dirs(): | ||
| eight_days_ago = time.time() - 8 * 24 * 3600 | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| dirname = os.path.join(tmpdir, "other_report") | ||
| os.mkdir(dirname) | ||
| os.utime(dirname, (eight_days_ago, eight_days_ago)) | ||
|
dierickxsimon marked this conversation as resolved.
Outdated
|
||
|
|
||
| assert len(os.listdir(tmpdir)) == 1 | ||
|
|
||
| _utils.prune_folder(tmpdir) | ||
|
|
||
| remaining_items = os.listdir(tmpdir) | ||
| assert len(remaining_items) == 1 | ||
|
dierickxsimon marked this conversation as resolved.
Outdated
|
||
| assert remaining_items[0] == "other_report" | ||
|
|
||
|
|
||
| def test_prune_folder_catch_exception(monkeypatch): | ||
| eight_days_ago = time.time() - 8 * 24 * 3600 | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| dirname = os.path.join(tmpdir, "full_data_op_report_test") | ||
| os.mkdir(dirname) | ||
| os.utime(dirname, (eight_days_ago, eight_days_ago)) | ||
|
|
||
| def mock_rmtree(path, *args, **kwargs): | ||
| raise OSError("Cannot delete folder") | ||
|
|
||
| monkeypatch.setattr("shutil.rmtree", mock_rmtree) | ||
|
|
||
| assert len(os.listdir(tmpdir)) == 1 | ||
|
|
||
| with pytest.warns(UserWarning, match="Could not delete"): | ||
| _utils.prune_folder(tmpdir) | ||
|
|
||
| monkeypatch.undo() | ||
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.
nitpick: the rest of skrub uses the (newer) pathlib module for those kind of filesystem manipulation rather than os, so for consistency it would be great to use it here, especially since the path you get from
_get_output_diris already a pathlib.Pathfor example you can get the mtime with stat, the Path object representing the directory has iterdir() and glob() methods, you can join with the
/operator orjoinpath, etc.also maybe we could name
foldersdirectoriesinstead? since we use the term 'directory' rather than 'folder' for other variables (same for the function name prune_folder)finally why do you prefer 2 separate loops for finding the directories and then removing them?
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.
totally agree with the variable naming — I’ll change it, and I’ll switch to using the pathlib module as well. As for the two separate loops: I initially used them for debugging purposes so I could easily print all the folders that were selected. But maybe its better to and even more readable to put it all in one loop. I will change it!