Fix IndexedTarStore writer reload truncation#150
Conversation
|
/ok to test 78dd098 |
|
This is great, thanks for fixing @fallintoplace - LGTM. Can you please still correct the commit message to conform to the conventional-commit requirements (maybe use I think we never ran into this as we only ever reloaded resources when reading (e.g., when training multi-process @ torch dataloader, where the stores are loaded in the main process, requiring a reload in the forked workers) - maybe I ask in which setting you ran into this while writing (wondering how you could have a setting where you'd need to reload the resources while writing)? |
janickm
left a comment
janickm
left a comment
There was a problem hiding this comment.
Code looks great - just the commit message requires fixes + signature are missing. Maybe adding details to the commit body would also be useful.
reload_resources() reopened write-mode stores with wb+, which truncates an in-progress archive while the in-memory index still references entries that were already written. Reopen write-mode stores with rb+ during resource reload so existing tar payloads are preserved, and cover the behavior with a regression test that writes before and after reload. Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
fallintoplace
force-pushed
the
fix/itar-reload-resources
branch
from
78dd098 to
3dba779
Compare
|
/ok to test 3dba779 |
|
thanks @fallintoplace! approved, merging once CI succeeds do you mind still sharing in which types of setups you ran into this problem (reloading resources while writing?) |
2 participants
Summary
IndexedTarStore.reload_resources()reopened write-mode archives withwb+, which truncates the archive during an in-progress write. This changes the writer reload path to reopen the existing archive withrb+so already-written tar payloads remain intact.I also added a regression test that writes one entry, reloads writer resources, verifies the file size and payload are preserved, writes a second entry, and then reopens the archive in read mode to check both entries.
Test
PYTHONPATH=$PWD uv run --no-project --python 3.12 --with 'pytest==8.3.4' --with 'cbor2==5.9.0' --with 'numcodecs==0.15.1' --with 'numpy==1.26.4' --with 'parameterized==0.9.0' --with 'universal-pathlib==0.3.6' --with 'zarr==2.18.4' pytest -q ncore/impl/data/stores_test.pyResult:
26 passed