Skip to content

feat!: reintegrate cpm-geozarr into main - #277

Merged
emmanuelmathot merged 12 commits into
mainfrom
integrate/cpm-geozarr-main
Sep 28, 2026
Merged

emmanuelmathot merged 12 commits into
mainfrom
integrate/cpm-geozarr-main

Conversation

@emmanuelmathot

Copy link
Copy Markdown
Contributor

Summary

This PR reintegrates the cpm-geozarr branch into main, as EODC requested. It is a draft for coordination with EODC.

The PR contains the work that is already merged on cpm-geozarr, plus main merged into it:

This PR deletes no modules. data_api/, pyz/, the minispec validator, the argparse CLI and the test suite stay as they are on main.

Why the PR comes from integrate/cpm-geozarr-main

I did not push the merge commits to cpm-geozarr directly. This branch is cpm-geozarr plus two merges of main. To complete the reintegration, we can merge this PR. Or, if EODC prefers to keep working on cpm-geozarr, we can fast-forward cpm-geozarr to this branch first.

Merge resolution

Verification

Local runs, on the final merge commit:

  • Full suite on Python 3.12 (-m "not network"): about 760 tests, all pass. 5 are skipped and 2 are expected failures, as on main.
  • CPM integration tests on Python 3.13 with --extra cpm: 42 pass
  • pre-commit: ruff, ruff format and pyright pass
  • CI on this PR

Coordination with EODC: the next refactor

EODC has a branch that is not a PR yet: shollendEODC/cpm-geozarr-eodc-large-refactor-s1slc. Its CHANGELOG.md describes 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-geozarr is effectively a PR into main. 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.

# Proposed removal Effect on main and other repos Related Decision needed
1 data_api/ (pydantic-zarr models for S1, S2, S3, GeoZarr) and pyz/ conversion/s1_ingest.py and stac/s1_rtc.py (S1 RTC, #216) import it. data-pipeline scripts validate_s1_rtc.py, ingest_v1_s1_rtc.py and register_v1_s1_rtc.py break. #258, #84, #61, #109, #40, #6 Keep, move to Zarr v3 (#258), or replace?
2 Minispec validator (data_api/geozarr/validation.py) and eopf-geozarr validate Removes the check from #237, with no replacement #163, #132 Keep until a replacement exists?
3 argparse CLI (cli.py does nothing) eopf-geozarr convert* commands stop working. The eopf convert-geozarr command only works with eopf installed (Python 3.13 or later). #240 Keep both CLIs during a transition?
4 Resampling by variable type (all variables get a masked mean) SCL and quality masks get mean values, which are not valid classes #274, #245 Keep classification and quality_mask resampling
5 conversion/geozarr.py (create_geozarr_dataset) and conversion/open_source.py (open_source_datatree) data-pipeline scripts convert_v0.py and convert_v1_s2.py import them — Deprecate first, then remove?
6 OGC TileMatrixSet multiscales (multiscales/tms.py) Only ZCM multiscales remain #77 Confirm that no consumer needs TMS
7 CF standard_name check The check rejects nothing today, so removal changes no behaviour #265 Agree to remove
8 All tests and fixtures (about 40 files and 100 fixtures) The branch has no test coverage #251, #102, #240 Rewrite on top of the current suite, not from zero

The EODC changelog also lists open bugs on that branch:

  • the S1 GCP check bool([...]) is always true
  • a second, unused OLCI converter remains

Effect on other open PRs

Issues

Closes #236, closes #253, closes #256.
Partially addresses #250 (S2 only) and #246.

cc @shollendEODC @d-v-b @lhoupert

🤖 Generated with Claude Code

d-v-b and others added 10 commits August 11, 2026 13:49
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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe @emmanuelmathot or @d-v-b have an idea?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed in 63928d4

from zarr.codecs import BloscCodec

compressor = BloscCodec(cname="zstd", clevel=3, shuffle="shuffle", blocksize=0)
encoding_chunks = get_chunking_for_encoding(var_data)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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, (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@shollendEODC shollendEODC Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed in 63928d4

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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This never runs: sample_dataset is opened with mask_and_scale=False, so add_offset is never in encoding.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

then it can be removed. Or would you also like to test it agaisnt mask_and_scale=True?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

covered in #281

Comment thread tests/conftest.py


@pytest.fixture(params=s2_example_json_paths, ids=get_stem)
@pytest.fixture(params=v3_s2_example_json_paths, ids=get_stem)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could assume that zarr v3 is the only supported version once this PR merged. If necessary, a v2 support branch could be started

@shollendEODC

Copy link
Copy Markdown
Collaborator

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 shollendEODC/cpm-geozarr-eodc-large-refactor-s1slc), are thus a bit out of my scope, as I am not familiar with the structure and nature of your EOPF-Explorer/data-model usage. Thus, my refactoring in this branch might diverge more for your usage than for our application (sample below):

from eopf.store.convert import convert
target_store_kwargs = {"engine": "geozarr", "keep_scale_offset": True, "spatial_chunk": 1024, "enable_sharding": True}
source_store_kwargs: dict[str, Any] = {"engine": "safe", "mode": "r"}

convert(source_path=str(safe_dir / file),
             target_path=str(zarr_dir / outfn),
             source_store_kwargs=source_store_kwargs,
             target_store_kwargs=target_store_kwargs)

@claxn

claxn commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Hi @emmanuelmathot and @lhoupert ,

many thanks for the nice overview and status summary.
Instead of heaving everything in a big PR, we decided to split up shollendEODC/cpm-geozarr-eodc-large-refactor-s1slc into smaller chunks/PRs in alignment with the open issues and decisions from your table above.

The highest priority at the moment is to onboard and operationalise every EOPF product. We would suggest to merge this PR as is to main and create a new release. Then we can start to add support for each product step by step (separate PRs to main) and address all open issues related to refactoring after that (except what is necessary to add the products). This should ensures backward compatibility and is easier to review and trace.

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:

  1. If the data_api is still required for EOPF-Explorer, then we would suggest to migrate pydantic classes to Zarr v3 models and deprecate Zarr v2.
  2. Depends on the decision from 1.
  3. We think it is still good to provide two interfaces, one for converting a product directly with CPM and one where you start from a CPM converted product.
  4. Mentioned in our listing above.
  5. Agreed.
  6. No comment.
  7. Agreed.
  8. Mentioned in our listing above.

Please let us know if you agree with the proposal and also the suggested task assignment.

@lhoupert lhoupert changed the title Reintegrate cpm-geozarr into main feat!: reintegrate cpm-geozarr into main Sep 28, 2026
emmanuelmathot and others added 2 commits September 28, 2026 13:40
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]>
@emmanuelmathot

Copy link
Copy Markdown
Contributor Author

Thanks @claxn and @shollendEODC, we agree with the plan: merge this PR, cut a release, then one PR per product into main, with the refactoring points kept open until we decide them.

Before the merge, I addressed @lhoupert's two blocking comments:

  • b08 injection. The L1C/L2A detection now reads stac_discovery.properties["product:type"] and falls back to the tree name. The CPM SAFE mappings (L1C and L2A, PSD14 and PSD15) set product:type as a constant, so this works in your CPM path and for trees opened from a Zarr store. There, the name is empty, which caused the missing b08 arrays. L2A now has b08 again at r20m–r720m, and L1C has all 13 bands from r60m.
  • Snapshot test. The golden snapshot test is back, on the Zarr v2 inputs (this also covers Loïc's point about v2 input). The snapshots are regenerated for the output of this branch. When we change chunking, fill values or dtypes, REGENERATE_SNAPSHOTS=1 pytest -k snapshot rewrites them, and the JSON diff shows the change.

Scale/offset encoding. Good news: we agree to make the Zarr codec the default. Followup tracked in #281

Your CPM calls pass keep_scale_offset=True. The writer will still accept it for one release, with a deprecation warning, so your conversions do not break. Details are in #281.

@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 main, ask for review from both teams, and merge after approval. That way we stay aligned on each change, and the small PRs you propose make this easy.

@emmanuelmathot
emmanuelmathot merged commit 92ac5b5 into main Sep 28, 2026
7 checks passed
@github-actions github-actions Bot mentioned this pull request Sep 28, 2026
@shollendEODC

Copy link
Copy Markdown
Collaborator

thanks for the merge, I will set up the PRs I have been working on now
our approval-testing frameworks currently runs on our buckets, so we will have to modify it a bit for full accessability

lhoupert added a commit that referenced this pull request Sep 28, 2026
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants