Repository navigation
Integrate/cpm geozarr main feat generic conversion s1slc - #292
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 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
…p traversal across metadata-ingestion mdoes
…ozarr) but allows a path to which is needed for rechunking
…-main-feat-generic-conversion-s1slc
…justed error messge from writer
… 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;
…s an atomic product due to SLC size
|
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
left a comment
There was a problem hiding this comment.
See previous comment
emmanuelmathot
left a comment
There was a problem hiding this comment.
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_encodingdiffers from its S2 original on 99 of 117 lines, andwrite_geo_metadataon 123 lines against 64. With both versions onmain, 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_codecreplaceskeep_scale_offset, #281).
Proposed order:
- 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 onmainafter #287. Small fixes:- add
generic_rechunkertooption_namesinvalidate_write_options - add a
--generic-rechunkerflag toeopf convert-geozarr - replace the
asserts with explicit errors
- add
- The uint16 encoding behaviour: discuss it in #281, and add it to #287 if needed, so there is one encoder.
- 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.
|
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. |
|
I will start addressing the recommendations and continuously add commits, will flag you once a review can be done. |
|
Thanks! In the meantime could you add the links to the zarr stores created by this PR? and I can already work with that? |
|
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. |
|
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? |
|
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.. |
Thank you! |
|
Thanks for the feedback #292 (review) I implemented all of your suggestion, it would be ready for another review! @emmanuelmathot |
…ttached -> relevant especially for S3
Alright, for now we have a public bucket where I am currently uploading the processed products: config I will create individual accesses in the bucket ( My current ls result (altough some products are still uploading:) The S3 products are generated with the 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)
|
…n), they got lost during the refactor, also adding/modifing tests to catch the new additin of metadta
|
Thank you @shollendEODC will look at that today! |
|
Thanks @shollendEODC! I ran five of the stores in the
I have to small requests:
Not covered yet: S1 SLC and OCN, and the S3 products. |
|
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.
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.
This is a bit out of my scope but I will inquire with those who set the bucket up! |
|
Thank you @shollendEODC , no problem to wait that this PR merge for the commit tracking. |
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:
consolidate_metadatastrategy for all dataproducts #291An additional goal of this PR is starting a generalization and refactoring of
utils.pyfunctions. Previously, each product processor had its own set of utils functions, which will be (step-by-step) harmonized and (if possible) moved into theeopf-geozarr/conversion/utils.pyscript.The main branch (39b06d6) was merged into this branch and its differences resolved in this branch.
changes
cpm/writer.py
generic_rechunkerwrite option. When set specifically toTrue:s2-optimized, and OLCI products tos3-olci-optimizedthrough thelooks_like_sentinel*functionsgeneric_rechunkerpipeline.False, the oldgenericpipeline remains and passes all products tocreate_geozarr_dataset-> this allows backwards compatibility, while also allowing to trigger thegeneric_rechunkerduring conversion with a generic keyword argumentgenericprocessor and a switch to solely usinggeneric_rechunkerwould be an option, but the current PR aims to allow both functionalities for nowgeneric_optimization/generic_converter.pycreate_generic_geozarr_dataset()walks the DataTree and, for each group that has no child groups and contains dataspatial_chunkmeasurementgroups be consolidated per default or only wherever the multiscales are? Decide on uniformconsolidate_metadatastrategy for all dataproducts #291zarr_conventionsonce they are passed from CPM Specify which CPM root metadata is carried through to GeoZarr output (stac_discovery is stale by construction) #257)stac_discoverymetadata addition (add_multiscale_pyramids_to_stac_metadata(..)) requested and discussed here (Specify which CPM root metadata is carried through to GeoZarr output (stac_discovery is stale by construction) #257) is purposedly not added here, as 1.) no multiscales are generated 2.) it (and the logic for triggering this function call) will be included in the next PRconversion/utils.pys2_optimization,s3_olci_optimization, andconversion/geozarr.pyfolders: 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)s2_optimization/*.pyandutils.pyto maintain compatibility, with existing workflows. Our goal in future PRs would be to harmonize these functions, stripping as much as possible froms2_optimization, ands3_olci_optimizationand move toutils.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 periodTests
tests/test_cpm_writer.pyandtests/test_generic_rechunker.pygeneric_rechunkerfollow-ups