Ingest obs4REF under its own source type - #898
Open
lewisjared wants to merge 10 commits into
Open
Conversation
The obs4REF collection was ingested as obs4MIPs, so the catalog could not show which datasets came from the registry and which from the archive. Ingests it as obs4ref instead. The solver folds the obs4REF catalog into the obs4MIPs one before matching, so diagnostics keep asking for obs4MIPs and a dataset held by both is taken from obs4MIPs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a dedicated Changesobs4REF source support
Sequence Diagram(s)sequenceDiagram
participant Ingest
participant Catalogues
participant Solver
participant Doctor
Ingest->>Catalogues: register obs4MIPs and obs4REF datasets
Solver->>Catalogues: merge missing obs4REF datasets into obs4MIPs requirements
Catalogues-->>Solver: provide source datasets for solving
Doctor->>Solver: evaluate diagnostics against ingested catalogues
Solver-->>Doctor: return executions or unsolvable requirements
Merge Risk: 🟡 Moderate · up to This change separates obs4REF data from obs4MIPs, but several updated catalog fixtures still contain stale integrity hashes, so required catalog checks will fail until the metadata and baseline manifests are regenerated or the failure is explicitly accepted. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed and relevant summary of the implementation and compatibility behaviour, but it omits the required Checklist section and confirmations for tests, documentation, and the changelog. Full details: Docstring CoverageExplanation Docstring coverage is 43.59% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 78 functions across 21 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…naliseable The misfiled check flagged any dataset an obs4REF registry carries, so a CERES-EBAF, GPCP, HadISST or TropFlux copy correctly fetched from ESGF was reported as needing a re-ingest. It now looks only at the directory layout the registry actually produces. The merged obs4MIPs catalog dropped its adapter and database, so an unfinalised dataset could no longer be finalised. It now carries them through, and ref doctor solves against the same catalogs the solver would use.
Shares one helper for unwrapping a catalog, hoists the solver imports in the doctor checks now that there is no cycle to dodge, and merges the catalogs once per unsolvable-diagnostics run rather than once per diagnostic explained. Also promotes normalize_requirement_sets, which the doctor checks now use.
Carrying the obs4MIPs adapter through the merge made the catalog reloadable, and a reload would go back to that adapter alone and drop every obs4REF row just merged in. The merge now carries no adapter, so it cannot reload.
lewisjared
marked this pull request as ready for review
September 2, 2026 07:47
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: c9b4051d-2e9f-4628-b415-2d9956b54af1
⛔ Files ignored due to path filters (2)
tests/test-data/esgf-catalog/obs4mips_catalog.parquetis excluded by!**/*.parquettests/test-data/esgf-catalog/obs4ref_catalog.parquetis excluded by!**/*.parquet
📒 Files selected for processing (66)
changelog/898.feature.mddocs/development.mddocs/getting-started/02-download-datasets.mddocs/getting-started/03-ingest.mddocs/getting-started/quickstart.mddocs/how-to-guides/diagnose-a-deployment.mddocs/how-to-guides/docker_deployment.mdpackages/climate-ref-core/src/climate_ref_core/reference_data.pypackages/climate-ref-core/src/climate_ref_core/summary.pypackages/climate-ref-core/tests/unit/test_datasets.pypackages/climate-ref-core/tests/unit/test_datasets/dataset_collection_obs4mips_hash.ymlpackages/climate-ref-core/tests/unit/test_reference_data.pypackages/climate-ref-core/tests/unit/test_summary.pypackages/climate-ref-example/src/climate_ref_example/surface_temperature.pypackages/climate-ref-example/tests/test-data/global-sst-bias/cmip7/catalog.yamlpackages/climate-ref-example/tests/test-data/global-sst-bias/default/catalog.yamlpackages/climate-ref-ilamb/src/climate_ref_ilamb/standard.pypackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip6/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/burntfractionall-gfed/cmip7/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip6/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/csoil-hwsd2/cmip7/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/gpp-wecann/cmip6/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/gpp-wecann/cmip7/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip6/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/mrro-lora/cmip7/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip6/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/nbp-hoffman/cmip7/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip6/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/snc-esacci/cmip7/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip6/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/so-woa2023-surface/cmip7/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip6/catalog.yamlpackages/climate-ref-ilamb/tests/test-data/thetao-woa2023-surface/cmip7/catalog.yamlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_amoc_rapid_.ymlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_burntfractionall_gfed_.ymlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_csoil_hwsd2_.ymlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_gpp_wecann_.ymlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_mrro_lora_.ymlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_nbp_hoffman_.ymlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_snc_esacci_.ymlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_so_woa2023_surface_.ymlpackages/climate-ref-ilamb/tests/unit/test_solve_regression/test_solve_regression_thetao_woa2023_surface_.ymlpackages/climate-ref-pmp/conftest.pypackages/climate-ref-pmp/src/climate_ref_pmp/diagnostics/enso.pypackages/climate-ref-pmp/src/climate_ref_pmp/diagnostics/variability_modes.pypackages/climate-ref-pmp/tests/test-data/enso_proc/cmip6/catalog.yamlpackages/climate-ref-pmp/tests/test-data/enso_proc/cmip7/catalog.yamlpackages/climate-ref-pmp/tests/test-data/enso_tel/cmip6/catalog.yamlpackages/climate-ref-pmp/tests/test-data/enso_tel/cmip7/catalog.yamlpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npgo/cmip6/catalog.yamlpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npgo/cmip7/catalog.yamlpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pdo/cmip6/catalog.yamlpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pdo/cmip7/catalog.yamlpackages/climate-ref/conftest.pypackages/climate-ref/src/climate_ref/conftest_plugin.pypackages/climate-ref/src/climate_ref/datasets/obs4mips.pypackages/climate-ref/src/climate_ref/doctor/checks/data.pypackages/climate-ref/src/climate_ref/doctor/context.pypackages/climate-ref/src/climate_ref/solver.pypackages/climate-ref/tests/unit/datasets/test_obs4mips/obs4mips_catalog_db.ymlpackages/climate-ref/tests/unit/datasets/test_obs4ref.pypackages/climate-ref/tests/unit/test_doctor.pypackages/climate-ref/tests/unit/test_doctor_registry.pypackages/climate-ref/tests/unit/test_solver.pypackages/climate-ref/tests/unit/test_solver/test_solve_metrics.ymlscripts/generate_esgf_catalog.py
💤 Files with no reviewable changes (1)
- packages/climate-ref/tests/unit/datasets/test_obs4mips/obs4mips_catalog_db.yml
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Tested against a real deployment on Gus, where 81 datasets are affected. One finding listing all of them wrapped into an unreadable blob, and the remedy told the reader to retract 'each of the rows above'. Findings sharing a remedy are already grouped under it once, so one per dataset reads as a list.
Tested against a real deployment on Gus. pmp/enso_tel groups its reference requirement by activity_id, and the adapter now stamps that from the source type, so the reference data split into an obs4MIPs group and an obs4REF group and every model solved twice against half its references. 892 extra executions across the deployment. The merged rows stand in for obs4MIPs data, so they now carry that activity_id. Their instance_id still names obs4REF, so the provenance is not lost. Also catches the ValueError a diagnostic with no data requirements raises, and adds obs4REF to the aggregate test fixture so it can exercise the fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 4c4844fb-e7f0-4f4e-b471-c1fd67a6d810
📒 Files selected for processing (5)
packages/climate-ref/src/climate_ref/conftest_plugin.pypackages/climate-ref/src/climate_ref/doctor/checks/data.pypackages/climate-ref/src/climate_ref/solver.pypackages/climate-ref/tests/unit/test_doctor.pypackages/climate-ref/tests/unit/test_solver.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/climate-ref/src/climate_ref/conftest_plugin.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
…alog A deployment that fetched only the obs4REF registry is the ordinary case, and it took the branch that returned the obs4REF catalog untouched. Those rows kept an obs4REF activity_id, so a requirement grouping by it split or rejected them. Normalisation now happens on the one path every added row takes. Also normalises path separators before matching a collection directory.
Contributor
Author
|
@coderabbitai review |
|
Mid-upgrade the re-ingested obs4REF row and the old misfiled obs4mips row sit side by side, and every one was reported as superseded. That told the user to retract exactly the row misfiled-obs4ref asks them to keep. Only a genuine obs4MIPs publication counts as one now.
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
1 participant
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.
Ingests the obs4REF collection under its own
obs4refsource type instead of folding it into obs4MIPs at ingest time. The catalog now records which datasets came from the registry and which from the ESGF archive.The diagnostics are unchanged and still declare obs4MIPs requirements. The solver folds the obs4REF catalog into the obs4MIPs one just before matching, so a dataset held by both is taken from obs4MIPs and obs4REF fills in the rest. Publishing a dataset to obs4MIPs therefore takes over from the registry copy with no change to the REF.
Worth a close look:
obs_dataset_keydecides when two rows are the same dataset by stripping the collection prefix and the version off theinstance_id. That is the whole basis of the tie break.activity_id. obs4REF republishes obs4MIPs files unchanged, so the file attribute cannot tell the collections apart. The adapter now setsactivity_idfrom the source type and only warns when the paths look misfiled.check_unsolvable_diagnosticsruns the solver fromref doctor, which is a heavier check than the others in that module.Data ingested with
--source-type obs4mipsin earlier releases still solves.ref doctorflags it undermisfiled-obs4refand recommends a re-ingest, andsuperseded-obs4reflists registry copies that obs4MIPs has since published.Summary by CodeRabbit
New Features
ref doctorchecks for misfiled, superseded, and unsolvable data.Bug Fixes
Documentation