Add ref doctor, and a generated reference data page - #865
Merged
Conversation
Three problems in this area are silent. Reference data that is missing means its diagnostics plan no executions, so a deployment looks healthy while producing nothing for them. Data ingested as `obs4ref` is never selected, because the solver matches a requirement against its own source type and no diagnostic declares an obs4REF requirement. And a dataset ingested from two collections at the same version merges on instance_id into one dataset holding both sets of files, so a diagnostic reads the overlapping period twice. `ref doctor` reports all three, plus the registry overlaps that cause the third. It exits non-zero on an error, or on a warning with --strict, so it can gate a run. Checks are plain functions over a DoctorContext; one that raises becomes a finding rather than stopping the rest. Which collection supplies a given dataset was not written down anywhere, so it had to be reconstructed from the providers' data requirements every time the question came up. `climate_ref_core.reference_data` works it out once, and both the new generated Reference data page and doctor's missing-data check read from it, so the published table and the local check cannot disagree. Provenance is resolved per (source type, source_id). Per source_id alone would credit the PMP registry's ERA-5 climatology for obs4MIPs ERA-5. Per (source_id, variable_id) would be a claim the archive cannot support: ESGF intersects its facets, so a requirement naming four sources and eight variables does not mean all thirty-two combinations exist. Verified against two real deployments: on one that ingested both the obs4REF registry and the ESGF mirror it reports exactly the five datasets that hold duplicate coverage, and on one that ingested obs4REF under the obs4ref source type it reports the eight reference datasets whose ILAMB diagnostics are consequently unreachable, plus the source type as the cause. Constraint: a check must be able to run against catalogs alone, so DoctorContext.from_catalogs builds one with no database behind it Rejected: Add an ingest-time guard that drops a file whose period is already covered | preferring the published obs4MIPs copy is the agreed fix and belongs with the obs4REF source-type migration; doctor reports the condition until then Rejected: Report missing data per (source_id, variable_id) | ESGF facet intersection makes most such pairs fictitious, which is how an earlier hand analysis produced 25 phantom gaps Confidence: high Scope-risk: moderate Directive: A new check goes in CHECKS and must tolerate an empty catalog for every source type; `from_catalogs` fills the ones it is not given Not-tested: The duplicate-coverage check assumes start_time/end_time are populated, so a catalog ingested with the DRS parser and not yet finalised will not be checked
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis change adds the ChangesDeployment diagnostics
Database URL redaction
Sequence Diagram(s)sequenceDiagram
participant Operator
participant DoctorCLI
participant DoctorContext
participant Diagnose
participant CheckRegistry
Operator->>DoctorCLI: Run ref doctor
DoctorCLI->>DoctorContext: Build deployment context
DoctorCLI->>Diagnose: Request diagnosis
Diagnose->>CheckRegistry: Run registered checks
CheckRegistry-->>Diagnose: Return findings
Diagnose-->>DoctorCLI: Return DoctorReport
DoctorCLI-->>Operator: Render report and exit status
Possibly related PRs
Mergeability Score: ⚪ Minimal · up to This PR adds a diagnostic command and generated reference-data documentation; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Two things were missing for doctor to grow the way it needs to. A check could only be added by editing a tuple in `climate_ref`, so a provider package could not ship one for its own setup problems. And the output named the finding but not the deployment, which is most of what a maintainer has to ask for when a report arrives. A check is now declared with `@check(slug, description)` and found through the `climate-ref.doctor-checks` entry point group, alongside `climate-ref.providers`. The slug lives only in the registration: the runner stamps it onto every finding, so the two cannot drift. A plugin module that fails to import becomes an error finding rather than taking the command down, because a check that never ran must not look like a check that passed. `ref doctor --list` shows what is registered and where each check came from. `--format markdown` and `--format json` add the versions, platform, configuration, paths, providers, ingested counts and REF_/DASK_/ESMVALTOOL_ variables of the deployment. Redaction happens where the values are collected rather than where they are displayed, so no output format can leak a credential; SQLAlchemy parses the database URL, since it is what parses it to build the engine. The module became a package: findings, context, registry and environment separate from the checks themselves, which now have somewhere to go other than one file. Doctor also no longer configures or registers providers when it loads them. It reads their metadata only, and the docstring already claimed it does not write to the database it is inspecting; `configure` bootstraps conda as a side effect. Constraint: a check must tolerate a context built from catalogs alone, with no database behind it Rejected: Reuse cli/_utils.OutputFormat | its members are table/json and findings are not a DataFrame; per-command format enums are the existing convention Rejected: Have `ref --version` render the doctor version block | a wider change to a stable output than this needs Confidence: high Scope-risk: moderate Directive: Redact in environment.py, never in a renderer, so a new output format cannot leak a credential Not-tested: A real plugin distribution; the entry point group is exercised through a stubbed `importlib.metadata.entry_points`
A Postgres deployment wrote its credentials into the log on every startup, and into anything that log was pasted into. - Adds `redact_url`, which hides the password and leaves the rest legible. - The doctor environment report already did this, so it now shares the one implementation. - An unparseable URL is redacted whole rather than printed on the hope it holds nothing.
The report repeated the whole remedy paragraph and the check slug once per finding, so a deployment missing seventeen reference datasets printed the same instruction seventeen times. - Splits `Finding.detail` into `detail`, `remedy` and `command`. Findings sharing a remedy are reported under it once, ahead of the list they apply to. - `command` is printed alone and unwrapped, so it survives a narrow terminal and stays pasteable. - Groups the findings by check, so the slug is named once per group rather than on every line. - Moves the summary count above the findings, and gives wrapped lines the indent of the line they continue. - Adds `pluralise`, so a count reads `1 diagnostic` rather than `1 diagnostic(s)`. Also fixes `ref doctor` no longer being recognised as read only, because the argument walk stopped matching a command that sits at the top level rather than in a group.
Displaying a report meant learning five calls and the order to make them in: build a context, run the checks, count them, collect the environment, then rank what came back. - Adds `diagnose`, which does all of that and returns a `DoctorReport`. - Drops `register_check`, which had no callers, and `SEVERITY_ORDER`, which was `tuple(Severity)`. - Collapses `EnvironmentReport` into the dict it wrapped, since it carried one field. - Keeps `RegisteredCheck`, `run_checks` and `collect_environment` for the package's own use. - Moves `pluralise` to `climate_ref.text`, because it is a text helper rather than a doctor concept. This takes the exports from twelve to eight, split between the two audiences: `diagnose`, `DoctorReport`, `Finding`, `Severity`, `worst_severity` and `iter_checks` for a caller, `check` and `DoctorContext` for a package contributing its own checks.
lewisjared
force-pushed
the
feat/ref-doctor
branch
from
August 13, 2026 05:48
0b91204 to
58b41d3
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
packages/climate-ref/tests/unit/test_doctor.py (1)
200-204: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert that environment collection stays offline.
Add the
no_networkfixture and assert that no blocked socket attempt occurred. This test uses an in-memoryDoctorContext, so a future network call should fail the test.Based on learnings, offline tests should record blocked socket attempts and assert that no attempt occurred where resolution is expected to be fully offline.
Source: Learnings
packages/climate-ref-core/src/climate_ref_core/reference_data.py (1)
38-45: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the source-type value set out of the loop.
Line 195 rebuilds
{t.value for t in REFERENCE_SOURCE_TYPES}for every requirement of every diagnostic. Compute the values once at module level.♻️ Proposed refactor
REFERENCE_SOURCE_TYPES = frozenset( { SourceDatasetType.obs4MIPs, SourceDatasetType.obs4REF, SourceDatasetType.PMPClimatology, SourceDatasetType.ESMValToolReference, } ) + +_REFERENCE_SOURCE_TYPE_VALUES = frozenset(t.value for t in REFERENCE_SOURCE_TYPES)- if requirement.source_type not in {t.value for t in REFERENCE_SOURCE_TYPES}: + if requirement.source_type not in _REFERENCE_SOURCE_TYPE_VALUES: continueAlso applies to: 193-200
packages/climate-ref/src/climate_ref/doctor/context.py (1)
16-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not share one mutable
DataFramebetween source types and contexts.
EMPTY_CATALOGis a single mutable object.from_catalogsassigns that same instance to every absent source type, andcatalog()returns it to callers. If any check or caller mutates the returned frame in place, the mutation becomes visible for all other source types and all other contexts. Return a new empty frame instead.♻️ Proposed refactor
-EMPTY_CATALOG = pd.DataFrame() -"""Stands in for a source type with nothing ingested.""" +def empty_catalog() -> pd.DataFrame: + """Stand in for a source type with nothing ingested.""" + return pd.DataFrame()complete = { - source_type: catalogs.get(source_type, EMPTY_CATALOG) for source_type in SourceDatasetType + source_type: catalogs.get(source_type, empty_catalog()) for source_type in SourceDatasetType }Also applies to: 58-61
packages/climate-ref/src/climate_ref/doctor/checks/data.py (1)
62-89: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe reported overlapping pair can name the wrong partner file.
The sweep tracks
running_endacross all earlier rows, but line 71 always recordspaths[index - 1]as the partner. If rowindexoverlaps an earlier row rather than its immediate predecessor, the detail names a file that does not overlap. The count and the finding stay correct, so this only affects the reported collection roots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6b0d370-57ba-4d5b-be94-c329d55459f7
📒 Files selected for processing (27)
changelog/865.feature.mdchangelog/865.fix.mddocs/gen_reference_data_stubs.pydocs/how-to-guides/diagnose-a-deployment.mdmkdocs.ymlpackages/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_reference_data.pypackages/climate-ref/src/climate_ref/cli/__init__.pypackages/climate-ref/src/climate_ref/cli/doctor.pypackages/climate-ref/src/climate_ref/database.pypackages/climate-ref/src/climate_ref/doctor/__init__.pypackages/climate-ref/src/climate_ref/doctor/checks/__init__.pypackages/climate-ref/src/climate_ref/doctor/checks/data.pypackages/climate-ref/src/climate_ref/doctor/context.pypackages/climate-ref/src/climate_ref/doctor/environment.pypackages/climate-ref/src/climate_ref/doctor/findings.pypackages/climate-ref/src/climate_ref/doctor/registry.pypackages/climate-ref/src/climate_ref/doctor/report.pypackages/climate-ref/src/climate_ref/text.pypackages/climate-ref/tests/unit/cli/test_doctor.pypackages/climate-ref/tests/unit/cli/test_root.pypackages/climate-ref/tests/unit/test_database.pypackages/climate-ref/tests/unit/test_doctor.pypackages/climate-ref/tests/unit/test_doctor_environment.pypackages/climate-ref/tests/unit/test_doctor_registry.pypackages/climate-ref/tests/unit/test_text.py
- Redacts a password held inside a URL-valued environment variable, not just a variable whose name marks it as a secret. - Matches a read-only top-level command in the command path only, so a path or an option value reading as a command name no longer skips the pre-migration backup. - Drops the checks a plugin registered before its import failed, so a half-loaded plugin contributes nothing. - Guards the catalog columns both data checks read, so an adapter with a different schema does not stop the run. - Skips only an unavailable provider when generating the reference data page, and reports why. - Fixes two missing words in the how-to guide.
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
2 participants
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.
Adds a
ref doctorcli command.This command provides useful information about potential configuration and data issues.
The current rules cover:
but it new rules can be added in the core or via each producer.
Reference data page
A side effect of collating with reference data are needed we now publish the required reference datasets to the docs.
Summary by CodeRabbit
New Features
ref doctorcommand to identify deployment issues, including missing, unreachable, duplicated, or overlapping reference data.Bug Fixes