Make patch_unsloth_gguf_save actually protect the save directory#7149
Make patch_unsloth_gguf_save actually protect the save directory#7149vineethsaivs wants to merge 3 commits into
Conversation
patch_unsloth_gguf_save was a no-op context manager: it saved and restored
shutil.rmtree but never replaced it, so the documented guard ('Prevent deletion
of the directory self.save_pretrained just created') did nothing. Its sibling
patch_unsloth_save patches shutil.rmtree correctly.
Replace shutil.rmtree with a wrapper that skips deletion of the protected save
directory (and its subtree) and delegates everything else to the original, so
unrelated temporary directories are still cleaned up. The context manager now
takes the directory to protect.
for more information, see https://pre-commit.ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in patch_unsloth_gguf_save where shutil.rmtree was saved and restored but never actually overridden, leaving the save directory unprotected. The fix implements a guarded version of shutil.rmtree that prevents deletion of the protected directory and its subdirectories, and a new test suite is added to verify this behavior. The review feedback recommends using os.path.normcase on the paths during comparison to ensure the guard works reliably on case-insensitive filesystems like Windows and macOS.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| protected = os.path.abspath(protected_directory) | ||
| original_rmtree = shutil.rmtree | ||
|
|
||
| def guarded_rmtree(path, *args, **kwargs): | ||
| target = os.path.abspath(os.fspath(path)) | ||
| if target == protected or target.startswith(protected + os.sep): | ||
| return | ||
| return original_rmtree(path, *args, **kwargs) |
There was a problem hiding this comment.
On case-insensitive filesystems (such as Windows or macOS), path comparisons can fail due to differences in casing (e.g., drive letters like C: vs c: or directory names). To ensure the guard reliably prevents deletion of the protected directory across all platforms, use os.path.normcase on both paths before performing the comparison.
| protected = os.path.abspath(protected_directory) | |
| original_rmtree = shutil.rmtree | |
| def guarded_rmtree(path, *args, **kwargs): | |
| target = os.path.abspath(os.fspath(path)) | |
| if target == protected or target.startswith(protected + os.sep): | |
| return | |
| return original_rmtree(path, *args, **kwargs) | |
| protected = os.path.normcase(os.path.abspath(protected_directory)) | |
| original_rmtree = shutil.rmtree | |
| def guarded_rmtree(path, *args, **kwargs): | |
| target = os.path.normcase(os.path.abspath(os.fspath(path))) | |
| if target == protected or target.startswith(protected + os.sep): | |
| return | |
| return original_rmtree(path, *args, **kwargs) |
There was a problem hiding this comment.
Good point, applied in 38eb2f1. Both paths now go through a normalize() helper that does os.path.normcase(os.path.abspath(os.fspath(path))), so the comparison holds on case-insensitive filesystems. The same commit also exempts the configured temporary_location from the guard, per the other review thread, since it can resolve inside the protected tree.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35c2f9f1b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if target == protected or target.startswith(protected + os.sep): | ||
| return |
There was a problem hiding this comment.
When callers put temporary_location under save_directory (or save to . so the default relative _unsloth_temporary_saved_buffers is inside the protected tree), this descendant check turns the cleanup in unsloth/save.py (shutil.rmtree(temporary_location, ignore_errors=True)) into a no-op. That leaves intermediate .pt buffers in the saved SentenceTransformer directory, and the push_to_hub=True path uploads the whole folder, including those scratch files. The guard should either protect only the directory Unsloth might wrongly delete or explicitly allow the configured temporary directory to be removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 38eb2f1. This was a real hole in the guard.
temporary_location defaults to the relative "_unsloth_temporary_saved_buffers", so it resolves under the current directory: saving to . puts it inside the protected tree and the descendant check turned save.py's shutil.rmtree(temporary_location, ignore_errors=True) into a no-op. Reproduced it directly against the previous revision, with the scratch .pt left sitting in the saved model afterwards, which the push_to_hub=True path would then upload.
Went with your first option, explicitly allowing the configured temporary directory. The context manager now takes it and exempts that subtree:
def guarded_rmtree(path, *args, **kwargs):
target = normalize(path)
if target == protected:
return
if temporary is not None and is_within(target, temporary):
return original_rmtree(path, *args, **kwargs)
if is_within(target, protected):
return
return original_rmtree(path, *args, **kwargs)The target == protected check stays first so the save directory itself is never removable, even if someone points temporary_location straight at it. Added a regression test for a temp directory nested inside the save directory: it fails on the previous revision and passes now.
…ctory temporary_location defaults to a relative path, so it resolves inside the save directory when the model is saved to the current directory. The guard turned its cleanup into a no-op, leaving scratch buffers in the saved model. Exempt the configured temporary location from the guard, and normcase the paths so the comparison holds on case-insensitive filesystems.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38eb2f1ef5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if temporary is not None and is_within(target, temporary): | ||
| return original_rmtree(path, *args, **kwargs) |
There was a problem hiding this comment.
When temporary_location is an ancestor of save_directory (for example, a user saves to a model subdirectory inside a custom scratch directory), the cleanup in unsloth_generic_save still calls shutil.rmtree(temporary_location, ignore_errors=True) and this branch delegates that delete before checking whether the target contains the protected output. In that scenario the whole scratch directory is removed, including the SentenceTransformer directory this guard is meant to preserve; the temp exception should not be allowed when the requested delete would also contain protected.
Useful? React with 👍 / 👎.
1 participant
Summary
patch_unsloth_gguf_saveinunsloth/models/sentence_transformer.pyis documented to "Prevent deletion of the directoryself.save_pretrainedjust created", but it is a no-op:It saves and restores
shutil.rmtreebut never replaces it, so nothing is guarded. The siblingpatch_unsloth_savein the same file does patchshutil.rmtree, and the adjacent code already computes an absolute path "for later commonpath comparison", so the guard was clearly meant to be there and was left unfinished.Fix
Replace
shutil.rmtreefor the duration of the GGUF save with a wrapper that skips deletion of the protected save directory (and its subtree) and delegates every other path to the originalshutil.rmtree. This makes the documented guard actually work without suppressing the legitimate temporary-directory cleanups (temporary_location, thellama.cppbuild dir) that also run inside the wrapped call. The context manager now takes the directory to protect, and the call site passessave_directory.Test
Added
tests/python/test_gguf_save_rmtree_guard.py, a CPU-only test (auto-discovered by the repo'spytest tests/, no GPU) that AST-extracts the context manager (unsloth can't be imported without a GPU) and asserts that, inside the guard, deleting the save directory or a subdirectory is a no-op while an unrelated temp directory is still removed, and thatshutil.rmtreeis restored on exit. It fails against the previous no-op version (the save directory gets deleted) and passes with this change.