Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
| except OSError as e: | ||
| if e.errno not in _TRANSIENT_REMOVE_ERRNOS or not path.exists(): | ||
| raise | ||
| time.sleep(delay) |
There was a problem hiding this comment.
Non-blocking: every old checkpoint whose removal keeps failing now adds up to 0.75 s of sleep to each cleanup, with no limit across directories, and every rank waits for it at the _agree barrier that ends save(), wait() and flush_pending_save(). With 10 old directories that kept raising ENOTEMPTY (as when a process on the same node holds their files open), one _cleanup slept 7.5 s over 50 rmtree calls, against 0 s and 10 calls on main.
There was a problem hiding this comment.
Capped in a9e7c51: the removals of one cleanup share the 0.05, 0.1, 0.2 and 0.4 s waits, so a cleanup requests at most 0.75 s of retry sleep however many directories keep failing. With your ten directories that is fourteen rmtree calls.
| _OPTIM_PREFIX = "optimizer." | ||
| _UNRECORDED_WEIGHT_DECAY_SCOPE = "exclude_norms_and_biases" | ||
| _TRANSIENT_REMOVE_ERRNOS = frozenset({errno.ENOTEMPTY, errno.EBUSY, errno.ENOENT}) | ||
| _REMOVE_RETRY_DELAYS = (0.05, 0.1, 0.2, 0.4) |
There was a problem hiding this comment.
Non-blocking: the 0.75 s budget fits the windows measured on netscratch but not every NFS server, so a run can still end with a leftover directory when its last prune misses. On a second NFS server (Isilon), writing a directory the way a save does and removing it at once left a .nfs entry that outlived 0.75 s in 1 of 317 failed removals (it lasted 4.0 s); the other 316 cleared within 0.38 s.
There was a problem hiding this comment.
Kept at 0.75 s per cleanup, since every rank waits for it at the barrier. The next cleanup still retries a directory that outlasts it, but on such a server old directories can remain, also when the run ends. The docs and the description now say so.
| that is not among the newest, as after a rewind, from being deleted | ||
| while it is written. | ||
|
|
||
| A directory that NFS briefly keeps non-empty is retried; one that still |
There was a problem hiding this comment.
The user docs still describe the old behavior: docs/checkpointing/auto-resume.md:158-160 says a .nfs placeholder makes the removal fail and leaves the checkpoint for the next cleanup, with no mention of the retries added here.
| monkeypatch.setattr(mgr_mod.logger, "warning", warnings.append) | ||
| mgr._cleanup() | ||
| assert (tmp_path / "step_10").is_dir() | ||
| assert attempts == ["step_10"] * 5 |
There was a problem hiding this comment.
Nit: no test checks that the fifth and last attempt holds the sidecar lock. With the lock dropped from the final rmtree (manager.py:421), all nine cleanup and sidecar tests still pass, since this test reaches the fifth attempt but records only the directory name and the NFS test stops after four.
There was a problem hiding this comment.
test_cleanup_retries_a_checkpoint_it_could_not_remove now records the lock state of all five tries and the four waits (a9e7c51), so a try without the lock fails it. On GPU, with the lock taken off the tries, it and two other tests fail.
|
#496 makes the tests of #494 and #372 tolerate a refused removal; this PR retries the removal itself, which also made those tests pass in your NFS runs. The two merge cleanly together, though a bounded retry cannot make every prune succeed. #531, from os.replace, is outside this change. The description now says how they relate. |
Summary
Testing
Closes #527