feat: ingest ESMValTool reference datasets for provenance#790
feat: ingest ESMValTool reference datasets for provenance#790lewisjared wants to merge 3 commits into
Conversation
Reference (observational/reanalysis) datasets used by ESMValTool diagnostics are ingested into the database under a dedicated `esmvaltool-reference` dataset type during `ref providers setup`. Because this data is not CMOR/obs4MIPs compliant, its metadata is parsed from the ESMValTool OBS/OBS6, native6 and obs4MIPs layout conventions rather than from file global attributes. Diagnostics now declare the reference datasets they use via a `reference_datasets` specification instead of hardcoding them in recipe construction, and each execution records the reference datasets it compared against so they appear alongside the model datasets in the execution's dataset list. The near-identical obs4MIPs and PMP climatology dataset models are consolidated onto a shared ReferenceDatasetMixin while remaining distinct dataset types.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an ChangesESMValTool reference dataset provenance
Sequence Diagram(s)sequenceDiagram
participant ESMValToolProvider
participant ESMValToolReferenceDatasetAdapter
participant parse_esmvaltool_reference
participant Database
ESMValToolProvider->>ESMValToolReferenceDatasetAdapter: ingest_data(config, db)
ESMValToolReferenceDatasetAdapter->>parse_esmvaltool_reference: parse file path
parse_esmvaltool_reference-->>ESMValToolReferenceDatasetAdapter: metadata or INVALID_ASSET
ESMValToolReferenceDatasetAdapter->>Database: store reference datasets
ESMValToolReferenceDatasetAdapter-->>ESMValToolProvider: ingestion stats
sequenceDiagram
participant Solver
participant Diagnostic
participant reference_provenance
participant Database
Solver->>Diagnostic: reference_dataset_selectors()
Diagnostic-->>Solver: selectors
Solver->>reference_provenance: link_reference_datasets(db, execution, diagnostic)
reference_provenance->>reference_provenance: resolve_reference_dataset_ids(db, selectors)
reference_provenance->>Database: query matching reference datasets
Database-->>reference_provenance: dataset ids
reference_provenance->>Database: insert execution_datasets links
reference_provenance-->>Solver: new link count
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
packages/climate-ref/src/climate_ref/models/dataset.py (1)
316-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the ESMValTool reference fields optional.
data_type,tier,long_name, andunitsare nullable in the table, so annotate them asNone-able to match what callers can actually receive.packages/climate-ref-esmvaltool/src/climate_ref_esmvaltool/__init__.py (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
TYPE_CHECKING-guardedDatabasehint instead ofAny.
db: Anysacrifices type-checking on the ingestion call for a param that's always aDatabaseat runtime. ATYPE_CHECKING-only import preserves the optional-dependency behaviour while giving static type coverage.Proposed refactor
-from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from climate_ref.database import Database ... - def ingest_data(self, config: Config, db: Any) -> None: + def ingest_data(self, config: Config, db: "Database") -> None:packages/climate-ref/src/climate_ref/reference_provenance.py (1)
24-59: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftSelector resolution re-queries the DB per execution instead of caching per diagnostic.
reference_dataset_selectors()is fixed per diagnostic (seeclimate_ref_esmvaltool/diagnostics/base.py), butresolve_reference_dataset_idsis invoked fresh for every execution built insolve_required_executions's loop (per thesolver.pysnippet showinglink_reference_datasetscalled right afterregister_datasetsfor each execution). For diagnostics with many executions, this repeats identical queries (and identical "no match" warnings) once per execution rather than once per diagnostic.Consider resolving/caching selector → dataset-id results once per diagnostic (or per unique selector set) in the solver loop, rather than per execution.
packages/climate-ref-core/src/climate_ref_core/diagnostics.py (1)
417-439: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
ReferenceDatasetSelectoris frozen but not truly hashable due to thefacetsdict field.
@frozen(attrs) auto-generates a__hash__that includes all fields wheneq=True. Sincefacetsis typically a plaindict,hash(selector)will raiseTypeError: unhashable type: 'dict'if any code ever needs to put these selectors in asetor use them as dict keys (e.g. dedup logic). This isn't exercised by the visible code today, but it's a common attrs footgun worth guarding against before this type is used more broadly for provenance dedup.Confirmed via attrs docs: with
frozen=True, attrs "will write a__hash__function for you automatically" that hashes field values, and dict fields are inherently unhashable.♻️ Option: make hashing explicit-safe or use a hashable facets representation
-@frozen +@frozen(eq=False) # or: keep eq for value equality but avoid hash generation issues class ReferenceDatasetSelector: ... - facets: Mapping[str, str] + facets: Mapping[str, str] # consider tuple[tuple[str, str], ...] if hashability is ever needed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d405ebf-2cfb-4052-95ee-5a6711047d3b
📒 Files selected for processing (23)
changelog/+esmvaltool-reference-datasets.feature.mdpackages/climate-ref-core/src/climate_ref_core/diagnostics.pypackages/climate-ref-core/src/climate_ref_core/source_types.pypackages/climate-ref-esmvaltool/src/climate_ref_esmvaltool/__init__.pypackages/climate-ref-esmvaltool/src/climate_ref_esmvaltool/diagnostics/base.pypackages/climate-ref-esmvaltool/src/climate_ref_esmvaltool/diagnostics/enso.pypackages/climate-ref-esmvaltool/src/climate_ref_esmvaltool/diagnostics/reference.pypackages/climate-ref-esmvaltool/src/climate_ref_esmvaltool/diagnostics/regional_historical_changes.pypackages/climate-ref-esmvaltool/src/climate_ref_esmvaltool/diagnostics/sea_ice_area_basic.pypackages/climate-ref-esmvaltool/tests/unit/diagnostics/recipes/recipe_enso_characteristics_cmip6.ymlpackages/climate-ref-esmvaltool/tests/unit/diagnostics/recipes/recipe_enso_characteristics_cmip7.ymlpackages/climate-ref-esmvaltool/tests/unit/diagnostics/recipes/recipe_regional_historical_trend_obs4mips.ymlpackages/climate-ref-esmvaltool/tests/unit/diagnostics/test_reference.pypackages/climate-ref-esmvaltool/tests/unit/test_provider.pypackages/climate-ref/src/climate_ref/datasets/__init__.pypackages/climate-ref/src/climate_ref/datasets/esmvaltool_reference.pypackages/climate-ref/src/climate_ref/migrations/versions/2026-07-03T0000_a7b8c9d0e1f2_add_esmvaltool_reference_dataset.pypackages/climate-ref/src/climate_ref/models/dataset.pypackages/climate-ref/src/climate_ref/reference_provenance.pypackages/climate-ref/src/climate_ref/solver.pypackages/climate-ref/tests/unit/datasets/test_datasets.pypackages/climate-ref/tests/unit/datasets/test_esmvaltool_reference.pypackages/climate-ref/tests/unit/test_reference_provenance.py
| # Record reference (observational) datasets the diagnostic compares against. | ||
| # These are provenance only: they are not solver inputs and do not affect the | ||
| # dataset hash or the data passed to the diagnostic. | ||
| link_reference_datasets(db, execution, potential_execution.diagnostic) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Backfill provenance for up-to-date executions too.
link_reference_datasets only runs when a fresh Execution is created. Existing up-to-date groups continue before this call, so running ref providers setup after an execution can leave the now-resolved reference datasets absent from provenance. Consider linking the latest existing execution before the skip path, or adding an explicit backfill during reference ingestion.
There was a problem hiding this comment.
Deferring — this is a deliberate scope decision. This PR records provenance for executions as they are created; backfilling reference datasets onto already up-to-date executions when ref providers setup is re-run afterwards is a larger behavioural change (a backfill pass over existing executions) that belongs in the follow-up alongside the read/API surface. Noting it there.
There was a problem hiding this comment.
@lewisjared, understood — deferring the backfill to the follow-up PR alongside the read/API surface makes sense given it's a distinct behavioural change (a pass over existing executions) rather than a natural extension of provenance-on-creation. Thanks for clarifying the scope boundary.
✏️ Learnings added
Learnt from: lewisjared
Repo: Climate-REF/climate-ref PR: 790
File: packages/climate-ref/src/climate_ref/solver.py:786-789
Timestamp: 2026-07-06T04:40:24.135Z
Learning: In climate-ref's `packages/climate-ref/src/climate_ref/solver.py`, `link_reference_datasets` is intentionally only called when a fresh `Execution` is created during `solve_required_executions`. Backfilling reference-dataset provenance onto already up-to-date execution groups (e.g., when `ref providers setup` is re-run after diagnostics were already executed) is a deliberate out-of-scope decision for the ESMValTool reference dataset provenance PR, to be addressed in a follow-up PR alongside the read/API surface for reference datasets.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- Migration a7b8c9d0e1f2 now widens the SQLite dataset_type VARCHAR to fit the new ESMValToolReference enum member, fixing the migrations-up-to-date integration check (Postgres was already handled via ALTER TYPE). - _parse_obs reads the timerange from the trailing token, matching _parse_obs4mips, so an unexpected extra segment does not drop the date range. - _parse_obs4mips rejects an empty filename stem instead of ingesting an empty variable_id. - Tighten the no-match provenance test to assert the emitted warning.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 55e4a0c2-b034-4e24-99f9-df167ff6be02
📒 Files selected for processing (4)
packages/climate-ref/src/climate_ref/datasets/esmvaltool_reference.pypackages/climate-ref/src/climate_ref/migrations/versions/2026-07-03T0000_a7b8c9d0e1f2_add_esmvaltool_reference_dataset.pypackages/climate-ref/tests/unit/datasets/test_esmvaltool_reference.pypackages/climate-ref/tests/unit/test_reference_provenance.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/climate-ref/tests/unit/test_reference_provenance.py
- packages/climate-ref/tests/unit/datasets/test_esmvaltool_reference.py
- packages/climate-ref/src/climate_ref/datasets/esmvaltool_reference.py
| op.drop_table("esmvaltool_reference_dataset") | ||
|
|
||
| # On SQLite, narrow ``dataset_type`` back to the previous enum member set. On PostgreSQL the | ||
| # ``ESMValToolReference`` value is intentionally left in the enum type: PostgreSQL does not | ||
| # support removing a value from an enum without recreating the type, and leaving it is harmless. | ||
| bind = op.get_bind() | ||
| if bind.dialect.name != "postgresql": |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove parent dataset rows before dropping the subtype table.
esmvaltool_reference_dataset is joined to dataset.id, so dropping only the child table can leave parent rows with dataset_type = 'ESMValToolReference'. On non-PostgreSQL, the later batch alter removes that enum member and can fail while copying existing rows; even where it succeeds, the downgraded app inherits unknown dataset rows.
🐛 Proposed downgrade fix
def downgrade() -> None:
+ op.execute(
+ sa.text(
+ "DELETE FROM dataset "
+ "WHERE dataset_type = :dataset_type"
+ ).bindparams(dataset_type="ESMValToolReference")
+ )
+
with op.batch_alter_table("esmvaltool_reference_dataset", schema=None) as batch_op:
batch_op.drop_index(batch_op.f("ix_esmvaltool_reference_dataset_variable_id"))
Mirror the PMP provider's ingest_data tests (skip-when-climate-ref-absent, missing data directory, no valid datasets, and the success path) so the new reference-data ingestion glue is exercised.
1 participant
Description
Surfaces the reference (observational/reanalysis) datasets that ESMValTool diagnostics use into the database, so they can be recorded for provenance and shown in the frontend alongside the model datasets an execution ran against. This is the first step of the reference-dataset work; the API/frontend surfacing comes in a follow-up.
Ingestion. A dedicated
esmvaltool-referencedataset type is ingested duringref providers setup. Because this data is not CMOR/obs4MIPs compliant, its metadata is parsed from the ESMValToolOBS/OBS6,native6andobs4MIPslayout conventions (DRS path + filename) rather than from file global attributes. This is why it is a deliberately separate dataset type from obs4MIPs, rather than reusing that model.Declarative reference datasets. Diagnostics now declare the reference datasets they compare against via a
reference_datasetsspecification, instead of hardcoding them inside recipe construction. This gives a single source of truth for provenance (enso, regional historical changes and sea-ice diagnostics migrated).Provenance linkage. Each execution now records the reference datasets it used, resolved from the ingested catalogue via the declared selectors, so they appear in the execution's dataset list next to the model datasets.
Model consolidation. The near-identical obs4MIPs and PMP climatology dataset models are consolidated onto a shared
ReferenceDatasetMixinwhile remaining distinct dataset types with their own tables and version lineage.Adds Alembic migration
a7b8c9d0e1f2(child of theadd_version_keyhead) creating theesmvaltool_reference_datasettable.Checklist
Please confirm that this pull request has done the following:
changelog/Summary by CodeRabbit
New Features
Bug Fixes