Skip to content

Integrate/cpm geozarr main feat generic conversion s1slc - #292

Merged
emmanuelmathot merged 50 commits into
EOPF-Explorer:mainfrom
shollendEODC:integrate/cpm-geozarr-main-feat-generic-conversion-s1slc
Oct 12, 2026
Merged

emmanuelmathot merged 50 commits into
EOPF-Explorer:mainfrom
shollendEODC:integrate/cpm-geozarr-main-feat-generic-conversion-s1slc

Conversation

@shollendEODC

Copy link
Copy Markdown
Collaborator

Summary

This PR introduces an additional generic processor suited to rechunking and sharding any CPM 3.0.0 product without the addition of geozarr multiscales. This was designed for all EOPF products without their own optimized pipeline, specific tests/fixtures were implemented with a sample of S1 GRD zarr v3. This package was tested against all products (manually and outside of this framework to not explode the test suite) and ran through as expected. If merged, we could (theoretically, after testing and resolving missed issued etc.) generate non-multiscale data for all products listed here #248 (also for S3 products if we omit the generation of multiscales, which still needs to be investigated and decided).

This PR would (at least from eodc's side) address these issues for the generic conversion:

An additional goal of this PR is starting a generalization and refactoring of utils.py functions. Previously, each product processor had its own set of utils functions, which will be (step-by-step) harmonized and (if possible) moved into the eopf-geozarr/conversion/utils.py script.

The main branch (39b06d6) was merged into this branch and its differences resolved in this branch.

changes

cpm/writer.py

  • New generic_rechunker write option. When set specifically to True:
    • S2 products still route to s2-optimized, and OLCI products to s3-olci-optimized through the looks_like_sentinel* functions
    • All other products route to the new generic_rechunker pipeline.
  • If set to False, the old generic pipeline remains and passes all products to create_geozarr_dataset -> this allows backwards compatibility, while also allowing to trigger the generic_rechunker during conversion with a generic keyword argument
  • If agreed upon by all users, a removal of the generic processor and a switch to solely using generic_rechunker would be an option, but the current PR aims to allow both functionalities for now

generic_optimization/generic_converter.py

conversion/utils.py

  • Introduced functions were harmonized by taking them from the s2_optimization, s3_olci_optimization, and conversion/geozarr.py folders: they were copied, modified, and then validated to work on all products (->this involved JUST running the code through, without a dedicated test suite; tests will be added once the individual products are added)
  • Functions exist as duplicates between s2_optimization/*.py and utils.py to maintain compatibility, with existing workflows. Our goal in future PRs would be to harmonize these functions, stripping as much as possible from s2_optimization, and s3_olci_optimization and move to utils.py. This will make development much easier (at least on my side :) ) as there will be less clutter lying around once completed. This comes at the cost of writing the functions adaptable for different products, but i consider it worth the effort, For now duplicated functionality is acceptable from my side to keep maintainability during this transformation period

Tests tests/test_cpm_writer.py and tests/test_generic_rechunker.py

  • These tests were written by Claude, so there might be some conceptual mistakes in there. Tried to validate them as best as i could
  • Updated for the new changes to the writer.py script
  • addition of a new test script for the generic_rechunker

follow-ups

  • Groups that hold both variables and child groups are skipped, only groups without child groups are converted, so variables on such a group are not written! This impacts mainly S3 OLCI EFR (which theoretically has its own pipeline), but a change of this logic might still be worthwhile (src/eopf_geozarr/generic_optimization/generic_converter.py Line:72)

d-v-b and others added 21 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 EOPF-Explorer#212 (Sentinel-3 OLCI GeoZarr export) merged to main after EOPF-Explorer#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 EOPF-Explorer#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 EOPF-Explorer#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 EOPF-Explorer#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 (EOPF-Explorer#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 (EOPF-Explorer#216), release 0.11.0 (EOPF-Explorer#235) and dependency
bumps (EOPF-Explorer#268, EOPF-Explorer#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 (EOPF-Explorer#275, fixes EOPF-Explorer#266).

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ition of generalised functionality to utils which are not used in other functions
…ozarr) but allows a path to which is needed for rechunking
… due to utils.create_uniform_encoding, Requesting input to check if tests or encoding needs to be adapted (in sight of new scale/offset codec)
…float32 per default if keep_scale_offset=False;
Comment thread src/eopf_geozarr/cpm/writer.py Fixed
@emmanuelmathot

Copy link
Copy Markdown
Contributor

Thx for the substantive PR @shollendEODC. Could we separate it and keep utils refactoring in a second one. I am still trying to figure out some of the new introduced features.

@emmanuelmathot emmanuelmathot left a comment

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.

See previous comment

@emmanuelmathot emmanuelmathot left a comment

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.

Thanks @shollendEODC. We want the generic rechunker, and the opt-in design is good. As agreed in #277 (small PRs, easier to review), could you split this PR? Three reasons:

  • The PR mixes a feature and a refactor. The pipeline itself is about 250 lines. The rest is 17 functions (+880 lines) added to conversion/utils.py. The pipeline needs 10 of them; the other 7 (249 lines) are not used in this PR.
  • The copied functions are forks. create_uniform_encoding differs from its S2 original on 99 of 117 lines, and write_geo_metadata on 123 lines against 64. With both versions on main, S2 and the generic pipeline would behave differently, and every fix, such as nodata masking (#246) or pixel edges (#275), would be needed twice.
  • The encoding change overlaps with #287 (scale_offset_codec replaces keep_scale_offset, #281).

Proposed order:

  1. This PR: the generic rechunker only. generic_converter.py, the writer option, the routing, the S1 GRDH fixture and tests, plus the helpers the pipeline needs. Rebase it on main after #287. Small fixes:
    • add generic_rechunker to option_names in validate_write_options
    • add a --generic-rechunker flag to eopf convert-geozarr
    • replace the asserts with explicit errors
  2. The uint16 encoding behaviour: discuss it in #281, and add it to #287 if needed, so there is one encoder.
  3. The helper harmonisation: small PRs, one per family (chunking and encoding, geo metadata and transforms, summaries and STAC). Each PR moves the functions to utils.py, so the originals are deleted, and shows with the S2 snapshot test and the OLCI tests that the output does not change.

@shollendEODC

Copy link
Copy Markdown
Collaborator Author

Before anyone starts a serious review I will address the 3.12 test failing (local pytest for 3.13 passed) and check if #275 was applied correctly to the reimplemented functions. Will trigger a request for review once its completed.

@shollendEODC

Copy link
Copy Markdown
Collaborator Author

I will start addressing the recommendations and continuously add commits, will flag you once a review can be done.

@lhoupert

lhoupert commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Thanks! In the meantime could you add the links to the zarr stores created by this PR? and I can already work with that?

@shollendEODC

Copy link
Copy Markdown
Collaborator Author

Yes, I will generate some samples covering all EOPF Sample Service data products today (S1 without RTC, S3 without any multiscales) for the STAC ingestion. I will provide some info on how you can access them tomorrow midday, once we have decided on the easiest method for (continuously) providing generated samples.

@lhoupert

lhoupert commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

I am wondering also if we could not provide the instructions somewhere on how to generate example zarr store from the raw data so anyone could just run that same conversion locally . Would it be easy, do you have these instructions already somewhere?

@shollendEODC

shollendEODC commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thats how I do it locally for testing, .SAFE files are downloaded from CDSE (they are used in our pipeline). Providing it would be easy I think (its in my main.py)

I just comment out the products I dont want for now..

import logging
import time
from pathlib import Path
from typing import Any

import xarray as xr
from eopf.store.convert import convert

logging.disable(logging.CRITICAL)


def convert_sample_files(
    safe_dir: Path, zarr_dir: Path, filelist: dict[str, str], output_prefix: str | None = None
) -> None:
    zarr_dir.mkdir(exist_ok=True)

    target_store_kwargs = {
        "engine": "geozarr",
        "spatial_chunk": 1024,
        "enable_sharding": True,
        "scale_offset_codec": False,
        "generic_rechunker": True,
    }

    source_store_kwargs: dict[str, Any] = {"engine": "safe", "mode": "r"}

    for name, file in filelist.items():
        if output_prefix:
            outfn = Path(f"{output_prefix}_{file}").with_suffix(".zarr")
        else:
            outfn = Path(file).with_suffix(".zarr")

        print("-" * 60)
        print(f"Processing {name}")

        try:
            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,
            )
        except Exception as e:
            print(f"ERROR: {e}")



if __name__ == "__main__":
    # add their geozarr writer to the eopf registry
    from eopf_geozarr.cpm.writer import register

    register()

    files = {
        # "S2C_MSIL1C": "S2A_MSIL1C_20260721T102041_N0512_R065_T32UPU_20260721T153621.SAFE", # full L1C tile
        # "S2C_MSIL2A": "S2C_MSIL2A_20260716T100601_N0512_R022_T32UQU_20260716T152417.SAFE", # full L2A tile
        # "S1A_IW_GRDH": "S1A_IW_GRDH_1SDV_20240201T164915_20240201T164940_052368_065517_750E.SAFE",
        # "S1B_EW_GRDM": "S1B_EW_GRDM_1SDV_20210528T082042_20210528T082142_027102_033CCF_D55D.SAFE",
        # "S1C_IW_OCN": "S1C_IW_OCN__2SDV_20260823T051821_20260823T051846_009118_0121A3_CC3B.SAFE",
        # "S1C_IW_SLC": "S1C_IW_SLC__1SDV_20260731T234807_20260731T234821_008794_0116E8_C079.SAFE",
        # "S2C_MSIL1C": "S2C_MSIL1C_20260909T124301_N0512_R095_T27WXN_20260909T143930.SAFE", # smaller L1C tile
        "S2C_MSIL2A": "S2B_MSIL2A_20260721T100559_N0512_R022_T33UWQ_20260721T143508.SAFE",
        # "S3A_OL_1_EFR": "S3B_OL_1_EFR____20260923T093057_20260923T093357_20260923T113901_0179_125_036_2160_ESA_O_NR_004.SEN3",
        # "S3B_OL_1_ERR": "S3B_OL_1_ERR____20260728T071230_20260728T075648_20260728T095240_2658_122_377______ESA_O_NR_004.SEN3",
        # "S3A_OL_2_LFR": "S3A_OL_2_LFR____20260916T095057_20260916T095357_20260916T120650_0179_144_079_2160_PS1_O_NR_003.SEN3",
        # "S3A_OL_2_LRR": "S3A_OL_2_LRR____20260915T100712_20260915T105125_20260915T122949_2653_144_065______PS1_O_NR_003.SEN3",
        # "S3A_SL_1_RBT": "S3A_SL_1_RBT____20241224T203525_20241224T203825_20241226T064306_0179_120_328_0900_PS1_O_NT_004.SEN3",
        # "S3A_SL_2_LST": "S3A_SL_2_LST____20240915T072403_20240915T072703_20240915T094328_0179_117_049_2340_PS1_O_NR_004.SEN3",
        # "S3A_SL_2_FRP": "S3A_SL_2_FRP____20260917T092446_20260917T092746_20260917T112506_0180_144_093______MAR_O_NR_003.SEN3",
    }
    output_folder = Path("...")
    output_prefix: str = "test_"

    convert_sample_files(
        safe_dir=Path("..."),
        zarr_dir=output_folder,
        filelist=files,
        output_prefix=output_prefix,
    )

@lhoupert

lhoupert commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Yes, I will generate some samples covering all EOPF Sample Service data products today (S1 without RTC, S3 without any multiscales) for the STAC ingestion. I will provide some info on how you can access them tomorrow midday, once we have decided on the easiest method for (continuously) providing generated samples.

Thank you!

@shollendEODC

Copy link
Copy Markdown
Collaborator Author

Thanks for the feedback #292 (review)

I implemented all of your suggestion, it would be ready for another review! @emmanuelmathot

@shollendEODC

shollendEODC commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thanks! In the meantime could you add the links to the zarr stores created by this PR? and I can already work with that?

Alright, for now we have a public bucket where I am currently uploading the processed products:

config

[profile eodc-cm]
endpoint_url = https://objects.eodc.eu

I will create individual accesses in the bucket (continuous-integration) for each PR-related branch: eopf-geozarr/branches/cpm-geozarr-main-feat-generic-conversion-s1slc/

My current ls result (altough some products are still uploading:)

$ aws s3 --profile eodc-cm --no-sign-request ls s3://continuous-integration/eopf-geozarr/branches/cpm-geozarr-main-feat-generic-conversion-s1slc/
                           PRE S1A_IW_GRDH_1SDV_20240201T164915_20240201T164940_052368_065517_750E.zarr/
                           PRE S1B_EW_GRDM_1SDV_20210528T082042_20210528T082142_027102_033CCF_D55D.zarr/
                           PRE S1C_IW_OCN__2SDV_20260823T051821_20260823T051846_009118_0121A3_CC3B.zarr/
                           PRE S1C_IW_SLC__1SDV_20260731T234807_20260731T234821_008794_0116E8_C079.zarr/
                           PRE S2B_MSIL2A_20260721T100559_N0512_R022_T33UWQ_20260721T143508.zarr/
                           PRE S2C_MSIL1C_20260909T124301_N0512_R095_T27WXN_20260909T143930.zarr/
                           PRE S3B_OL_1_EFR____20260923T093057_20260923T093357_20260923T113901_0179_125_036_2160_ESA_O_NR_004.zarr/

The S3 products are generated with the generic_rechunker and not the olci_optimized one as its not fully developed right now.

Ping me if you need anything else :) @lhoupert

Edit: There is also an experimental S2L2A product with multiscales for cloud, snow, l2a and l1c_classification available. It was generated from another pre-PR staging branch (https://github.com/shollendEODC/data-model/tree/integrate/cpm-geozarr-main-feat-generic-conversion-s2-cld-snw-scl-MS)

aws s3 --profile eodc-cm --no-sign-request ls s3://continuous-integration/eopf-geozarr/branches/cpm-geozarr-main-feat-generic-conversion-s2-cld-snw-scl-MS/

…n), they got lost during the refactor, also adding/modifing tests to catch the new additin of metadta
@lhoupert

lhoupert commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Thank you @shollendEODC will look at that today!

@lhoupert

lhoupert commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Thanks @shollendEODC! I ran five of the stores in the continuous-integration bucket through our acceptance checks:

I have to small requests:

  1. Could each store record the commit that produced it (e.g. in processing_history)? If would help me to track which version I tested if stores are rewritten in place
  2. Could the bucket answer 404 instead of 403 for missing keys? Some clients, e.g. OpenLayers ≤ 10.10, treat a 403 as an error.

Not covered yet: S1 SLC and OCN, and the S3 products.

@shollendEODC

Copy link
Copy Markdown
Collaborator Author

great news, as i just answered in EOPF-Explorer/data_model_validation_tests#8, the addition of other multiscales and product specific changes will be applied once this PR is merged.

Could each store record the commit that produced it (e.g. in processing_history)? If would help me to track which version I tested if stores are rewritten in place

Yes! This is already implemented in https://github.com/shollendEODC/data-model/tree/integrate/cpm-geozarr-main-feat-add-geozarr-version , the goal would be to make a PR for it once this one is merged, but if its required before that we can make a separate PR with just this feature. Its branched of this PRs branch, so some cherrypicking would need to be done.

Could the bucket answer 404 instead of 403 for missing keys? Some clients, e.g. OpenLayers ≤ 10.10, treat a 403 as an error.

This is a bit out of my scope but I will inquire with those who set the bucket up!

@lhoupert

lhoupert commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Thank you @shollendEODC , no problem to wait that this PR merge for the commit tracking.

@emmanuelmathot
emmanuelmathot self-requested a review October 12, 2026 01:49
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.

6 participants