Repository navigation
Conversation
For zarr files, set up a .complete marker file to check when data is fully written. This replaces relying on zarr.json -- that doesn't work because zarr.json is the first to be written. This might happen from a failed or aborted calculation, or because multiple processes are trying to write the same file (e.g. running 2 countries which share a tile) A partially-written zarr might be not cause any warning signs later, since it might be techncially valid in spite of missing data. Now an empty .complete marker is written beside the zarr after its last chunk, and the entry counts as cached only once the marker exists. A zarr found without its marker is a write that was killed part-way or is still running: serialize raises FileExistsError and leaves it for someone to resolve by hand, usually by deleting the zarr, rather than writing over it and risking a race with a live writer. Entries written before this change have no marker, so each raises once until deleted.
danohu
marked this pull request as draft
October 15, 2026 18:38
An autouse fixture in jdluc/__tests__/conftest.py stubs Config.from_dot_env for every test not marked integration. Each field gets its type's empty value, with two dask workers, so a unit test that reaches for a storage root fails instead of touching a bucket. Integration tests still read the real .env. This replaces the fixture storage_test carried for itself, so the next unit test that writes a zarr needs no copy of it, and a new Config field needs no edit to the tests.
danohu
marked this pull request as ready for review
October 15, 2026 19:35
This was referenced Oct 19, 2026
jeffseif
requested changes
Oct 22, 2026
jeffseif
left a comment
jeffseif
left a comment
Member
There was a problem hiding this comment.
From the IRL discussion:
- we would need a push plan where all existing zarr's have the complete file touched.
| @property | ||
| def exists_uri(self) -> str: | ||
| return f"{self.uri:s}/zarr.json" | ||
| return f"{self.uri:s}.complete" |
Member
There was a problem hiding this comment.
Suggested change
| return f"{self.uri:s}.complete" | |
| return f"{self.uri:s}/complete" |
Comment on lines
+172
to
+177
| if path_exists(uri=self.uri): | ||
| raise FileExistsError( | ||
| f"{self.uri=:s} exists without {self.exists_uri=:s}: a write was killed " | ||
| "part-way or is still running. Once no process is writing it, delete the " | ||
| "zarr to recompute it." | ||
| ) |
Member
There was a problem hiding this comment.
Summarizing IRL discussion
- we need to capture three states instead of just two: doesn't exist, exists but incomplete, exists and complete
- we could inform this from a second property
complete_uri(which is identical toexists_urifor atomic formats) which is checked first - raises so that callers can condition handling (e.g., polling, skipping, immediately raising, etc.)
- if exists and complete:
ZarrExistsAndCompleteException - if exists and not complete:
ZarrExistsAndIncompleteException
- if exists and complete:
Comment on lines
+8
to
+27
| # The fields a unit test needs a real value for | ||
| TEST_ENV = {"number_of_dask_workers": 2} | ||
|
|
||
|
|
||
| def get_unit_test_config() -> Config: | ||
| """TEST_ENV over every other field left empty, so a unit test that reaches for a root fails rather than touching real storage.""" | ||
| types = typing.get_type_hints(Config) | ||
| empty = {field.name: types[field.name]() for field in dataclasses.fields(Config)} | ||
| return Config(**{**empty, **TEST_ENV}) | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def config_without_dot_env( | ||
| request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| """Unit tests need no .env; integration tests still read the real one.""" | ||
| if request.node.get_closest_marker("integration"): | ||
| return | ||
| config = get_unit_test_config() | ||
| monkeypatch.setattr(Config, "from_dot_env", classmethod(lambda cls: config)) |
Member
There was a problem hiding this comment.
Consider moving this to the location it is being used, to defer the creation of a conftest
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For zarr files, set up a .complete marker file to check when data is fully written.
This replaces relying on zarr.json -- that doesn't work because zarr.json is the first to be written. This might happen from a failed or aborted calculation, or because multiple processes are trying to write the same file (e.g. running 2 countries which share a tile)
A partially-written zarr might be not cause any warning signs later, since it might be techncially valid in spite of missing data.
Now an empty .complete marker is written beside the zarr after its last chunk, and the entry counts as cached only once the marker exists. A zarr found without its marker is a write that was killed part-way or is still running: serialize raises FileExistsError and leaves it for someone to resolve by hand, usually by deleting the zarr, rather than writing over it and risking a race with a live writer.
Entries written before this change have no marker, so each raises once until deleted.
Secondary Change -- allow unit tests to run without a .env
Questions for reviewers
What to do when we hit an incomplete zarr? options:
This will break existing storage, requiring either a bump of the appropriate versions or to delete the zarr folders. Maybe merge when we're bumping the version anyway?