Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions changelog/775.feature.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Every PMP scalar metric value now keys its reference identity on `reference_source_id`
and declares its role with the first-class `kind` of `model`.
The ENSO diagnostics previously keyed reference identity on `reference_datasets`,
so a consumer now reads reference identity the same way for every PMP diagnostic.
ENSO references that the metrics package scores against two observation datasets jointly
(for example `HadISST_GPCP-Monthly-3-2`) are kept as a single combined `reference_source_id` value,
because the score is one value that depends on both references and cannot be split per reference.
Comment on lines +5 to +7

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Minor grammar nit: unnecessary comma before "because".

Static analysis flags a restrictive "because" clause preceded by a comma.

✏️ Proposed fix
-ENSO references that the metrics package scores against two observation datasets jointly
-(for example `HadISST_GPCP-Monthly-3-2`) are kept as a single combined `reference_source_id` value,
-because the score is one value that depends on both references and cannot be split per reference.
+ENSO references that the metrics package scores against two observation datasets jointly
+(for example `HadISST_GPCP-Monthly-3-2`) are kept as a single combined `reference_source_id` value
+because the score is one value that depends on both references and cannot be split per reference.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ENSO references that the metrics package scores against two observation datasets jointly
(for example `HadISST_GPCP-Monthly-3-2`) are kept as a single combined `reference_source_id` value,
because the score is one value that depends on both references and cannot be split per reference.
ENSO references that the metrics package scores against two observation datasets jointly
(for example `HadISST_GPCP-Monthly-3-2`) are kept as a single combined `reference_source_id` value
because the score is one value that depends on both references and cannot be split per reference.
🧰 Tools
🪛 LanguageTool

[formatting] ~6-~6: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...gle combined reference_source_id value, because the score is one value that depends on ...

(COMMA_BEFORE_BECAUSE)

Source: Linters/SAST tools

5 changes: 5 additions & 0 deletions changelog/775.fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Committed regression baselines no longer embed the machine-specific source-checkout path or the
host operating system in provider command-line provenance.
Both are now redacted to portable ``<SOURCE_DIR>`` and ``<OS>`` placeholders,
so a baseline minted on one platform (for example macOS) reproduces one re-derived on another
(for example Linux in CI) instead of drifting.
35 changes: 28 additions & 7 deletions packages/climate-ref-core/src/climate_ref_core/pycmec/metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,14 @@
from pydantic_core import CoreSchema

from climate_ref_core.env import env
from climate_ref_core.metric_values import ScalarMetricValue
from climate_ref_core.metric_values import MetricValueKind, ScalarMetricValue

ALLOW_EXTRA_KEYS = env.bool("ALLOW_EXTRA_KEYS", default=True)

# The CMEC dimension that carries a metric value's role. It is lifted out of the free
# dimensions into ``ScalarMetricValue.kind`` (its own column) when a bundle declares it.
_KIND_DIMENSION = "kind"


class MetricCV(Enum):
"""
Expand Down Expand Up @@ -535,6 +539,27 @@ def iter_results(self) -> Generator[ScalarMetricValue]:
yield from _walk_results(dimensions, self.RESULTS, {})


def _build_scalar_value(metadata: dict[str, str], value: float, attributes: Any) -> ScalarMetricValue:
"""
Build a scalar value, lifting ``kind`` out of the dimensions into its own field.

``kind`` is carried as an ordinary CMEC dimension in the bundle so a producer can declare
a value's role inline, but on the value it is a first-class field with its own database
column. It is therefore removed from the free dimensions here. A bundle that does not carry
a ``kind`` dimension is unchanged: the field keeps its default, so existing baselines and
providers that predate the contract are unaffected.
"""
kind = metadata.get(_KIND_DIMENSION)
dimensions = {k: v for k, v in metadata.items() if k != _KIND_DIMENSION}
if kind is None:
return ScalarMetricValue(dimensions=dimensions, value=value, attributes=attributes)
# ``kind`` comes from the untyped JSON dimensions; the model's Literal field still
# validates it at construction, so an unknown role is rejected rather than trusted.
return ScalarMetricValue(
dimensions=dimensions, value=value, kind=cast(MetricValueKind, kind), attributes=attributes
)


def _walk_results(
dimensions: list[str], results: dict[str, Any], metadata: dict[str, str]
) -> Generator[ScalarMetricValue]:
Expand All @@ -545,16 +570,12 @@ def _walk_results(
continue
metadata[dimension] = key
if isinstance(value, float | int):
yield ScalarMetricValue(
dimensions=metadata, value=value, attributes=results.get(MetricCV.ATTRIBUTES.value)
)
yield _build_scalar_value(metadata, value, results.get(MetricCV.ATTRIBUTES.value))
elif value is None:
# Replace any None values with NaN
# This translates null values in JSON to Python NaN's
# Missing values are different from NaN values
yield ScalarMetricValue(
dimensions=metadata, value=np.nan, attributes=results.get(MetricCV.ATTRIBUTES.value)
)
yield _build_scalar_value(metadata, np.nan, results.get(MetricCV.ATTRIBUTES.value))
else:
yield from _walk_results(dimensions[1:], value, {**metadata})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from __future__ import annotations

import json
import re
import shutil
from pathlib import Path
from typing import TYPE_CHECKING
Expand Down Expand Up @@ -53,23 +54,31 @@
# output bundle's lowercase ``provenance`` block; series.json never carries one.
_PROVENANCE_BLOCK_KEYS: frozenset[str] = frozenset({"PROVENANCE", "provenance"})

# Provenance fields redacted to stable placeholders so the committed bundle stays portable
# (machine-independent) and reproducible: ``userId`` is the minting user and ``date`` is a
# non-reproducible wall-clock timestamp. Absolute paths in ``commandLine`` are made portable
# by path placeholdering (``<OUTPUT_DIR>`` / ``<SOFTWARE_ROOT_DIR>``), not by redaction.
# Redacted to stable placeholders so committed bundles are machine-independent.
_REDACTED_PROVENANCE_FIELDS: dict[str, str] = {
"userId": "<USER>",
"date": "<DATE>",
}

# Host fields in the provenance ``platform`` sub-block: ``Name`` (hostname) and ``Version`` (kernel)
# leak host identity and churn the committed digest across machines, so they are redacted; the coarse
# ``OS`` carries no host identity and is kept.
# ``platform`` sub-block fields, redacted so a baseline is portable across hosts and OSes
# (a ``Darwin`` mint must match a ``Linux`` CI execute).
_REDACTED_PROVENANCE_PLATFORM_FIELDS: dict[str, str] = {
"Name": "<HOSTNAME>",
"Version": "<HOST_VERSION>",
"OS": "<OS>",
}

_SOURCE_DIR_PLACEHOLDER = "<SOURCE_DIR>"

# Matches the machine-specific prefix before ``/packages/climate-ref-<pkg>/`` in in-repo paths.
# Checkout-agnostic, so Linux and macOS checkouts redact to identical bytes.
_SOURCE_PATH_RE = re.compile(r'(?:/[^\s"]*?)/packages/(climate-ref-[A-Za-z0-9_.-]+/)')


def _redact_source_paths(text: str) -> str:
"""Redact the checkout-root prefix of in-repo package paths (e.g. a provenance ``commandLine``)."""
return _SOURCE_PATH_RE.sub(rf"{_SOURCE_DIR_PLACEHOLDER}/packages/\1", text)


def _redact_fields(block: dict[str, object], fields: dict[str, str]) -> bool:
"""Overwrite each present ``fields`` key in ``block`` with its placeholder; return if changed."""
Expand Down Expand Up @@ -118,7 +127,8 @@ def _canonicalise_committed_bundle(regression_dir: Path) -> None:
For each file in :data:`COMMITTED_BUNDLE_FILES` that is present:
round floats to a stable precision (:func:`~climate_ref_core.regression._quantise.round_floats`),
redact host/user CMEC provenance (:func:`_redact_provenance_fields`),
then re-serialise with :data:`_COMMITTED_JSON_DUMP_KWARGS`.
re-serialise with :data:`_COMMITTED_JSON_DUMP_KWARGS`,
then redact the checkout-root prefix of in-repo command-line paths (:func:`_redact_source_paths`).

The same transform runs on every file so the committed files are deterministic
regardless of how the diagnostic originally serialised them,
Expand All @@ -137,10 +147,9 @@ def _canonicalise_committed_bundle(regression_dir: Path) -> None:
_redact_provenance_fields(data)
# Terminate with a newline so the committed bytes match the manifest serialisation
# (Manifest.save) and satisfy POSIX/end-of-file-fixer conventions.
path.write_text(
json.dumps(data, **_COMMITTED_JSON_DUMP_KWARGS) + "\n", # type: ignore[arg-type]
encoding="utf-8",
)
serialised = json.dumps(data, **_COMMITTED_JSON_DUMP_KWARGS) + "\n" # type: ignore[arg-type]
# Text pass: the paths live inside string values, not their own JSON fields.
path.write_text(_redact_source_paths(serialised), encoding="utf-8")


def write_committed_bundle(
Expand Down
38 changes: 38 additions & 0 deletions packages/climate-ref-core/tests/unit/pycmec/test_cmec_metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,44 @@ def test_iter_results(cmec_metric):
assert list(cmec_metric.iter_results())


def test_iter_results_lifts_kind():
# A ``kind`` dimension in the bundle is lifted onto the value's first-class ``kind``
# field and removed from the free dimensions (it has its own database column).
bundle = CMECMetric(
DIMENSIONS={
"json_structure": ["kind", "source_id", "metric"],
"kind": {"reference": {}},
"source_id": {"ACCESS-ESM1-5": {}},
"metric": {"rmse": {}},
},
RESULTS={"reference": {"ACCESS-ESM1-5": {"rmse": 1.5}}},
)

(value,) = list(bundle.iter_results())

assert value.kind == "reference"
assert "kind" not in value.dimensions
assert value.dimensions == {"source_id": "ACCESS-ESM1-5", "metric": "rmse"}


def test_iter_results_without_kind_defaults_to_model():
# A bundle that predates the contract (no ``kind`` dimension) is unchanged: the value
# keeps the default role and its dimensions are untouched.
bundle = CMECMetric(
DIMENSIONS={
"json_structure": ["source_id", "metric"],
"source_id": {"ACCESS-ESM1-5": {}},
"metric": {"rmse": {}},
},
RESULTS={"ACCESS-ESM1-5": {"rmse": 2.0}},
)

(value,) = list(bundle.iter_results())

assert value.kind == "model"
assert value.dimensions == {"source_id": "ACCESS-ESM1-5", "metric": "rmse"}


def test_iter_results_empty(cmec_right_metric_dict):
cmec_metric = CMECMetric.model_validate(CMECMetric.create_template())
assert not list(cmec_metric.iter_results())
Expand Down
27 changes: 25 additions & 2 deletions packages/climate-ref-core/tests/unit/regression/test_capture.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
)
from climate_ref_core.pycmec.output import CMECOutput
from climate_ref_core.regression.capture import (
_redact_source_paths,
build_native_snapshot,
capture_execution,
materialise_native,
Expand Down Expand Up @@ -231,10 +232,10 @@ def test_write_committed_bundle_redacts_and_placeholders_provenance(tmp_path):
diag = json.loads((regression_dir / "diagnostic.json").read_text())
assert diag["PROVENANCE"]["userId"] == "<USER>"
assert diag["PROVENANCE"]["date"] == "<DATE>"
# Host fields redacted; coarse OS kept as portable context.
# Host and platform fields redacted so a baseline stays portable across machines and OSes.
assert diag["PROVENANCE"]["platform"]["Name"] == "<HOSTNAME>"
assert diag["PROVENANCE"]["platform"]["Version"] == "<HOST_VERSION>"
assert diag["PROVENANCE"]["platform"]["OS"] == "Linux"
assert diag["PROVENANCE"]["platform"]["OS"] == "<OS>"
assert diag["RESULTS"]["score"] == 1.234568

# output.json is re-dumped canonically (sorted keys); every provenance key is still present.
Expand Down Expand Up @@ -269,6 +270,28 @@ def test_redaction_leaves_clean_provenance_untouched(tmp_path):
assert list(out.keys()) == sorted(out.keys())


def test_redact_source_paths_is_checkout_agnostic():
# A committed baseline minted on one machine and re-derived on another must redact the in-repo
# driver/param path prefix to the same token regardless of the checkout root.
suffix = "packages/climate-ref-pmp/src/climate_ref_pmp/drivers/enso_driver.py --mc ENSO"
mac = f"python /Users/jane/code/climate-ref/{suffix}"
linux = f"python /home/ci/work/climate-ref/{suffix}"
expected = f"python <SOURCE_DIR>/{suffix}"
assert _redact_source_paths(mac) == expected
assert _redact_source_paths(linux) == expected


def test_redact_source_paths_leaves_other_tokens_untouched():
# Already-portable placeholders (conda software root, test data, output dir) are not in-repo
# package paths and must survive the source-path redaction unchanged.
for token_path in (
"<SOFTWARE_ROOT_DIR>/conda/pmp-abc/bin/mean_climate_driver",
"<TEST_DATA_DIR>/enso_tel/cmip6/regression",
"<OUTPUT_DIR>/input_ENSO.json",
):
assert _redact_source_paths(token_path) == token_path


def test_build_native_snapshot_digests_relpaths(tmp_path):
base = tmp_path / "results" / "frag"
base.mkdir(parents=True)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ class AnnualCycle(CommandLineDiagnostic):
name = "Annual Cycle"
slug = "annual-cycle"
facets = (
"kind",
"mip_id",
"source_id",
"member_id",
Expand All @@ -290,7 +291,7 @@ class AnnualCycle(CommandLineDiagnostic):
"statistic",
"season",
)
version = 2
version = 3

_variable_obs_pairs = (
# ERA-5 as reference dataset, spatial 2-D variables
Expand Down Expand Up @@ -413,7 +414,7 @@ def build_cmds(self, definition: ExecutionDefinition) -> list[list[str]]: # noq
"""
model_source_type = get_model_source_type(definition)
input_datasets = definition.datasets[model_source_type]
reference_datasets = definition.datasets[SourceDatasetType.PMPClimatology]
reference_collection = definition.datasets[SourceDatasetType.PMPClimatology]

source_id = input_datasets["source_id"].unique()[0]
experiment_id = input_datasets["experiment_id"].unique()[0]
Expand All @@ -435,10 +436,10 @@ def build_cmds(self, definition: ExecutionDefinition) -> list[list[str]]: # noq
logger.debug(f"input_datasets: {input_datasets}")
logger.debug(f"input_datasets.keys(): {input_datasets.keys()}")

reference_dataset_name = reference_datasets["source_id"].unique()[0]
reference_dataset_path = reference_datasets.datasets.iloc[0]["path"]
reference_dataset_name = reference_collection["source_id"].unique()[0]
reference_dataset_path = reference_collection.datasets.iloc[0]["path"]

logger.debug(f"reference_dataset.datasets: {reference_datasets.datasets}")
logger.debug(f"reference_dataset.datasets: {reference_collection.datasets}")
logger.debug(f"reference_dataset_name: {reference_dataset_name}")
logger.debug(f"reference_dataset_path: {reference_dataset_path}")

Expand Down Expand Up @@ -583,14 +584,17 @@ def build_execution_result(self, definition: ExecutionDefinition) -> ExecutionRe

# Add missing dimensions to the output
member_id_col = "variant_label" if model_source_type == SourceDatasetType.CMIP7 else "member_id"
reference_datasets = definition.datasets[SourceDatasetType.PMPClimatology]
reference_collection = definition.datasets[SourceDatasetType.PMPClimatology]
cmec_metric_bundle = cmec_metric_bundle.prepend_dimensions(
{
# PMP scalars are model-performance scores against a reference, not reference
# (observation) values, so every value's role is ``model``.
"kind": "model",
"source_id": input_datasets["source_id"].unique()[0],
"member_id": input_datasets[member_id_col].unique()[0],
"experiment_id": input_datasets["experiment_id"].unique()[0],
"variable_id": input_datasets["variable_id"].unique()[0],
"reference_source_id": reference_datasets["source_id"].unique()[0],
"reference_source_id": reference_collection["source_id"].unique()[0],
}
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,17 @@ class ENSO(CommandLineDiagnostic):

reconstruction_inputs = PMP_RECONSTRUCTION_INPUTS

version = 2

facets = (
"kind",
"mip_id",
"source_id",
"member_id",
"grid_label",
"experiment_id",
"metric",
"reference_datasets",
"reference_source_id",
)

def __init__(self, metrics_collection: str, experiments: Collection[str] = ("historical",)) -> None:
Expand Down Expand Up @@ -392,6 +395,9 @@ def build_execution_result(self, definition: ExecutionDefinition) -> ExecutionRe
],
).prepend_dimensions(
{
# PMP scalars are model-performance scores against a reference, not reference
# (observation) values, so every value's role is ``model``.
"kind": "model",
"mip_id": model_source_type.value,
"source_id": source_id,
"member_id": member_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ class ExtratropicalModesOfVariability(CommandLineDiagnostic):
ts_modes = ("PDO", "NPGO", "AMO")
psl_modes = ("NAO", "NAM", "PNA", "NPO", "SAM")

version = 2

facets = (
"kind",
"mip_id",
"source_id",
"member_id",
Expand Down Expand Up @@ -331,7 +334,7 @@ def build_execution_result(self, definition: ExecutionDefinition) -> ExecutionRe

cmec_output_bundle, cmec_metric_bundle = process_json_result(results_files[0], png_files, data_files)
input_datasets = definition.datasets[model_source_type]
reference_datasets = definition.datasets[SourceDatasetType.obs4MIPs]
reference_collection = definition.datasets[SourceDatasetType.obs4MIPs]
member_id_col = "variant_label" if model_source_type == SourceDatasetType.CMIP7 else "member_id"
cmec_metric_bundle = cmec_metric_bundle.remove_dimensions(
[
Expand All @@ -341,11 +344,14 @@ def build_execution_result(self, definition: ExecutionDefinition) -> ExecutionRe
],
).prepend_dimensions(
{
# PMP scalars are model-performance scores against a reference, not reference
# (observation) values, so every value's role is ``model``.
"kind": "model",
"mip_id": model_source_type.value,
"source_id": input_datasets["source_id"].unique()[0],
"member_id": input_datasets[member_id_col].unique()[0],
"experiment_id": input_datasets["experiment_id"].unique()[0],
"reference_source_id": reference_datasets["source_id"].unique()[0],
"reference_source_id": reference_collection["source_id"].unique()[0],
}
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,11 @@ def write_CMEC_json(json_file):
ref_datasets_dict = {ref: {} for ref in ref_datasets}

dimensions_dict = {
"json_structure": ["model", "realization", "metric", "reference_datasets"],
"json_structure": ["model", "realization", "metric", "reference_source_id"],
"model": {mod: {}},
"realization": {run: {}},
"metric": metrics_dict,
"reference_datasets": ref_datasets_dict,
"reference_source_id": ref_datasets_dict,
}

results_dict = {}
Expand Down
Loading
Loading