Skip to content

Conversation

shoyer
Copy link
Contributor

@shoyer shoyer commented Aug 27, 2025

Fixes #3411

I use the standard strategy of writing to a temporary file in the same directory, and then renaming it to the desired name. This ensure that Zarr writes are either complete or not written at all.

It would be nice if partial writes (_put() with start) could also be atomic, but I'm honestly not sure if both atomic and efficient partial writes of files are possible. Interestingly, I don't see any uses of set_partial_values() inside Zarr, so maybe this feature isn't needed after all? (see #2859 for discussion)

TODO:

  • Add unit tests and/or doctests in docstrings
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

Fixes zarr-developers#3411

I use the standard strategy of writing to a temporary file in the same
directory, and then renaming it to the desired name.

This ensure that Zarr writes are either complete or not written at all.
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Aug 27, 2025
@shoyer shoyer changed the title Use atomic writes in LocalStore Use atomic writes for new files in LocalStore Aug 27, 2025
Copy link

codecov bot commented Aug 27, 2025

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.70%. Comparing base (c9509ee) to head (f2c1fcd).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/zarr/storage/_local.py 91.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3412      +/-   ##
==========================================
- Coverage   94.70%   94.70%   -0.01%     
==========================================
  Files          79       79              
  Lines        9532     9549      +17     
==========================================
+ Hits         9027     9043      +16     
- Misses        505      506       +1     
Files with missing lines Coverage Δ
src/zarr/storage/_local.py 92.39% <91.66%> (+0.17%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

) -> int | None:
path.parent.mkdir(parents=True, exist_ok=True)
# write takes any object supporting the buffer protocol
view = value.as_buffer_like()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes me wonder why Buffer doesn't implement the buffer protocol!

Copy link
Contributor

@d-v-b d-v-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably get a release note for this change. I'm happy to write one if that works. Otherwise it looks g2g!

@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Aug 28, 2025
@d-v-b d-v-b enabled auto-merge (squash) August 28, 2025 17:26
@shoyer
Copy link
Contributor Author

shoyer commented Aug 28, 2025

The missing code coverage is for the unused/untested set_partial_values (#2859) and for os.rename on Windows (I guess Codecov does not include the Windows build?)

@d-v-b d-v-b merged commit 96a531b into zarr-developers:main Aug 28, 2025
29 checks passed
@shoyer shoyer deleted the atomic-write branch August 28, 2025 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Atomic writes in LocalStore

2 participants