Repository navigation
feat!: reintegrate cpm-geozarr into main - #277
Conversation
Adds eopf_geozarr.cpm, an integration package for eopf-cpm (the eopf package, available via the new 'cpm' extra on Python >= 3.13): - GeoZarrWriter, registered in CPM's EOWriterRegistry as engine "geozarr" (by-name only: cpm_zarr already claims .zarr targets in by-target discovery). Routes products to the Sentinel-2 optimized pipeline or the generic pipeline based on the declared CPM product:type, with a structural fallback. Zarr v3 only; rejects unsupported modes, formats, and options loudly. - an 'eopf convert-geozarr' subcommand exposed through CPM's eopf.cli entry-point group, delegating to eopf.store.convert with the engine preset, enabling one-pass SAFE -> GeoZarr conversion. - routing helpers importable without eopf installed. convert_s2/convert_s2_optimized now skip backing-store validation for DataTrees with no zarr backend (e.g. built in memory by the CPM SAFE reader) instead of crashing in get_zarr_group. Verified against eopf 3.0.0 on Python 3.13: engine registration, convert-geozarr discovery in the eopf CLI, and an end-to-end write_datatree(engine="geozarr") producing the flat r10m..r720m pyramid from an S2 example store. Assisted-by: ClaudeCode:claude-fable-5
Fixes the confirmed findings from a multi-agent adversarial review: - write() now runs every argument-derivable check (target type, remote scheme, w- existence, pipeline routing, required 'groups') BEFORE the destructive mode="w" removal of an existing store. Previously a pure options mistake (e.g. missing groups) deleted the user's previous product and then failed with nothing written. - accept compute=True (rejecting only compute=False): CPM's staged-output path (staging.py) injects compute=True when a remote Dask client is active, so the strict kwargs rejection broke the exact --stage-output flow the writer's own error message recommends. - validate_write_options now applies the target checks (type, remote scheme), so CPM's triggering preflight rejects unsupported targets at parse time instead of at write time. - routing: only S02MSIL1C/S02MSIL2A products go to the S2-optimized pipeline; other MSI types (S02MSIL0_, S02MSIRAW) lack the reflectance pyramid structure and now route generic. - eopf_geozarr.cpm raises a clear ImportError explaining the Python >= 3.13 floor when eopf is missing (pip install 'eopf-geozarr[cpm]' silently installs nothing on 3.12). - _validate_s2_input: cover both branches with tests; also skip validation for zarr-v3-backed trees (the S2 input model reads Zarr v2 only and raised TypeError on CPM 3.x products). Verified on Python 3.13 with eopf 3.0.0: 42 tests pass, including new regression tests for the destroy-then-fail ordering and compute handling. Assisted-by: ClaudeCode:claude-fable-5
- test-cpm job: installs the cpm extra (eopf pinned via uv.lock) on Python 3.13 and runs the CPM writer/routing tests on every push/PR, so they no longer silently skip in CI. - test-cpm-latest job: weekly scheduled run against the newest published eopf release to catch upstream contract drift (writer registry, staging, CLI entry-point loading) independently of PR failures. - new test: invoke the real `eopf convert-geozarr` console script via subprocess on a synthetic S2 store, covering entry-point discovery, click option wiring, and convert()'s reader dispatch through to the geozarr engine. Assisted-by: ClaudeCode:claude-fable-5
PR #212 (Sentinel-3 OLCI GeoZarr export) merged to main after #238 was branched, leaving select_pipeline unable to reach it: OLCI products fell into the generic pipeline and failed on the missing 'groups' option. Adds S3_OLCI_PRODUCT_TYPE_PREFIXES and looks_like_sentinel3_olci to routing.py, structural fallback included -- oa01_radiance is a data variable of the measurements node, not a child node, unlike the S2 r{N}m pyramid groups. Wires convert_olci_optimized into the writer with a new s3_olci_optimized flag and an output_grid option, kept additive to the existing write() signature (s2_optimized is untouched) since that's the EOWriter entry point other CPM code calls into.
* ci: run CI on the cpm-geozarr branch The workflow only triggered on main, so no work on the cpm-geozarr feature branch ever got checks -- including #247, which is 20 commits and +29105/-3763 across 23 files. The Samples Service runs this branch in its September GeoZarr production track, so changes reached a production pipeline unchecked. Closes #253 * style: wrap the resolved_spatial_chunk conditional to satisfy ruff format Introduced by the OLCI routing commit, which renamed the local `pipeline` variable to `selected_pipeline` and pushed the line past the limit. It went unnoticed because the branch had no CI -- which is what this PR fixes.
* init project setup * adding and harmonising the chunking/sharding functionality across measuremetn and non-measurement groups - allows the contuniation of chunking behaviour from input eopf datatree instead if overwriting by force - scale_offset logic not touched * addign nan masking for coarsening operation for multiscale calualtation (only for s2_optimising, later extend!) response to issue #246 * addign rechunking after readin to circumvent chunking at read-in (which is hardwired to eopf/store/mappers) * adding L1C injected bands for mutliscale calculation -> with rechukning of added bands to spatial_chunk size * readding prior launch script * implementing changes recommended by claxn and d-v-b, mainly docs, cleanup, simplification * adressing change requests, improving chunk workflow * simplifing encoding retrieval for (all and) 1d arrays * hotfix: adding stac metadata to root level was previously not inlcuded -> copying root metadata to new tree root now * adding docstrings * working on the tests, frustrated with test layout * adding zarr v3 test samples * change of the data_api tests * removing test structure which is not adhering to s2 specs, also 'notebook' scripts referenced in tests didnt seem to exist * changing the s2 zarr v3 structure and all associated tests. Also removing all emojis from output messages * adapting to new zarr layout, changed function calls, and multiscale calculation changes * removing emojis * adding checks to add metadata to arrays if keep_scale_offset=False, modifying checks for s2 structure to align with tests, addig docs * fixing enum to strenum to allow detection of S2type in simple == comparison * applying changes to validate pre-commit pyright issues * addressign requested chagnes for stAc metadata addition adn fill_value fallbacks, removing 'experimental_scale_offset_codec'functionality for encoding2 * small issue of not rechuking in tests * removing addition of multiscales to stac asset metadata, other small commetns * adding _FillValue to encoding to allow xarray decode_cf to correctly read zarr store in abscence of eopf-xarray reader, removed large commented section --------- Co-authored-by: Emmanuel Mathot <[email protected]>
…ng pr247 (#267) * adding single /measurement/refectance group to stac as requested during pr247 * adressing comments for crs reuse and proj_attrs_for_crs use * adding test to incorporate new functionalities and expanding comments
Brings in S1 GRD RTC ingestion (#216), release 0.11.0 (#235) and dependency bumps (#268, #269). Conflicts: tests/conftest.py keeps the cpm-geozarr v3 S2 fixtures and adds main's S1 RTC fixtures; uv.lock re-locked from main's lock. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Brings in the spatial:bbox pixel-edge fix (#275, fixes #266). Co-Authored-By: Claude Opus 5.5 <[email protected]>
| compression_level: int, | ||
| validate_output: bool, | ||
| keep_scale_offset: bool, | ||
| experimental_scale_offset_codec: bool = False, |
There was a problem hiding this comment.
Are we ok to remove experimental_scale_offset_codec? It will break the current data-pipeline: scripts/convert_v1_s2.py (L173) always passes it, so every S2 conversion raises TypeError, after the script has already deleted the previous output (L106).
There was a problem hiding this comment.
as mentioned below, during our usage of the conversion experimental_scale_offset_codec was not triggered and its use dropped from the encoding generation, if it is required on your end it can be added back again, but as we discussed a possible full switch to the FixedScaleOffsetCodec we could maybe re-integrate such functionality during the follow-up discussion?
There was a problem hiding this comment.
Thanks @shollendEODC. We agree to move to codec-based encoding. We keep one switch: the Zarr codec becomes the default, and the ESA layout (CF attributes + STAC raster:scale/raster:offset) is the opt-in. Your keep_scale_offset=True calls keep working for one release, with a deprecation warning. Details and the codec choice (scale_offset + cast_value rather than FixedScaleOffset, because the viewer can already read them) are in #281.
| # The scale levels in the output data. 10, 20, 60 already exist in the source data. | ||
|
|
||
| # cheap dEtermination if its L2A or L1C | ||
| filename = dt_input.name |
There was a problem hiding this comment.
Claude flagged this changed as Blocking.: dt_input.name is None for zarr-opened trees (open_source_datatree, the CLI, CPM's zarr reader), so no band gets injected: L2A loses b08 at r20m–r720m, and L1C keeps only b01/b09/b10 from r60m.
There was a problem hiding this comment.
didnt notice this at all during our tests of reading/writing (by passing the eopf.store.safe_reader.py output directly to the geozarr writer). Any idea for a more general passing down of such info from the zarr reader?
| from zarr.codecs import BloscCodec | ||
|
|
||
| compressor = BloscCodec(cname="zstd", clevel=3, shuffle="shuffle", blocksize=0) | ||
| encoding_chunks = get_chunking_for_encoding(var_data) |
There was a problem hiding this comment.
Overview chunks come from the parent's dask chunks, not spatial_chunk. Sharded (as data-pipeline runs it), r120m is a single 915×915 chunk, so a 64×64 tile read decodes ~2.5 MB instead of 102 KB on main. Unsharded, r720m ends up as 64 chunks of 21×21.
There was a problem hiding this comment.
As part of our production we require a uniform rechunking to 1024 x 1024, which is why _rechunk_ds(..) gets applied to all arrays in the datatree... We rechunk the datatree before the multiscale computation, which is why they inherit the 1024 chunking
As of now (in the refactored S1/S3 branches), chunking should be applied across polarization (1 x 1024 x 1024) but are sharded together into a single file
There was a problem hiding this comment.
Thanks both. To keep a trace of this, I opened #282 to measure the downstream effect (titiler-eopf, the OpenLayers viewer, titiler-openeo) of 1024 chunks and single-chunk overviews compared with 256. We expect little impact. If a consumer does degrade, the numbers go into the chunk policy work in #222 / #243.
| assert isinstance(observed_level, zarr.Group) | ||
| input_bands = _band_names(input_level) | ||
| observed_bands = _band_names(observed_level) | ||
| assert input_bands <= observed_bands, ( |
There was a problem hiding this comment.
Blocking. The golden snapshots are what would catch the changes above: main's snapshot test run on this branch reports 5 missing b08 arrays and 111 arrays with a different dtype, fill or chunks. Can we keep it and regenerate the snapshots where the change is intended?
There was a problem hiding this comment.
The missing of injected b08 bands needs to be checked and validated, the outputs should inlcude these.
Regarding the other differences, I would adapt the golden snapshots once we agree on the chunking/sharding, fill values and dypes.
| for var_name in sample_dataset.data_vars: | ||
| if "add_offset" in sample_dataset[var_name].encoding: | ||
| for var_name in output_dataset.data_vars: | ||
| if "add_offset" in output_dataset[var_name].encoding: |
There was a problem hiding this comment.
This never runs: sample_dataset is opened with mask_and_scale=False, so add_offset is never in encoding.
There was a problem hiding this comment.
then it can be removed. Or would you also like to test it agaisnt mask_and_scale=True?
|
|
||
|
|
||
| @pytest.fixture(params=s2_example_json_paths, ids=get_stem) | ||
| @pytest.fixture(params=v3_s2_example_json_paths, ids=get_stem) |
There was a problem hiding this comment.
With V3-only fixtures the default job no longer covers V2 S2 input, which is what data-pipeline converts today, are we ok with that?
There was a problem hiding this comment.
We could assume that zarr v3 is the only supported version once this PR merged. If necessary, a v2 support branch could be started
|
As a general note: I did the refactoring and changes introduced in this merge solely based on our requirements/usage of this repo as its standalone product, not considering its role in the whole EOPF-Explorer project and your pipeline. The issues raised here (particularily regarding the larger refactoring mentioned in this PR in |
|
Hi @emmanuelmathot and @lhoupert , many thanks for the nice overview and status summary. The highest priority at the moment is to onboard and operationalise every EOPF product. We would suggest to merge this PR as is to We would suggest the following next steps, which are important from our side to produce the data:
We would suggest to keep all other points mentioned above in the table as unresolved for the time being. We will leave the remaining points up to you to decide what is useful to be kept and what not in the scope of the EOPF-Explorer project. Nevertheless, here are some comments:
Please let us know if you agree with the proposal and also the suggested task assignment. |
Brings in #279 and #280. uv.lock re-locked from main's lock. Co-Authored-By: Claude Opus 5.5 <[email protected]>
L1C/L2A detection read dt_input.name, which is None for trees opened from a Zarr store, so no band was injected: L2A lost b08 at r20m-r720m and L1C kept only b01/b09/b10 from r60m. Detect the level from stac_discovery product:type (set by every CPM S2 mapping), with the name as fallback. inject_missing_bands also updated the source band's attrs in place, so native r10m bands sometimes carried the injected bands' provenance attrs, depending on write timing. Copy the attrs instead. Restore the golden snapshot test, run on the Zarr v2 inputs, and regenerate the snapshots for this branch's output (REGENERATE_SNAPSHOTS=1 rewrites them). Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks @claxn and @shollendEODC, we agree with the plan: merge this PR, cut a release, then one PR per product into Before the merge, I addressed @lhoupert's two blocking comments:
Scale/offset encoding. Good news: we agree to make the Zarr codec the default. Followup tracked in #281 Your CPM calls pass @lhoupert's point on the overview chunk layout (1024 chunks, single-chunk r120m/r360m) is tracked in #282. Thanks @shollendEODC for offering your benchmark results there. We will measure the downstream effect on our side too, before the chunking work in #222 / #243. Assignments:
In general, let's keep our usual review process for all these PRs: open them against |
|
thanks for the merge, I will set up the PRs I have been working on now |
Resolve the tests/test_data_api/test_geozarr/test_common.py conflict with #277: keep main's hand-built test_multiscales_round_trip() and drop the CF tests. uv.lock is main's lock minus cf-xarray. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Summary
This PR reintegrates the
cpm-geozarrbranch intomain, as EODC requested. It is a draft for coordination with EODC.The PR contains the work that is already merged on
cpm-geozarr, plusmainmerged into it:geozarrengine and theeopf convert-geozarrcommand (cpm/, Integrate with CPM by writing a writer plugin #236)cpm-geozarr(ci: run CI on the cpm-geozarr branch #254)/measurements/reflectanceSTAC asset withprofile=multiscales(adding single /measurement/refectance group to stac as requested during pr247 #267)This PR deletes no modules.
data_api/,pyz/, the minispec validator, the argparse CLI and the test suite stay as they are onmain.Why the PR comes from
integrate/cpm-geozarr-mainI did not push the merge commits to
cpm-geozarrdirectly. This branch iscpm-geozarrplus two merges ofmain. To complete the reintegration, we can merge this PR. Or, if EODC prefers to keep working oncpm-geozarr, we can fast-forwardcpm-geozarrto this branch first.Merge resolution
tests/conftest.py: keeps the Zarr v3 S2 fixtures fromcpm-geozarrand adds the S1 RTC fixtures frommain(feat(s1): Sentinel-1 GRD RTC GeoZarr ingestion, data model and STAC builders #216). The fixtures thatcpm-geozarrremoved have no remaining users.uv.lock: re-locked from the lockfile onmain. No version onmainchanges. Thecpmextra adds 36 packages.eopf3.x limits numpy. On Python 3.13 and later, thecpmextra therefore resolves numpy 2.4.6, not 2.5.3.spatial:bboxpixel-edge fix (fix(s2): compute spatial:bbox and spatial:transform from pixel edges #275). It merged with no conflicts, so it fixes Fix correct bounds for spatial:bbox attributes due to mismatch/offset between coords and bounds #266 on both lines. We do not need a cherry-pick.Verification
Local runs, on the final merge commit:
-m "not network"): about 760 tests, all pass. 5 are skipped and 2 are expected failures, as onmain.--extra cpm: 42 passCoordination with EODC: the next refactor
EODC has a branch that is not a PR yet:
shollendEODC/cpm-geozarr-eodc-large-refactor-s1slc. ItsCHANGELOG.mddescribes a large refactor. It adds S1 GRD and SLC, and S3 SLSTR. It also removes large parts of the package.The team has not agreed to these removals. After this PR merges, a PR into
cpm-geozarris effectively a PR intomain. Thus we must discuss each proposed removal before that branch becomes a PR. A test merge of that branch against this one gives 27 conflicts.mainand other reposdata_api/(pydantic-zarr models for S1, S2, S3, GeoZarr) andpyz/conversion/s1_ingest.pyandstac/s1_rtc.py(S1 RTC, #216) import it.data-pipelinescriptsvalidate_s1_rtc.py,ingest_v1_s1_rtc.pyandregister_v1_s1_rtc.pybreak.data_api/geozarr/validation.py) andeopf-geozarr validatecli.pydoes nothing)eopf-geozarr convert*commands stop working. Theeopf convert-geozarrcommand only works witheopfinstalled (Python 3.13 or later).classificationandquality_maskresamplingconversion/geozarr.py(create_geozarr_dataset) andconversion/open_source.py(open_source_datatree)data-pipelinescriptsconvert_v0.pyandconvert_v1_s2.pyimport themmultiscales/tms.py)standard_namecheckThe EODC changelog also lists open bugs on that branch:
bool([...])is always trueEffect on other open PRs
uv.lock. Re-lock after this merge.Issues
Closes #236, closes #253, closes #256.
Partially addresses #250 (S2 only) and #246.
cc @shollendEODC @d-v-b @lhoupert
🤖 Generated with Claude Code