Repository navigation
subset, recast, split, concat: write beside the output, rename into place - #42
Conversation
Fixes #35 and the other non-atomic writes found alongside it. - subset: resolve the selection before the output exists, refuse an output that names the input (on Zarr it emptied the store), and write through staged_outputs, so a failed query leaves nothing and an existing output survives a failure. - subset/recast --inplace: the source was deleted before the new store was moved in; it is now renamed aside and deleted only afterwards. recast -o is staged too. - split: stage every output, write the manifest last, refuse an output that lands on the input. - staged_outputs: refuse a leftover *.adata-old.* rather than deleting what may be the only copy, put back already-swapped outputs if a rename fails, and keep a suffixless Zarr store's temporary a Zarr store. - same_path moves to util/path.py; convert's copy answered "different" when it could not tell. Drop recast's unused ALL_PREFIXES. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failure part way through left a partial store at -o, which then read as a finished one and blocked the rerun (concat refuses an existing output). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Test Results (py3.12)1 217 tests 1 217 ✅ 2m 34s ⏱️ Results for commit 8a3550a. |
![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: 8a3550a54e
ℹ️ 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 path.is_dir() and path.suffix != ".zarr": | ||
| # A Zarr store named without the suffix. A temporary named the same | ||
| # way would not exist yet, and so be opened as HDF5. | ||
| temp = temp.with_name(f"{temp.name}.zarr") |
There was a problem hiding this comment.
When a subset, recast, or split destination is an existing ordinary directory without a .zarr suffix, path.is_dir() incorrectly classifies it as a suffixless Zarr store. The temporary path consequently receives .zarr, writing succeeds there, and the swap later moves the original directory aside and recursively deletes it as a backup. Previously open_store() inspected the destination and rejected directories lacking Zarr metadata, so a typo such as -o existing-directory now destroys unrelated contents; use the Zarr-content check rather than treating every directory as a store.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Test Results (py3.13)1 217 tests 1 217 ✅ 4m 1s ⏱️ Results for commit 8a3550a. |
Fixes #35, plus the other non-atomic writes found alongside it. These are data-safety fixes for 0.7.1.
What was wrong
subset x.zarr -o x.zarremptied the store. The output was opened"w"before the selection was read from the source it had just cleared, and the command reported success.-otarget.subset --inplaceandrecast --inplacedeleted the source, then moved the new store in. An interrupt between the two steps lost the data.recast's module docstring called this "an atomic swap".recast -o,splitandconcatwrote straight into their outputs. A failure left a partial store, which the next command would read as a finished one.What changed
subset: resolves the selection before any output exists, and refuses an output that names the input (use--inplace). Overwriting an existing-ooutput still works as before; the old output is replaced only once its successor is complete.subset,recast,splitandconcat: all now write throughutil/staging.staged_outputs, asconvertalready did.--inplacerenames the source aside and deletes it only after the new store is in place.splitwrites its manifest last, and refuses an output path that lands on the input.staged_outputsis hardened:*.adata-old.*instead of deleting it, since after an interrupted swap that backup may be the only copy;.zarrsuffix now stays Zarr (store.adata-tmpwould otherwise be opened as HDF5).same_pathmoves toutil/path.py.convert's copy answered "different paths" when it couldn't resolve them, which is the unsafe answer.recast's unusedALL_PREFIXESis removed.Tests
tests/test_write_safety.pyreproduces each bug on both h5ad and Zarr. 13 of its tests fail onmain.tests/test_staging.pycovers the helper's failure paths. It also takes over the two direct staging tests fromtest_convert.py.pytest -m "not integration": 1221 passed, coverage 94.1%;recast --inplaceon a 300k × 3k store (18M nonzeros), for both h5ad and Zarr.*.adata-tmp*copy stays, and the next run refuses to start until it is removed.#36 (concat's Zarr version) is stacked on this branch in a separate PR.
🤖 Generated with Claude Code