Let a writer keep a half-written directory out of an upload - #13
Let a writer keep a half-written directory out of an upload#13v-positronic wants to merge 1 commit into
Conversation
`exclude` matches glob patterns against a path, so it cannot say "a directory that contains `.unfinished`" — that is a fact about the directory's contents, and the path carries no sign of it. A writer marking its in-progress output therefore had no way to keep it out of the destination, and the workarounds are a move or a post-hoc cleanup, which lose the artifact or the tidiness they were meant to protect. `skip_dirs_containing` names those marker files. The local scan skips a directory holding one, with its whole subtree, on every upload the registration performs — so a directory that finishes and drops its marker uploads on the next pass, and one that never finishes never leaves the machine. Upload half only, deliberately: a directory the destination already holds is not deleted there for gaining a marker. Ticket: Positronic-Robotics/internal#388 #refs
|
@codex review |
![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: 625905ed15
ℹ️ 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".
| _filter_fileinfo(_scan_local(local_path, skip_dirs_containing), exclude), | ||
| _filter_fileinfo(self._scan_s3(scan_bucket, scan_prefix, profile), exclude), |
There was a problem hiding this comment.
When delete=True (the default) and a marked directory already has objects in S3, pruning that directory only from the local iterator makes _compute_sync_diff classify every remote entry under it as target-only; the deletion loop then schedules those objects for removal. Thus adding a marker deletes the previously uploaded subtree instead of merely preventing new bytes from leaving, contrary to the documented behavior. Keep marked prefixes out of to_delete while still suppressing their uploads.
Useful? React with 👍 / 👎.
|
Closing: the case that motivated this is no longer a case. Uploading a half-written episode directory to the run prefix is acceptable — every reader that opens the dataset there ignores it (the recorder marks it, the dataset listing skips it, and the episode reader refuses it), so what reaches S3 is an extra directory nobody opens rather than a broken episode anybody sees. That leaves |
2 participants
What
upload()andsync()takeskip_dirs_containing: filenames that mark a local directory as not-to-be-uploaded. A directory holding one is skipped with its whole subtree, on every upload the registration performs.Why
excludematches glob patterns against a path. "A directory that contains.unfinished" is a fact about the directory's contents, and the path carries no sign of it — so a writer that marks its in-progress output has no way to keep it out of the destination.The case this comes from: an episode recorder writes into
<run>/<block>/<episode>/and drops a.unfinishedmarker while the episode is open, clearing it on commit. A run that crashes leaves that directory half-written, and today it uploads to the customer-visible prefix like any other. The alternatives are a move (rename or relocate the directory so a glob can see it) or a post-hoc cleanup — one is machinery whose failure modes need their own retry semantics, the other deletes the artifact somebody wanted for debugging.Behaviour
sync()passes it to the upload it registers; the download is untouched.exists()per directory and skips the subtree rather than filtering it file by file.Verification
uv run pytest— 140 passed. Three new tests: a marked directory and its subtree are absent from the scan while a sibling is kept; naming no marker keeps everything; a directory that loses its marker is scanned again.Version bumped to 0.4.0 with a changelog entry — the consumer that needs it pins a release.