Repository navigation
Adding refactoring and Harmonization of Feat/harmonize root consolidation - #314
Closed
shollendEODC wants to merge 61 commits into
Closed
shollendEODC wants to merge 61 commits into
shollendEODC wants to merge 61 commits into
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 <emmanuel.mathot@gmail.com>
…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 <noreply@anthropic.com>
Brings in the spatial:bbox pixel-edge fix (EOPF-Explorer#275, fixes EOPF-Explorer#266). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…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
…ate_write_options', removing assert and introducing new pass-through error
…esult_datatree, rechunk_dataset_for_encoding, get_chunking_for_encoding), keeping functions from s2 processing (simple_root_consolidation, stream_write_dataset->stream_write_s2dataset)
…eeds to be discussed
…_store_root_geo_metadata() which uses shapely for bounds calculation from geojson geometry
…s to mutliscale ingestion
…ersion-s2-cld-snw-scl-MS
…sues remaining in S3
…unctions -> induced changes in other places as well
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…m spatial:transform or array.rio accessor
shollendEODC
marked this pull request as ready for review
October 14, 2026 08:40
Contributor
|
Thanks @shollendEODC! one consolidation path for every product will make #249 much easier to follow! A few points :
Happy to run our acceptance checks on the test stores if you can upload some the same way you did it for 292. Thanks! |
Collaborator
Author
|
adding bugfix for potential S3 SLSTR during our acceptance checks for consolidation #315 |
Collaborator
Author
Collaborator
Author
|
@lhoupert Files are currently being uploaded to s3://continuous-integration/eopf-geozarr/branches/feat-harmonize-root-consolidation/ and are processed with this commit 7dd1af1 These samples include ONLY S1 and S2 products, as S3 would route through the Please refer to my last comment in #249 for updates regarding the formalization of rules for attributes and consolidation. |
emmanuelmathot
left a comment
emmanuelmathot
left a comment
Contributor
There was a problem hiding this comment.
Great work @shollendEODC ! Thx! Please see my comments and suggestions below
lhoupert
previously approved these changes
Oct 26, 2026
lhoupert
dismissed
their stale review
October 27, 2026 15:34
Posted on the wrong PR by mistake; meant for #318.
claxn
added a commit
that referenced
this pull request
Oct 29, 2026
… (supersedes #314) (#328) * feat(cpm): add CPM writer plugin registering the geozarr engine 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 * fix(cpm): apply adversarial-review findings to the writer plugin 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 * ci(cpm): run CPM integration tests in CI and add a CLI end-to-end test - 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 * feat(cpm): route Sentinel-3 OLCI products to the optimized pipeline 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 (#254) * 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. * Cpm geozarr eodc changes (#247) * 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 <emmanuel.mathot@gmail.com> * adding single /measurement/refectance group to stac as requested during 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 * first explorative introduction of the generic geozarr processing. Addition of generalised functionality to utils which are not used in other functions * swiching consolodation methodology * updating the processing of consolidated metadata and harmonizing group traversal across metadata-ingestion mdoes * removal of functions from utils.py which are not essential for this PR * adding new routing which keeps the original routing same (allows ->geozarr) but allows a path to which is needed for rechunking * small restructure of Error raising in writer, adjust test to catch adjusted error messge from writer * updating docstrings and removing unneeded root consolidation function * adding tests for generic converter logic, two tests are still failing 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) * configuring create_uniform_encoding(..) to NOT cast uint16 inputs to float32 per default if keep_scale_offset=False; * adding S1_GRDH zarrv3 fixture instead of SLC as it can be contained as an atomic product due to SLC size * removal of unneccassary fucntions from utils.py * adding 'generic_rechunker' flags to eopf 'convert-geozarr' and 'validate_write_options', removing assert and introducing new pass-through error * removing from s2_multiscales.py and all associated changes * removing from s2_multiscales.py and all associated changes - forgot 2 scripts * removing functions from s2 processing (optimization_summary, create_result_datatree, rechunk_dataset_for_encoding, get_chunking_for_encoding), keeping functions from s2 processing (simple_root_consolidation, stream_write_dataset->stream_write_s2dataset) * reverting uv.lock changes to 39b06d6 * fixing tests-changes induced by changes in code, NOT fixing as this needs to be discussed * add shapely to dependency so tests dont fail as they call utils.write_store_root_geo_metadata() which uses shapely for bounds calculation from geojson geometry * switch to new encoding logic, move of functions to new file to keep encoding atomic and separated, inclusion of new encoding logic and coordinate are not compressed anymore in create_uniform_encoding() * adding scipy to dependencies for SLC conversion, also updating uv.lock for this change * modifying tests to reflect switch to scale_offset_codec * removal of * removal of * moving 'valid_min' and 'valid_max' to encoding_utils.py/_decode_packed() to only be removed during flaot decoding and keep for int dtypes * remodelling of chunking functions, small additions * adding to for harmonization * adressing requested cahgnes in utils.py/updated_root_consolidaiton to improve readabillity, small associated refactors in other files due to function name changes * removing check which was only valid for enable_sharding=True * removing raster:scale and raster:offset * regenerating S2 fixtures * simplifying create_uniform_encoding() for variable and coordinate chunk/sharding * small updates to utils and tests to reflect changes * not working, erroneous commit for s2 cld/snw/scl-multiscales and measureemtn multiscales harmonisation * not working, erroneous commit for s2 cld/snw/scl-multiscales and measureemtn multiscales harmonisation * fixing bugs in MS calculation, tests/test_s2_multiscale.py::test_create_multiscale_from_datatree_snapshot, tests/test_s2_multiscale.py::test_create_multiscale_from_datatree_behavior failing duie to missing geozarr attrs before MS calculation * first addition of cld&snw and l1c&l2a_classification multiscales * adding ms bands addtion to stac_discovery similar to 6f9476a * small refactrs of s2_multiscale.py, restructure of determine_variable_type() * applying changes to tests and derived changes&bugfixes&simplifications to mutliscale ingestion * adding consolidatin for MS groups * stash and test updated_root_consolidation * working status of root consolidation for all products, with slight issues remaining in S3 * switching globvally to updated uniform root_consodlidation * massive claude assisted refactor and reprocessing of the Sentinel-2 functions -> induced changes in other places as well * re-outsourcing the S2-stac adding logic to own function as its independent * adding hotfix implemted in 655e5e6 for retrieval of spatial attrs from spatial:transform or array.rio accessor * removal of commented code, improving docstrings, sorting helper functions in utils * addressing non-consolidation of slstr in potential generic conversion * refactor: updated all tests and addressed all review comments * build: updated msgpack version to 1.2.4 * docs: function call updates and removed max_entries * fix: pass compression_level through the chain * test: added test_parent_measurements_group_is_consolidated * refactor!: changed geo utility function return behaviour to guarded behaviour with deterministic return types --------- Co-authored-by: Davis Vann Bennett <davis.v.bennett@gmail.com> Co-authored-by: Emmanuel Mathot <emmanuel.mathot@gmail.com> Co-authored-by: shollendEODC <samuel.hollendonner@eodc.eu> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: claudio.schein-navacchi <claudio.schein-navacchi@eodc.eu>
Collaborator
|
Can be closed now, since the PR superseding it was merged (#328). |
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.
This PR can be merged/adressed after the following PRs are merged:
1.) #311
2.) #310
3.) #313
This PR achieves two main goals:
Theoretically, no changes to the output of products should be applied, the S2 products (as well as all other products) should match previous outputs. As this PR is based on #313, the changes of fixtures and tests is still applied. The git logic might be a bit confusing here, as I am preparing several PRs at once, but no fixtures were changes as part of this PR. The tests run through, which validates the adherence to previous data models.
655e5e6 is added to this PR to resolve known errors in the test-cpm stage.
Overview of changes:
utils.py: addition of functions from S2 scripts (add_multiscale_pyramids_to_stac_metadata,write_dataset_geo_metadata, spatial helper functions) and rework ofupdated_root_consolidationwriter.py,generic_converter.py, ``: removal of argumentsenable_sharding, `max_retries`common.py: deletion, no application anymores2_converter.py: removal of functions toutils.py, addition of S2 specific helper functions and deletion ofsimple_root_simplifications2_multiscale.py: addition of multiscale computation from Addition of multiscale pyramids for cloud/snow probability and classification layers #313, removal of helper functions and deletion of functions not used anymores2_resampling.py: same changes as in Addition of multiscale pyramids for cloud/snow probability and classification layers #313cli.py: refactor due toe argument removaltest_s2_converter_simplified.py,test_s2_multiscale.py,test_s2_multiscale_geo_metadata.py,test_s2_resampling.py: refactoring due to function changes, restructuring to adhere to multiscale generation procedure as in normal workflow