From 5f0682136dbab890ed0a507a1d4c314ce3f90c1e Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Tue, 31 Mar 2026 20:19:05 +0900 Subject: [PATCH 01/19] feat(cli): add reingest command with enum-based mode selection Add `ref executions reingest` command that re-runs build_execution_result() on existing outputs without re-executing diagnostics. Uses ReingestMode enum for the --mode option, providing tab completion and validated choices (additive, replace, versioned) via Typer's native enum support. --- .../src/climate_ref/cli/executions.py | 157 ++++ .../src/climate_ref/executor/__init__.py | 10 +- .../src/climate_ref/executor/reingest.py | 696 ++++++++++++++++++ .../tests/unit/executor/test_reingest.py | 641 ++++++++++++++++ 4 files changed, 1503 insertions(+), 1 deletion(-) create mode 100644 packages/climate-ref/src/climate_ref/executor/reingest.py create mode 100644 packages/climate-ref/tests/unit/executor/test_reingest.py diff --git a/packages/climate-ref/src/climate_ref/cli/executions.py b/packages/climate-ref/src/climate_ref/cli/executions.py index 688949987..574be90d0 100644 --- a/packages/climate-ref/src/climate_ref/cli/executions.py +++ b/packages/climate-ref/src/climate_ref/cli/executions.py @@ -21,6 +21,7 @@ from climate_ref.cli._utils import df_to_table, parse_facet_filters, pretty_print_df from climate_ref.config import Config +from climate_ref.executor.reingest import ReingestMode from climate_ref.models import Execution, ExecutionGroup from climate_ref.models.diagnostic import Diagnostic from climate_ref.models.execution import execution_datasets, get_execution_group_and_latest_filtered @@ -741,6 +742,162 @@ def stats( pretty_print_df(results_df, console=console) +@app.command() +def reingest( # noqa: PLR0913 + ctx: typer.Context, + group_ids: Annotated[ + list[int] | None, + typer.Argument(help="Execution group IDs to reingest. If omitted, uses filters."), + ] = None, + mode: Annotated[ + ReingestMode, + typer.Option( + help="Reingest mode: 'additive' (keep existing, add new), " + "'replace' (delete existing, re-ingest), " + "'versioned' (create new execution record)." + ), + ] = ReingestMode.additive, + provider: Annotated[ + list[str] | None, + typer.Option( + help="Filter by provider slug (substring match, case-insensitive). " + "Multiple values can be provided." + ), + ] = None, + diagnostic: Annotated[ + list[str] | None, + typer.Option( + help="Filter by diagnostic slug (substring match, case-insensitive). " + "Multiple values can be provided." + ), + ] = None, + include_failed: Annotated[ + bool, + typer.Option( + "--include-failed", + help="Also attempt reingest on failed executions.", + ), + ] = False, + dry_run: Annotated[ + bool, + typer.Option( + "--dry-run", + help="Show what would be reingested without making changes.", + ), + ] = False, + force: bool = typer.Option(False, help="Skip confirmation prompt"), +) -> None: + """ + Reingest existing executions without re-running diagnostics. + + Re-runs build_execution_result() on existing output files and re-ingests + the results into the database. Useful when new series definitions or + metadata extraction logic has been added. + + Three modes are available: + + - additive: Keep existing metric values, add any new ones. Outputs are replaced. + + - replace: Delete all existing metric values and outputs, then re-ingest from scratch. + + - versioned: Create a new Execution record under the same ExecutionGroup, + leaving the original execution untouched. + + The dirty flag is never modified by this command. + """ + import pandas as pd + + from climate_ref.executor.reingest import ( + get_executions_for_reingest, + reingest_execution, + ) + from climate_ref.provider_registry import ProviderRegistry + + config: Config = ctx.obj.config + db = ctx.obj.database + console = ctx.obj.console + + # Require at least one filter to prevent accidental full reingest + if not any([group_ids, provider, diagnostic]): + logger.error( + "At least one filter is required (group IDs, --provider, or --diagnostic). " + "This prevents accidental reingest of all executions." + ) + raise typer.Exit(code=1) + + # Build provider registry + provider_registry = ProviderRegistry.build_from_config(config, db) + + # Query eligible executions + results = get_executions_for_reingest( + db, + execution_group_ids=group_ids, + provider_filters=provider, + diagnostic_filters=diagnostic, + include_failed=include_failed, + ) + + if not results: + console.print("No executions found matching the specified criteria.") + return + + # Build preview table + preview_df = pd.DataFrame( + [ + { + "group_id": eg.id, + "execution_id": ex.id, + "provider": eg.diagnostic.provider.slug, + "diagnostic": eg.diagnostic.slug, + "key": eg.key, + "successful": ex.successful, + } + for eg, ex in results + ] + ) + + if dry_run: + console.print(f"[bold]Dry run:[/] would reingest {len(results)} execution(s) in {mode.value} mode:") + pretty_print_df(preview_df, console=console) + return + + # Show preview and confirm + console.print(f"Will reingest {len(results)} execution(s) in [bold]{mode.value}[/] mode:") + pretty_print_df(preview_df, console=console) + + if not force: + if not typer.confirm("\nProceed with reingest?"): + console.print("Reingest cancelled.") + return + + # Process each execution + success_count = 0 + skip_count = 0 + for eg, ex in results: + with db.session.begin(): + # Safety net: reingest_execution does not modify dirty, but we + # save/restore as a guard against future regressions. + dirty_before = eg.dirty + + ok = reingest_execution( + config=config, + database=db, + execution=ex, + provider_registry=provider_registry, + mode=mode, + ) + + # Restore dirty flag (reingest must not modify it) + eg.dirty = dirty_before + + if ok: + success_count += 1 + else: + skip_count += 1 + + console.print(f"\n[green]Reingest complete:[/] {success_count} succeeded, {skip_count} skipped.") + + @app.command() def flag_dirty(ctx: typer.Context, execution_id: int) -> None: """ diff --git a/packages/climate-ref/src/climate_ref/executor/__init__.py b/packages/climate-ref/src/climate_ref/executor/__init__.py index 9dd61acfb..57cba3cfa 100644 --- a/packages/climate-ref/src/climate_ref/executor/__init__.py +++ b/packages/climate-ref/src/climate_ref/executor/__init__.py @@ -18,7 +18,15 @@ HPCExecutor = exc # type: ignore from .local import LocalExecutor +from .reingest import ReingestMode, reingest_execution from .result_handling import handle_execution_result from .synchronous import SynchronousExecutor -__all__ = ["HPCExecutor", "LocalExecutor", "SynchronousExecutor", "handle_execution_result"] +__all__ = [ + "HPCExecutor", + "LocalExecutor", + "ReingestMode", + "SynchronousExecutor", + "handle_execution_result", + "reingest_execution", +] diff --git a/packages/climate-ref/src/climate_ref/executor/reingest.py b/packages/climate-ref/src/climate_ref/executor/reingest.py new file mode 100644 index 000000000..4e743f43d --- /dev/null +++ b/packages/climate-ref/src/climate_ref/executor/reingest.py @@ -0,0 +1,696 @@ +""" +Reingest existing execution results without re-running diagnostics. + +This module provides functionality to re-run ``build_execution_result()`` and +re-ingest the results into the database for executions that have already completed. +This is useful when new series definitions or metadata extraction logic has been added +and you want to apply it to existing outputs without re-executing the diagnostics. + +Three reingest modes are supported: + +* **additive** -- preserve existing metric values, insert only values with + dimension signatures not already present for the execution +* **replace** -- delete all existing metric values and outputs, then re-ingest +* **versioned** -- create a new ``Execution`` record under the same ``ExecutionGroup`` + with its own output directory, leaving the original execution untouched +""" + +import enum +import hashlib +import pathlib +import shutil +from collections import defaultdict +from collections.abc import Sequence +from typing import TYPE_CHECKING, Any + +import pandas as pd +from loguru import logger +from sqlalchemy import delete, insert + +from climate_ref.datasets import get_slug_column +from climate_ref.models import ScalarMetricValue, SeriesMetricValue +from climate_ref.models.execution import ( + Execution, + ExecutionGroup, + ExecutionOutput, + ResultOutputType, + execution_datasets, + get_execution_group_and_latest_filtered, +) +from climate_ref.models.metric_value import MetricValue +from climate_ref_core.datasets import ( + DatasetCollection, + ExecutionDatasetCollection, + SourceDatasetType, +) +from climate_ref_core.diagnostics import ExecutionDefinition, ExecutionResult, ensure_relative_path +from climate_ref_core.exceptions import ResultValidationError +from climate_ref_core.metric_values import SeriesMetricValue as TSeries +from climate_ref_core.pycmec.controlled_vocabulary import CV +from climate_ref_core.pycmec.metric import CMECMetric +from climate_ref_core.pycmec.output import CMECOutput, OutputDict + +if TYPE_CHECKING: + from climate_ref.config import Config + from climate_ref.database import Database + from climate_ref.models.dataset import Dataset + from climate_ref.provider_registry import ProviderRegistry + from climate_ref_core.diagnostics import Diagnostic + + +class ReingestMode(enum.Enum): + """Mode for reingesting execution results.""" + + additive = "additive" + replace = "replace" + versioned = "versioned" + + +def reconstruct_execution_definition( + config: "Config", + execution: Execution, + diagnostic: "Diagnostic", +) -> ExecutionDefinition: + """ + Reconstruct an ``ExecutionDefinition`` from database state. + + This rebuilds the definition that was originally used to produce the execution, + using the execution's stored datasets, output fragment, and the live diagnostic + object from the provider registry. + + Parameters + ---------- + config + Application configuration (provides ``paths.results``) + execution + The database ``Execution`` record to reconstruct from + diagnostic + The live ``Diagnostic`` instance resolved from the provider registry + + Returns + ------- + : + A reconstructed ``ExecutionDefinition`` pointing at the results directory + """ + execution_group = execution.execution_group + + # Build DatasetCollection per source type from the execution's linked datasets + datasets_by_type: dict[SourceDatasetType, list[Any]] = defaultdict(list) + for dataset in execution.datasets: + datasets_by_type[dataset.dataset_type].append(dataset) + + collection: dict[SourceDatasetType | str, DatasetCollection] = {} + for source_type, ds_list in datasets_by_type.items(): + slug_column = get_slug_column(source_type) + + # Build a DataFrame from the DB dataset records and their files + rows = [] + for dataset in ds_list: + # Get all attributes from the polymorphic dataset model + dataset_attrs = _extract_dataset_attributes(dataset) + for file in dataset.files: + row = { + **dataset_attrs, + "path": file.path, + "start_time": file.start_time, + "end_time": file.end_time, + } + if hasattr(file, "tracking_id"): + row["tracking_id"] = file.tracking_id + rows.append((dataset.id, row)) + + if rows: + index = [r[0] for r in rows] + data = [r[1] for r in rows] + df = pd.DataFrame(data, index=index) + else: + df = pd.DataFrame() + + # Retrieve the selector for this source type from the execution group + selector_key = source_type.value + selector = tuple(tuple(pair) for pair in execution_group.selectors.get(selector_key, [])) + + collection[source_type] = DatasetCollection( + datasets=df, + slug_column=slug_column, + selector=selector, + ) + + # Point at the scratch directory -- the caller is expected to copy results + # to scratch before calling build_execution_result() to avoid mutating + # the live results tree. + output_directory = config.paths.scratch / execution.output_fragment + + return ExecutionDefinition( + diagnostic=diagnostic, + key=execution_group.key, + datasets=ExecutionDatasetCollection(collection), + root_directory=config.paths.scratch, + output_directory=output_directory, + ) + + +def _extract_dataset_attributes(dataset: "Dataset") -> dict[str, object]: + """ + Extract all column values from a polymorphic dataset model as a dict. + + Introspects the SQLAlchemy mapper to get all mapped columns for the concrete + dataset type (e.g. CMIP6Dataset, Obs4MIPsDataset). + """ + attrs = {} + # Get columns from the concrete mapper (handles polymorphic inheritance) + mapper = type(dataset).__mapper__ + for column in mapper.columns: + col_name = column.key + # Skip internal/FK columns + if col_name in ("id", "dataset_type"): + continue + val = getattr(dataset, col_name, None) + if val is not None: + attrs[col_name] = val + return attrs + + +def _handle_reingest_output_bundle( + config: "Config", + database: "Database", + execution: Execution, + cmec_output_bundle_filename: pathlib.Path, +) -> None: + """ + Process the output bundle for reingest (no file copy, files already in results dir). + """ + cmec_output_bundle = CMECOutput.load_from_json(cmec_output_bundle_filename) + _handle_reingest_outputs( + cmec_output_bundle.plots, + output_type=ResultOutputType.Plot, + config=config, + database=database, + execution=execution, + ) + _handle_reingest_outputs( + cmec_output_bundle.data, + output_type=ResultOutputType.Data, + config=config, + database=database, + execution=execution, + ) + _handle_reingest_outputs( + cmec_output_bundle.html, + output_type=ResultOutputType.HTML, + config=config, + database=database, + execution=execution, + ) + + +def _handle_reingest_outputs( + outputs: dict[str, OutputDict] | None, + output_type: "ResultOutputType", + config: "Config", + database: "Database", + execution: Execution, +) -> None: + """Register outputs in the DB without copying files (they are already in place).""" + outputs = outputs or {} + results_base = config.paths.results / execution.output_fragment + + for key, output_info in outputs.items(): + filename = ensure_relative_path(output_info.filename, results_base) + database.session.add( + ExecutionOutput.build( + execution_id=execution.id, + output_type=output_type, + filename=str(filename), + description=output_info.description, + short_name=key, + long_name=output_info.long_name, + dimensions=output_info.dimensions or {}, + ) + ) + + +def _process_reingest_series( + database: "Database", + result: ExecutionResult, + execution: Execution, + cv: CV, +) -> None: + """ + Process series values for reingest. + + Like ``_process_execution_series`` but skips the file copy since the series + file is already in the results directory. Unlike the normal ingestion path, + errors are raised rather than swallowed so that the caller can roll back. + """ + assert result.series_filename, "Series filename must be set in the result" + + # Load the series values directly from the results directory + series_values_path = result.to_output_path(result.series_filename) + series_values = TSeries.load_from_json(series_values_path) + + try: + cv.validate_metrics(series_values) + except (ResultValidationError, AssertionError): + logger.exception("Diagnostic values do not conform with the controlled vocabulary") + + series_values_content = [ + { + "execution_id": execution.id, + "values": series_result.values, + "attributes": series_result.attributes, + "index": series_result.index, + "index_name": series_result.index_name, + **series_result.dimensions, + } + for series_result in series_values + ] + logger.debug(f"Ingesting {len(series_values)} series values for execution {execution.id}") + if series_values: + database.session.execute( + insert(SeriesMetricValue), + series_values_content, + ) + + +def _process_reingest_scalar( + database: "Database", + result: ExecutionResult, + execution: Execution, + cv: CV, +) -> None: + """ + Process scalar values for reingest. + + Like ``_process_execution_scalar`` but raises on insertion errors + so the caller can roll back. + """ + cmec_metric_bundle = CMECMetric.load_from_json(result.to_output_path(result.metric_bundle_filename)) + + try: + cv.validate_metrics(cmec_metric_bundle) + except (ResultValidationError, AssertionError): + logger.exception("Diagnostic values do not conform with the controlled vocabulary") + + scalar_values = [ + { + "execution_id": execution.id, + "value": metric_result.value, + "attributes": metric_result.attributes, + **metric_result.dimensions, + } + for metric_result in cmec_metric_bundle.iter_results() + ] + logger.debug(f"Ingesting {len(scalar_values)} scalar values for execution {execution.id}") + if scalar_values: + database.session.execute( + insert(ScalarMetricValue), + scalar_values, + ) + + +def _get_existing_metric_dimensions( + database: "Database", execution: Execution +) -> set[tuple[tuple[str, str], ...]]: + """ + Get the dimension signatures of all existing metric values for an execution. + + Each signature is a sorted tuple of (dimension_name, value) pairs, making + it suitable for set-based deduplication. + """ + sigs: set[tuple[tuple[str, str], ...]] = set() + for mv in database.session.query(MetricValue).filter(MetricValue.execution_id == execution.id).all(): + dims = tuple(sorted(mv.dimensions.items())) + sigs.add(dims) + return sigs + + +def _process_reingest_scalar_additive( + database: "Database", + result: ExecutionResult, + execution: Execution, + cv: CV, +) -> None: + """ + Process scalar values for additive reingest. + + Only inserts values whose dimension signatures are not already present + for this execution. + """ + cmec_metric_bundle = CMECMetric.load_from_json(result.to_output_path(result.metric_bundle_filename)) + + try: + cv.validate_metrics(cmec_metric_bundle) + except (ResultValidationError, AssertionError): + logger.exception("Diagnostic values do not conform with the controlled vocabulary") + + existing = _get_existing_metric_dimensions(database, execution) + + new_values = [] + for metric_result in cmec_metric_bundle.iter_results(): + dims = tuple(sorted(metric_result.dimensions.items())) + if dims not in existing: + new_values.append( + { + "execution_id": execution.id, + "value": metric_result.value, + "attributes": metric_result.attributes, + **metric_result.dimensions, + } + ) + + logger.debug( + f"Additive: {len(new_values)} new scalar values " + f"(skipped {len(list(cmec_metric_bundle.iter_results())) - len(new_values)} existing) " + f"for execution {execution.id}" + ) + if new_values: + database.session.execute(insert(ScalarMetricValue), new_values) + + +def _process_reingest_series_additive( + database: "Database", + result: ExecutionResult, + execution: Execution, + cv: CV, +) -> None: + """ + Process series values for additive reingest. + + Only inserts series whose dimension signatures are not already present + for this execution. + """ + assert result.series_filename, "Series filename must be set in the result" + + series_values_path = result.to_output_path(result.series_filename) + series_values = TSeries.load_from_json(series_values_path) + + try: + cv.validate_metrics(series_values) + except (ResultValidationError, AssertionError): + logger.exception("Diagnostic values do not conform with the controlled vocabulary") + + existing = _get_existing_metric_dimensions(database, execution) + + new_values = [] + for series_result in series_values: + dims = tuple(sorted(series_result.dimensions.items())) + if dims not in existing: + new_values.append( + { + "execution_id": execution.id, + "values": series_result.values, + "attributes": series_result.attributes, + "index": series_result.index, + "index_name": series_result.index_name, + **series_result.dimensions, + } + ) + + logger.debug( + f"Additive: {len(new_values)} new series values " + f"(skipped {len(series_values) - len(new_values)} existing) " + f"for execution {execution.id}" + ) + if new_values: + database.session.execute(insert(SeriesMetricValue), new_values) + + +def _copy_results_to_scratch( + config: "Config", + output_fragment: str, +) -> pathlib.Path: + """ + Copy an execution's results directory to scratch for safe re-extraction. + + ``build_execution_result()`` may write files (CMEC bundles) into its + ``definition.output_directory``. Running it directly against the live + results tree would mutate the original outputs before the DB savepoint + has a chance to roll back on failure. Copying to scratch first keeps + the results tree unchanged until we decide to commit. + + Returns the scratch output directory. + """ + src = config.paths.results / output_fragment + dst = config.paths.scratch / output_fragment + if dst.exists(): + shutil.rmtree(dst) + shutil.copytree(src, dst) + return dst + + +def _delete_execution_results(database: "Database", execution: Execution) -> None: + """Delete all metric values and outputs for an execution.""" + database.session.execute(delete(ExecutionOutput).where(ExecutionOutput.execution_id == execution.id)) + # MetricValue uses single-table inheritance so we delete from the base table + database.session.execute(delete(MetricValue).where(MetricValue.execution_id == execution.id)) + + +def _apply_reingest_mode( # noqa: PLR0913 + config: "Config", + database: "Database", + execution: Execution, + result: ExecutionResult, + mode: ReingestMode, + cv: CV, +) -> Execution: + """ + Apply mode-specific DB mutations inside a savepoint. + + Returns the target execution (may differ from ``execution`` in versioned mode). + """ + target_execution = execution + execution_group = execution.execution_group + + with database.session.begin_nested(): + if mode == ReingestMode.versioned: + version_hash = hashlib.sha1( # noqa: S324 + f"{execution.output_fragment}-reingest-{execution.id}".encode() + ).hexdigest()[:12] + + target_execution = Execution( + execution_group=execution_group, + dataset_hash=execution.dataset_hash, + output_fragment=f"{execution.output_fragment}_v{version_hash}", + ) + database.session.add(target_execution) + database.session.flush() + + for dataset in execution.datasets: + database.session.execute( + execution_datasets.insert().values( + execution_id=target_execution.id, + dataset_id=dataset.id, + ) + ) + elif mode == ReingestMode.replace: + _delete_execution_results(database, target_execution) + + if result.output_bundle_filename: + if mode != ReingestMode.additive: + database.session.execute( + delete(ExecutionOutput).where(ExecutionOutput.execution_id == target_execution.id) + ) + _handle_reingest_output_bundle( + config, + database, + target_execution, + result.to_output_path(result.output_bundle_filename), + ) + + _ingest_metrics(database, result, target_execution, cv, additive=mode == ReingestMode.additive) + + if mode == ReingestMode.versioned: + assert result.metric_bundle_filename is not None + target_execution.mark_successful(result.as_relative_path(result.metric_bundle_filename)) + + return target_execution + + +def _ingest_metrics( + database: "Database", + result: ExecutionResult, + execution: Execution, + cv: CV, + *, + additive: bool, +) -> None: + """Ingest scalar and series metric values, using additive dedup when requested.""" + if result.series_filename: + if additive: + _process_reingest_series_additive(database=database, result=result, execution=execution, cv=cv) + else: + _process_reingest_series(database=database, result=result, execution=execution, cv=cv) + + if additive: + _process_reingest_scalar_additive(database=database, result=result, execution=execution, cv=cv) + else: + _process_reingest_scalar(database=database, result=result, execution=execution, cv=cv) + + +def _copy_scratch_to_results( + config: "Config", + scratch_dir: pathlib.Path, + target_execution: Execution, + mode: ReingestMode, + original_results_dir: pathlib.Path, +) -> None: + """Copy re-extracted files from scratch to the results tree after DB success.""" + target_results_dir = config.paths.results / target_execution.output_fragment + if mode == ReingestMode.versioned: + if target_results_dir.exists(): + shutil.rmtree(target_results_dir) + shutil.copytree(scratch_dir, target_results_dir) + else: + shutil.copytree(scratch_dir, original_results_dir, dirs_exist_ok=True) + + +def reingest_execution( + config: "Config", + database: "Database", + execution: Execution, + provider_registry: "ProviderRegistry", + mode: ReingestMode = ReingestMode.additive, +) -> bool: + """ + Reingest an existing execution. + + Re-runs ``build_execution_result()`` and processes the results into the database. + + Parameters + ---------- + config + Application configuration + database + Database instance + execution + The ``Execution`` record to reingest + provider_registry + Registry of active providers (used to resolve the live diagnostic) + mode + Reingest mode: additive, replace, or versioned + + Returns + ------- + : + True if reingest was successful, False if it was skipped due to an error + """ + execution_group = execution.execution_group + diagnostic_model = execution_group.diagnostic + provider_slug = diagnostic_model.provider.slug + diagnostic_slug = diagnostic_model.slug + + try: + diagnostic = provider_registry.get_metric(provider_slug, diagnostic_slug) + except KeyError: + logger.error( + f"Could not resolve diagnostic {provider_slug}/{diagnostic_slug} " + f"from provider registry. Skipping execution {execution.id}." + ) + return False + + results_dir = config.paths.results / execution.output_fragment + + # Verify output directory exists + if not results_dir.exists(): + logger.error(f"Output directory does not exist: {results_dir}. Skipping execution {execution.id}.") + return False + + # Copy the results directory to scratch so that build_execution_result() + # can write CMEC bundles without mutating the live results tree. + # If anything fails, the original files remain untouched. + scratch_dir = _copy_results_to_scratch(config, execution.output_fragment) + + # Reconstruct the definition pointing at the scratch copy + definition = reconstruct_execution_definition(config, execution, diagnostic) + + # Re-run build_execution_result on the scratch copy + try: + result = diagnostic.build_execution_result(definition) + except Exception: + logger.exception( + f"build_execution_result failed for execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}). Skipping." + ) + return False + + if not result.successful or result.metric_bundle_filename is None: + logger.warning( + f"build_execution_result returned unsuccessful result for execution {execution.id}. Skipping." + ) + return False + + cv = CV.load_from_file(config.paths.dimensions_cv) + + # All mode-specific mutations happen inside a single savepoint so that + # any failure rolls back everything, preserving the original DB state. + try: + target_execution = _apply_reingest_mode( + config=config, + database=database, + execution=execution, + result=result, + mode=mode, + cv=cv, + ) + except Exception: + logger.exception( + f"Ingestion failed for execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}). Rolling back changes." + ) + return False + + # DB transaction succeeded -- copy files from scratch to the results tree. + _copy_scratch_to_results(config, scratch_dir, target_execution, mode, results_dir) + + logger.info( + f"Successfully reingested execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}) in {mode.value} mode" + ) + return True + + +def get_executions_for_reingest( + database: "Database", + *, + execution_group_ids: Sequence[int] | None = None, + provider_filters: list[str] | None = None, + diagnostic_filters: list[str] | None = None, + include_failed: bool = False, +) -> list[tuple[ExecutionGroup, Execution]]: + """ + Query executions eligible for reingest. + + Parameters + ---------- + database + Database instance + execution_group_ids + If provided, only include these execution group IDs + provider_filters + Filter by provider slug (substring, case-insensitive) + diagnostic_filters + Filter by diagnostic slug (substring, case-insensitive) + include_failed + If True, also include failed executions + + Returns + ------- + : + List of (ExecutionGroup, latest Execution) tuples + """ + # Use the existing filtered query + results = get_execution_group_and_latest_filtered( + database.session, + diagnostic_filters=diagnostic_filters, + provider_filters=provider_filters, + successful=None if include_failed else True, + ) + + # Filter by execution group IDs if provided + if execution_group_ids: + id_set = set(execution_group_ids) + results = [(eg, ex) for eg, ex in results if eg.id in id_set] + + # Filter out entries with no execution + return [(eg, ex) for eg, ex in results if ex is not None] diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py new file mode 100644 index 000000000..55e445dbc --- /dev/null +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -0,0 +1,641 @@ +"""Tests for the reingest module.""" + +import pathlib + +import pytest +from climate_ref_esmvaltool import provider as esmvaltool_provider +from climate_ref_pmp import provider as pmp_provider + +from climate_ref.executor.reingest import ( + ReingestMode, + _delete_execution_results, + _extract_dataset_attributes, + get_executions_for_reingest, + reconstruct_execution_definition, + reingest_execution, +) +from climate_ref.models import ScalarMetricValue +from climate_ref.models.dataset import CMIP6Dataset +from climate_ref.models.diagnostic import Diagnostic as DiagnosticModel +from climate_ref.models.execution import ( + Execution, + ExecutionGroup, + ExecutionOutput, +) +from climate_ref.models.metric_value import MetricValue +from climate_ref.models.provider import Provider as ProviderModel +from climate_ref.provider_registry import ProviderRegistry, _register_provider +from climate_ref_core.diagnostics import ExecutionResult +from climate_ref_core.metric_values import SeriesMetricValue as TSeries +from climate_ref_core.pycmec.metric import CMECMetric +from climate_ref_core.pycmec.output import CMECOutput + + +@pytest.fixture +def reingest_db(db, config): + """Set up a database with an execution group, execution, and result files on disk.""" + with db.session.begin(): + provider_model = ProviderModel(name="mock_provider", slug="mock_provider", version="v0.1.0") + db.session.add(provider_model) + db.session.flush() + + diag_model = DiagnosticModel( + name="mock", + slug="mock", + provider_id=provider_model.id, + ) + db.session.add(diag_model) + db.session.flush() + + eg = ExecutionGroup( + key="test-key", + diagnostic_id=diag_model.id, + selectors={"cmip6": [["source_id", "ACCESS-ESM1-5"], ["variable_id", "tas"]]}, + dirty=False, + ) + db.session.add(eg) + db.session.flush() + + execution = Execution( + execution_group_id=eg.id, + successful=True, + output_fragment="mock_provider/mock/abc123", + dataset_hash="hash1", + ) + db.session.add(execution) + db.session.flush() + + return db + + +@pytest.fixture +def reingest_execution_obj(reingest_db): + """Get the execution object from the reingest_db fixture.""" + return reingest_db.session.query(Execution).one() + + +@pytest.fixture +def mock_provider_registry(provider): + """Create a mock ProviderRegistry that returns the test provider.""" + return ProviderRegistry(providers=[provider]) + + +@pytest.fixture +def output_dir_with_results(config, reingest_execution_obj): + """Create a results directory with CMEC output files.""" + results_dir = config.paths.results / reingest_execution_obj.output_fragment + results_dir.mkdir(parents=True, exist_ok=True) + + cmec_metric = CMECMetric(**CMECMetric.create_template()) + cmec_metric.dump_to_json(results_dir / "diagnostic.json") + + cmec_output = CMECOutput(**CMECOutput.create_template()) + cmec_output.dump_to_json(results_dir / "output.json") + + series_data = [ + TSeries( + dimensions={"source_id": "test-model"}, + values=[1.0, 2.0, 3.0], + index=[0, 1, 2], + index_name="time", + attributes={"units": "K"}, + ) + ] + TSeries.dump_to_json(results_dir / "series.json", series_data) + + return results_dir + + +class TestReingestMode: + def test_enum_values(self): + assert ReingestMode.additive.value == "additive" + assert ReingestMode.replace.value == "replace" + assert ReingestMode.versioned.value == "versioned" + + def test_from_string(self): + assert ReingestMode("additive") == ReingestMode.additive + assert ReingestMode("replace") == ReingestMode.replace + assert ReingestMode("versioned") == ReingestMode.versioned + + +class TestExtractDatasetAttributes: + def test_extracts_cmip6_attributes(self, db_seeded): + dataset = db_seeded.session.query(CMIP6Dataset).first() + attrs = _extract_dataset_attributes(dataset) + + assert "variable_id" in attrs + assert "source_id" in attrs + assert "instance_id" in attrs + assert "id" not in attrs + assert "dataset_type" not in attrs + + +class TestReconstructExecutionDefinition: + def test_reconstruct_basic(self, config, reingest_db, reingest_execution_obj, provider): + """Test that a basic execution definition can be reconstructed.""" + diagnostic = provider.get("mock") + definition = reconstruct_execution_definition(config, reingest_execution_obj, diagnostic) + + assert definition.key == "test-key" + assert definition.diagnostic is diagnostic + # Definition points at scratch (not results) so build_execution_result + # writes to a safe location. + assert definition.output_directory == config.paths.scratch / "mock_provider/mock/abc123" + + def test_reconstruct_output_directory_under_scratch( + self, config, reingest_db, reingest_execution_obj, provider + ): + """Output directory should be under scratch for safe re-extraction.""" + diagnostic = provider.get("mock") + definition = reconstruct_execution_definition(config, reingest_execution_obj, diagnostic) + + assert str(definition.output_directory).startswith(str(config.paths.scratch)) + + +class TestDeleteExecutionResults: + def test_deletes_metric_values_and_outputs(self, reingest_db, reingest_execution_obj): + """Deleting results should remove all metric values and outputs.""" + execution = reingest_execution_obj + + # Add some metric values and outputs + reingest_db.session.add( + ScalarMetricValue( + execution_id=execution.id, + value=42.0, + attributes={"test": True}, + ) + ) + reingest_db.session.add( + ExecutionOutput( + execution_id=execution.id, + output_type="plot", + filename="test.png", + short_name="test", + ) + ) + reingest_db.session.commit() + + assert reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() == 1 + assert reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).count() == 1 + + _delete_execution_results(reingest_db, execution) + reingest_db.session.commit() + + assert reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() == 0 + assert reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).count() == 0 + + +class TestReingestExecution: + def test_reingest_missing_output_dir( + self, config, reingest_db, reingest_execution_obj, mock_provider_registry + ): + """Should return False when output directory doesn't exist.""" + result = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + assert result is False + + def test_reingest_unresolvable_diagnostic(self, config, reingest_db, reingest_execution_obj): + """Should return False when provider registry can't resolve diagnostic.""" + empty_registry = ProviderRegistry(providers=[]) + result = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=empty_registry, + mode=ReingestMode.replace, + ) + assert result is False + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_reingest_replace_mode( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Replace mode should delete existing values and re-ingest.""" + execution = reingest_execution_obj + + # Pre-populate some metric values + reingest_db.session.add( + ScalarMetricValue( + execution_id=execution.id, + value=99.0, + attributes={"old": True}, + ) + ) + reingest_db.session.commit() + + old_count = reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() + assert old_count == 1 + + # Mock build_execution_result to return a controlled result + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = pathlib.Path("output.json") + mock_result.series_filename = pathlib.Path("series.json") + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=execution, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + reingest_db.session.commit() + + assert ok is True + # Old value (99.0) should be gone + old_vals = ( + reingest_db.session.query(ScalarMetricValue) + .filter_by(execution_id=execution.id) + .filter(ScalarMetricValue.value == 99.0) + .all() + ) + assert len(old_vals) == 0 + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_reingest_versioned_creates_new_execution( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Versioned mode should create a new Execution record.""" + original_id = reingest_execution_obj.id + original_count = reingest_db.session.query(Execution).count() + + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = pathlib.Path("output.json") + mock_result.series_filename = pathlib.Path("series.json") + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.versioned, + ) + reingest_db.session.commit() + + assert ok is True + new_count = reingest_db.session.query(Execution).count() + assert new_count == original_count + 1 + + # Original execution should be untouched + original = reingest_db.session.get(Execution, original_id) + assert original is not None + + def test_reingest_build_execution_result_failure( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Should return False and skip when build_execution_result raises.""" + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mocker.patch.object( + mock_diagnostic, + "build_execution_result", + side_effect=RuntimeError("Extraction failed"), + ) + + result = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + assert result is False + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_reingest_does_not_touch_dirty_flag( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """The dirty flag should remain unchanged after reingest. + + Note: reingest_execution itself doesn't manage the dirty flag -- + that's the CLI's responsibility. This test verifies that reingest_execution + does not set dirty=False (unlike handle_execution_result). + """ + eg = reingest_execution_obj.execution_group + eg.dirty = True + reingest_db.session.commit() + + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = None + mock_result.series_filename = None + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + reingest_db.session.commit() + + reingest_db.session.refresh(eg) + assert eg.dirty is True + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_additive_preserves_existing_values( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Additive mode should keep pre-existing metric values untouched.""" + execution = reingest_execution_obj + + # Pre-populate with a value that has a unique dimension signature + reingest_db.session.add( + ScalarMetricValue( + execution_id=execution.id, + value=42.0, + attributes={"pre_existing": True}, + source_id="pre-existing-model", + ) + ) + reingest_db.session.commit() + + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = None + mock_result.series_filename = None + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + reingest_execution( + config=config, + database=reingest_db, + execution=execution, + provider_registry=mock_provider_registry, + mode=ReingestMode.additive, + ) + reingest_db.session.commit() + + # The pre-existing value should still be there + pre_existing = ( + reingest_db.session.query(ScalarMetricValue) + .filter_by(execution_id=execution.id) + .filter(ScalarMetricValue.value == 42.0) + .all() + ) + assert len(pre_existing) == 1, "Additive mode should preserve pre-existing values" + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_additive_reingest_is_idempotent( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Running additive reingest twice should not create duplicate rows.""" + execution = reingest_execution_obj + + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = pathlib.Path("output.json") + mock_result.series_filename = pathlib.Path("series.json") + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + # First reingest + reingest_execution( + config=config, + database=reingest_db, + execution=execution, + provider_registry=mock_provider_registry, + mode=ReingestMode.additive, + ) + reingest_db.session.commit() + + count_after_first = ( + reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() + ) + series_count_first = ( + reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() + ) + + # Second reingest (should be idempotent) + reingest_execution( + config=config, + database=reingest_db, + execution=execution, + provider_registry=mock_provider_registry, + mode=ReingestMode.additive, + ) + reingest_db.session.commit() + + count_after_second = ( + reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() + ) + series_count_second = ( + reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() + ) + + assert count_after_first == count_after_second, ( + f"Additive reingest created duplicates: {count_after_first} -> {count_after_second}" + ) + assert series_count_first == series_count_second, ( + f"Additive reingest created duplicate series: {series_count_first} -> {series_count_second}" + ) + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_replace_preserves_data_on_ingestion_failure( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """If ingestion fails in replace mode, original data should be preserved.""" + execution = reingest_execution_obj + + # Pre-populate with known values + reingest_db.session.add( + ScalarMetricValue( + execution_id=execution.id, + value=42.0, + attributes={"original": True}, + ) + ) + reingest_db.session.commit() + + original_count = ( + reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() + ) + assert original_count == 1 + + # Mock build_execution_result to return a result that will fail during scalar ingestion + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = None + mock_result.series_filename = None + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + # Make the scalar ingestion fail by corrupting the metric bundle + (output_dir_with_results / "diagnostic.json").write_text("not valid json") + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=execution, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + reingest_db.session.commit() + + assert ok is False + + # Original data should be preserved since the savepoint rolled back + preserved_count = ( + reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() + ) + assert preserved_count == original_count, ( + f"Replace mode lost data on failure: {original_count} -> {preserved_count}" + ) + + +class TestGetExecutionsForReingest: + def test_filters_by_success(self, db_seeded): + """By default should only return successful executions.""" + with db_seeded.session.begin(): + _register_provider(db_seeded, pmp_provider) + _register_provider(db_seeded, esmvaltool_provider) + + diag = db_seeded.session.query(DiagnosticModel).first() + eg = ExecutionGroup(key="test-filter", diagnostic_id=diag.id, selectors={}) + db_seeded.session.add(eg) + db_seeded.session.flush() + + db_seeded.session.add( + Execution( + execution_group_id=eg.id, + successful=True, + output_fragment="out-s", + dataset_hash="h1", + ) + ) + + results = get_executions_for_reingest(db_seeded, execution_group_ids=[eg.id], include_failed=False) + assert len(results) == 1 + assert results[0][1].successful is True + + def test_include_failed(self, db_seeded): + """With include_failed=True, should also return failed executions.""" + with db_seeded.session.begin(): + _register_provider(db_seeded, pmp_provider) + _register_provider(db_seeded, esmvaltool_provider) + + diag = db_seeded.session.query(DiagnosticModel).first() + eg = ExecutionGroup(key="test-failed", diagnostic_id=diag.id, selectors={}) + db_seeded.session.add(eg) + db_seeded.session.flush() + + db_seeded.session.add( + Execution( + execution_group_id=eg.id, + successful=False, + output_fragment="out-f", + dataset_hash="h2", + ) + ) + + results = get_executions_for_reingest(db_seeded, execution_group_ids=[eg.id], include_failed=True) + assert len(results) >= 1 + + def test_filters_by_group_ids(self, db_seeded): + """Should only return executions for specified group IDs.""" + with db_seeded.session.begin(): + _register_provider(db_seeded, pmp_provider) + + diag = db_seeded.session.query(DiagnosticModel).first() + eg1 = ExecutionGroup(key="group-a", diagnostic_id=diag.id, selectors={}) + eg2 = ExecutionGroup(key="group-b", diagnostic_id=diag.id, selectors={}) + db_seeded.session.add_all([eg1, eg2]) + db_seeded.session.flush() + + db_seeded.session.add( + Execution( + execution_group_id=eg1.id, + successful=True, + output_fragment="out-a", + dataset_hash="ha", + ) + ) + db_seeded.session.add( + Execution( + execution_group_id=eg2.id, + successful=True, + output_fragment="out-b", + dataset_hash="hb", + ) + ) + + results = get_executions_for_reingest(db_seeded, execution_group_ids=[eg1.id]) + assert len(results) == 1 + assert results[0][0].id == eg1.id From 6ec695a4e1973c276a6e70d97171079f716c027a Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Tue, 31 Mar 2026 20:25:46 +0900 Subject: [PATCH 02/19] refactor(reingest): consolidate duplicated ingestion functions and add scratch cleanup Collapse four near-identical metric ingestion functions into two by adding an optional `existing` parameter for additive dedup. Hoist the dimension query into `_ingest_metrics` to avoid duplicate DB queries. Fix double iteration of `iter_results()` generator. Add try/finally scratch directory cleanup to prevent disk accumulation during batch reingests. Remove redundant dirty-flag save/restore in CLI. --- .../src/climate_ref/cli/executions.py | 7 - .../src/climate_ref/executor/reingest.py | 300 +++++++----------- 2 files changed, 116 insertions(+), 191 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/cli/executions.py b/packages/climate-ref/src/climate_ref/cli/executions.py index 574be90d0..0cf5c9b75 100644 --- a/packages/climate-ref/src/climate_ref/cli/executions.py +++ b/packages/climate-ref/src/climate_ref/cli/executions.py @@ -875,10 +875,6 @@ def reingest( # noqa: PLR0913 skip_count = 0 for eg, ex in results: with db.session.begin(): - # Safety net: reingest_execution does not modify dirty, but we - # save/restore as a guard against future regressions. - dirty_before = eg.dirty - ok = reingest_execution( config=config, database=db, @@ -887,9 +883,6 @@ def reingest( # noqa: PLR0913 mode=mode, ) - # Restore dirty flag (reingest must not modify it) - eg.dirty = dirty_before - if ok: success_count += 1 else: diff --git a/packages/climate-ref/src/climate_ref/executor/reingest.py b/packages/climate-ref/src/climate_ref/executor/reingest.py index 4e743f43d..f864e8695 100644 --- a/packages/climate-ref/src/climate_ref/executor/reingest.py +++ b/packages/climate-ref/src/climate_ref/executor/reingest.py @@ -230,85 +230,6 @@ def _handle_reingest_outputs( ) -def _process_reingest_series( - database: "Database", - result: ExecutionResult, - execution: Execution, - cv: CV, -) -> None: - """ - Process series values for reingest. - - Like ``_process_execution_series`` but skips the file copy since the series - file is already in the results directory. Unlike the normal ingestion path, - errors are raised rather than swallowed so that the caller can roll back. - """ - assert result.series_filename, "Series filename must be set in the result" - - # Load the series values directly from the results directory - series_values_path = result.to_output_path(result.series_filename) - series_values = TSeries.load_from_json(series_values_path) - - try: - cv.validate_metrics(series_values) - except (ResultValidationError, AssertionError): - logger.exception("Diagnostic values do not conform with the controlled vocabulary") - - series_values_content = [ - { - "execution_id": execution.id, - "values": series_result.values, - "attributes": series_result.attributes, - "index": series_result.index, - "index_name": series_result.index_name, - **series_result.dimensions, - } - for series_result in series_values - ] - logger.debug(f"Ingesting {len(series_values)} series values for execution {execution.id}") - if series_values: - database.session.execute( - insert(SeriesMetricValue), - series_values_content, - ) - - -def _process_reingest_scalar( - database: "Database", - result: ExecutionResult, - execution: Execution, - cv: CV, -) -> None: - """ - Process scalar values for reingest. - - Like ``_process_execution_scalar`` but raises on insertion errors - so the caller can roll back. - """ - cmec_metric_bundle = CMECMetric.load_from_json(result.to_output_path(result.metric_bundle_filename)) - - try: - cv.validate_metrics(cmec_metric_bundle) - except (ResultValidationError, AssertionError): - logger.exception("Diagnostic values do not conform with the controlled vocabulary") - - scalar_values = [ - { - "execution_id": execution.id, - "value": metric_result.value, - "attributes": metric_result.attributes, - **metric_result.dimensions, - } - for metric_result in cmec_metric_bundle.iter_results() - ] - logger.debug(f"Ingesting {len(scalar_values)} scalar values for execution {execution.id}") - if scalar_values: - database.session.execute( - insert(ScalarMetricValue), - scalar_values, - ) - - def _get_existing_metric_dimensions( database: "Database", execution: Execution ) -> set[tuple[tuple[str, str], ...]]: @@ -325,95 +246,109 @@ def _get_existing_metric_dimensions( return sigs -def _process_reingest_scalar_additive( +def _process_reingest_series( database: "Database", result: ExecutionResult, execution: Execution, cv: CV, + *, + existing: set[tuple[tuple[str, str], ...]] | None = None, ) -> None: """ - Process scalar values for additive reingest. + Process series values for reingest. - Only inserts values whose dimension signatures are not already present - for this execution. + When ``existing`` is provided, only inserts series whose dimension + signatures are not already present (additive mode). """ - cmec_metric_bundle = CMECMetric.load_from_json(result.to_output_path(result.metric_bundle_filename)) + assert result.series_filename, "Series filename must be set in the result" + + series_values_path = result.to_output_path(result.series_filename) + series_values = TSeries.load_from_json(series_values_path) try: - cv.validate_metrics(cmec_metric_bundle) + cv.validate_metrics(series_values) except (ResultValidationError, AssertionError): logger.exception("Diagnostic values do not conform with the controlled vocabulary") - existing = _get_existing_metric_dimensions(database, execution) - new_values = [] - for metric_result in cmec_metric_bundle.iter_results(): - dims = tuple(sorted(metric_result.dimensions.items())) - if dims not in existing: - new_values.append( - { - "execution_id": execution.id, - "value": metric_result.value, - "attributes": metric_result.attributes, - **metric_result.dimensions, - } - ) + for series_result in series_values: + if existing is not None: + dims = tuple(sorted(series_result.dimensions.items())) + if dims in existing: + continue + new_values.append( + { + "execution_id": execution.id, + "values": series_result.values, + "attributes": series_result.attributes, + "index": series_result.index, + "index_name": series_result.index_name, + **series_result.dimensions, + } + ) + + skipped = len(series_values) - len(new_values) + if existing is not None: + logger.debug( + f"Additive: {len(new_values)} new series values " + f"(skipped {skipped} existing) for execution {execution.id}" + ) + else: + logger.debug(f"Ingesting {len(new_values)} series values for execution {execution.id}") - logger.debug( - f"Additive: {len(new_values)} new scalar values " - f"(skipped {len(list(cmec_metric_bundle.iter_results())) - len(new_values)} existing) " - f"for execution {execution.id}" - ) if new_values: - database.session.execute(insert(ScalarMetricValue), new_values) + database.session.execute(insert(SeriesMetricValue), new_values) -def _process_reingest_series_additive( +def _process_reingest_scalar( database: "Database", result: ExecutionResult, execution: Execution, cv: CV, + *, + existing: set[tuple[tuple[str, str], ...]] | None = None, ) -> None: """ - Process series values for additive reingest. + Process scalar values for reingest. - Only inserts series whose dimension signatures are not already present - for this execution. + When ``existing`` is provided, only inserts values whose dimension + signatures are not already present (additive mode). """ - assert result.series_filename, "Series filename must be set in the result" - - series_values_path = result.to_output_path(result.series_filename) - series_values = TSeries.load_from_json(series_values_path) + cmec_metric_bundle = CMECMetric.load_from_json(result.to_output_path(result.metric_bundle_filename)) try: - cv.validate_metrics(series_values) + cv.validate_metrics(cmec_metric_bundle) except (ResultValidationError, AssertionError): logger.exception("Diagnostic values do not conform with the controlled vocabulary") - existing = _get_existing_metric_dimensions(database, execution) - new_values = [] - for series_result in series_values: - dims = tuple(sorted(series_result.dimensions.items())) - if dims not in existing: - new_values.append( - { - "execution_id": execution.id, - "values": series_result.values, - "attributes": series_result.attributes, - "index": series_result.index, - "index_name": series_result.index_name, - **series_result.dimensions, - } - ) + total_count = 0 + for metric_result in cmec_metric_bundle.iter_results(): + total_count += 1 + if existing is not None: + dims = tuple(sorted(metric_result.dimensions.items())) + if dims in existing: + continue + new_values.append( + { + "execution_id": execution.id, + "value": metric_result.value, + "attributes": metric_result.attributes, + **metric_result.dimensions, + } + ) + + skipped = total_count - len(new_values) + if existing is not None: + logger.debug( + f"Additive: {len(new_values)} new scalar values " + f"(skipped {skipped} existing) for execution {execution.id}" + ) + else: + logger.debug(f"Ingesting {len(new_values)} scalar values for execution {execution.id}") - logger.debug( - f"Additive: {len(new_values)} new series values " - f"(skipped {len(series_values) - len(new_values)} existing) " - f"for execution {execution.id}" - ) if new_values: - database.session.execute(insert(SeriesMetricValue), new_values) + database.session.execute(insert(ScalarMetricValue), new_values) def _copy_results_to_scratch( @@ -516,16 +451,14 @@ def _ingest_metrics( additive: bool, ) -> None: """Ingest scalar and series metric values, using additive dedup when requested.""" + existing = _get_existing_metric_dimensions(database, execution) if additive else None + if result.series_filename: - if additive: - _process_reingest_series_additive(database=database, result=result, execution=execution, cv=cv) - else: - _process_reingest_series(database=database, result=result, execution=execution, cv=cv) + _process_reingest_series( + database=database, result=result, execution=execution, cv=cv, existing=existing + ) - if additive: - _process_reingest_scalar_additive(database=database, result=result, execution=execution, cv=cv) - else: - _process_reingest_scalar(database=database, result=result, execution=execution, cv=cv) + _process_reingest_scalar(database=database, result=result, execution=execution, cv=cv, existing=existing) def _copy_scratch_to_results( @@ -601,53 +534,52 @@ def reingest_execution( # If anything fails, the original files remain untouched. scratch_dir = _copy_results_to_scratch(config, execution.output_fragment) - # Reconstruct the definition pointing at the scratch copy - definition = reconstruct_execution_definition(config, execution, diagnostic) - - # Re-run build_execution_result on the scratch copy try: - result = diagnostic.build_execution_result(definition) - except Exception: - logger.exception( - f"build_execution_result failed for execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}). Skipping." - ) - return False + definition = reconstruct_execution_definition(config, execution, diagnostic) + + try: + result = diagnostic.build_execution_result(definition) + except Exception: + logger.exception( + f"build_execution_result failed for execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}). Skipping." + ) + return False - if not result.successful or result.metric_bundle_filename is None: - logger.warning( - f"build_execution_result returned unsuccessful result for execution {execution.id}. Skipping." - ) - return False + if not result.successful or result.metric_bundle_filename is None: + logger.warning( + f"build_execution_result returned unsuccessful result for execution {execution.id}. Skipping." + ) + return False + + cv = CV.load_from_file(config.paths.dimensions_cv) + + try: + target_execution = _apply_reingest_mode( + config=config, + database=database, + execution=execution, + result=result, + mode=mode, + cv=cv, + ) + except Exception: + logger.exception( + f"Ingestion failed for execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}). Rolling back changes." + ) + return False - cv = CV.load_from_file(config.paths.dimensions_cv) + _copy_scratch_to_results(config, scratch_dir, target_execution, mode, results_dir) - # All mode-specific mutations happen inside a single savepoint so that - # any failure rolls back everything, preserving the original DB state. - try: - target_execution = _apply_reingest_mode( - config=config, - database=database, - execution=execution, - result=result, - mode=mode, - cv=cv, - ) - except Exception: - logger.exception( - f"Ingestion failed for execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}). Rolling back changes." + logger.info( + f"Successfully reingested execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}) in {mode.value} mode" ) - return False - - # DB transaction succeeded -- copy files from scratch to the results tree. - _copy_scratch_to_results(config, scratch_dir, target_execution, mode, results_dir) - - logger.info( - f"Successfully reingested execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}) in {mode.value} mode" - ) - return True + return True + finally: + if scratch_dir.exists(): + shutil.rmtree(scratch_dir) def get_executions_for_reingest( From 42a9e338a6c6b7eb9d0817605fd61e87edfdbc8c Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Tue, 31 Mar 2026 20:36:15 +0900 Subject: [PATCH 03/19] style(cli): remove redundant inline comments in reingest command --- packages/climate-ref/src/climate_ref/cli/executions.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/cli/executions.py b/packages/climate-ref/src/climate_ref/cli/executions.py index 0cf5c9b75..f2ec86cc9 100644 --- a/packages/climate-ref/src/climate_ref/cli/executions.py +++ b/packages/climate-ref/src/climate_ref/cli/executions.py @@ -817,7 +817,6 @@ def reingest( # noqa: PLR0913 db = ctx.obj.database console = ctx.obj.console - # Require at least one filter to prevent accidental full reingest if not any([group_ids, provider, diagnostic]): logger.error( "At least one filter is required (group IDs, --provider, or --diagnostic). " @@ -825,10 +824,8 @@ def reingest( # noqa: PLR0913 ) raise typer.Exit(code=1) - # Build provider registry provider_registry = ProviderRegistry.build_from_config(config, db) - # Query eligible executions results = get_executions_for_reingest( db, execution_group_ids=group_ids, @@ -841,7 +838,6 @@ def reingest( # noqa: PLR0913 console.print("No executions found matching the specified criteria.") return - # Build preview table preview_df = pd.DataFrame( [ { @@ -861,7 +857,6 @@ def reingest( # noqa: PLR0913 pretty_print_df(preview_df, console=console) return - # Show preview and confirm console.print(f"Will reingest {len(results)} execution(s) in [bold]{mode.value}[/] mode:") pretty_print_df(preview_df, console=console) @@ -870,7 +865,8 @@ def reingest( # noqa: PLR0913 console.print("Reingest cancelled.") return - # Process each execution + # Process each execution in a separate session + # This rolls back any failures success_count = 0 skip_count = 0 for eg, ex in results: From 484d6bcf1470c8c45d88ddcb7e761544ba37d6c4 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Tue, 31 Mar 2026 20:39:52 +0900 Subject: [PATCH 04/19] chore: add changelog entry for PR #610 --- changelog/610.feature.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/610.feature.md diff --git a/changelog/610.feature.md b/changelog/610.feature.md new file mode 100644 index 000000000..b2c759346 --- /dev/null +++ b/changelog/610.feature.md @@ -0,0 +1 @@ +Added `ref executions reingest` command to re-ingest existing execution results without re-running diagnostics. Supports three modes: `additive` (keep existing values, add new), `replace` (delete and re-ingest), and `versioned` (create new execution record). From f977aa156a9cd3b0a34338962bdfd2547da49f93 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Thu, 2 Apr 2026 12:17:21 +0900 Subject: [PATCH 05/19] fix(reingest): address PR review comments - Fix ensure_relative_path to fall back to scratch base when output bundle contains absolute paths under scratch directory - Always clear ExecutionOutput rows before re-inserting in all modes (fixes duplicate outputs in additive mode) - Add _n suffix for repeated versioned reingests on the same execution - Mark execution as successful for all modes, not just versioned - Add path traversal safety checks in copy operations - Document single-threaded scratch directory assumption - Fix test to use ResultOutputType enum instead of raw string - Fix comment wording (session -> transaction) - Add tests for versioned re-run uniqueness, mark_successful on failed executions, and scratch directory cleanup --- .../src/climate_ref/cli/executions.py | 3 +- .../src/climate_ref/executor/reingest.py | 41 ++++- .../tests/unit/executor/test_reingest.py | 157 +++++++++++++++++- 3 files changed, 189 insertions(+), 12 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/cli/executions.py b/packages/climate-ref/src/climate_ref/cli/executions.py index f2ec86cc9..ae2a48f5f 100644 --- a/packages/climate-ref/src/climate_ref/cli/executions.py +++ b/packages/climate-ref/src/climate_ref/cli/executions.py @@ -865,8 +865,7 @@ def reingest( # noqa: PLR0913 console.print("Reingest cancelled.") return - # Process each execution in a separate session - # This rolls back any failures + # Process each execution in a separate transaction success_count = 0 skip_count = 0 for eg, ex in results: diff --git a/packages/climate-ref/src/climate_ref/executor/reingest.py b/packages/climate-ref/src/climate_ref/executor/reingest.py index f864e8695..7e683aead 100644 --- a/packages/climate-ref/src/climate_ref/executor/reingest.py +++ b/packages/climate-ref/src/climate_ref/executor/reingest.py @@ -214,9 +214,16 @@ def _handle_reingest_outputs( """Register outputs in the DB without copying files (they are already in place).""" outputs = outputs or {} results_base = config.paths.results / execution.output_fragment + scratch_base = config.paths.scratch / execution.output_fragment for key, output_info in outputs.items(): - filename = ensure_relative_path(output_info.filename, results_base) + # Output bundles produced during re-extraction may contain absolute + # paths under the scratch directory. Fall back to scratch_base when + # the filename is not relative to results_base. + try: + filename = ensure_relative_path(output_info.filename, results_base) + except ValueError: + filename = ensure_relative_path(output_info.filename, scratch_base) database.session.add( ExecutionOutput.build( execution_id=execution.id, @@ -365,9 +372,19 @@ def _copy_results_to_scratch( the results tree unchanged until we decide to commit. Returns the scratch output directory. + + Note: the scratch path is deterministic (scratch/output_fragment), so + concurrent reingest of the same execution is not supported. The CLI + is single-threaded, so this is not a practical concern. """ src = config.paths.results / output_fragment dst = config.paths.scratch / output_fragment + + if not src.is_relative_to(config.paths.results): + raise ValueError(f"Unsafe source path: {src} is not under {config.paths.results}") + if not dst.is_relative_to(config.paths.scratch): + raise ValueError(f"Unsafe destination path: {dst} is not under {config.paths.scratch}") + if dst.exists(): shutil.rmtree(dst) shutil.copytree(src, dst) @@ -403,10 +420,16 @@ def _apply_reingest_mode( # noqa: PLR0913 f"{execution.output_fragment}-reingest-{execution.id}".encode() ).hexdigest()[:12] + base_fragment = f"{execution.output_fragment}_v{version_hash}" + existing_count = sum( + 1 for e in execution_group.executions if e.output_fragment.startswith(base_fragment) + ) + version_suffix = f"_n{existing_count + 1}" if existing_count > 0 else "" + target_execution = Execution( execution_group=execution_group, dataset_hash=execution.dataset_hash, - output_fragment=f"{execution.output_fragment}_v{version_hash}", + output_fragment=f"{base_fragment}{version_suffix}", ) database.session.add(target_execution) database.session.flush() @@ -422,10 +445,9 @@ def _apply_reingest_mode( # noqa: PLR0913 _delete_execution_results(database, target_execution) if result.output_bundle_filename: - if mode != ReingestMode.additive: - database.session.execute( - delete(ExecutionOutput).where(ExecutionOutput.execution_id == target_execution.id) - ) + database.session.execute( + delete(ExecutionOutput).where(ExecutionOutput.execution_id == target_execution.id) + ) _handle_reingest_output_bundle( config, database, @@ -435,9 +457,8 @@ def _apply_reingest_mode( # noqa: PLR0913 _ingest_metrics(database, result, target_execution, cv, additive=mode == ReingestMode.additive) - if mode == ReingestMode.versioned: - assert result.metric_bundle_filename is not None - target_execution.mark_successful(result.as_relative_path(result.metric_bundle_filename)) + assert result.metric_bundle_filename is not None + target_execution.mark_successful(result.as_relative_path(result.metric_bundle_filename)) return target_execution @@ -470,6 +491,8 @@ def _copy_scratch_to_results( ) -> None: """Copy re-extracted files from scratch to the results tree after DB success.""" target_results_dir = config.paths.results / target_execution.output_fragment + if not target_results_dir.is_relative_to(config.paths.results): + raise ValueError(f"Unsafe target path: {target_results_dir} is not under {config.paths.results}") if mode == ReingestMode.versioned: if target_results_dir.exists(): shutil.rmtree(target_results_dir) diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index 55e445dbc..bdd3054f9 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -21,6 +21,7 @@ Execution, ExecutionGroup, ExecutionOutput, + ResultOutputType, ) from climate_ref.models.metric_value import MetricValue from climate_ref.models.provider import Provider as ProviderModel @@ -168,7 +169,7 @@ def test_deletes_metric_values_and_outputs(self, reingest_db, reingest_execution reingest_db.session.add( ExecutionOutput( execution_id=execution.id, - output_type="plot", + output_type=ResultOutputType.Plot, filename="test.png", short_name="test", ) @@ -559,6 +560,160 @@ def test_replace_preserves_data_on_ingestion_failure( f"Replace mode lost data on failure: {original_count} -> {preserved_count}" ) + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_versioned_reingest_twice_creates_unique_fragments( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Running versioned reingest twice should create distinct output fragments.""" + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = pathlib.Path("output.json") + mock_result.series_filename = pathlib.Path("series.json") + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + # First versioned reingest + ok1 = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.versioned, + ) + reingest_db.session.commit() + assert ok1 is True + + # Second versioned reingest + reingest_db.session.refresh(reingest_execution_obj) + ok2 = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.versioned, + ) + reingest_db.session.commit() + assert ok2 is True + + # Should have 3 executions total: original + 2 versioned + all_executions = reingest_db.session.query(Execution).all() + assert len(all_executions) == 3 + + fragments = [e.output_fragment for e in all_executions] + assert len(set(fragments)) == 3, f"Expected unique fragments, got: {fragments}" + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_reingest_marks_failed_execution_as_successful( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Reingest should mark a previously-failed execution as successful.""" + reingest_execution_obj.successful = False + reingest_db.session.commit() + assert reingest_execution_obj.successful is False + + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = None + mock_result.series_filename = None + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + reingest_db.session.commit() + + assert ok is True + reingest_db.session.refresh(reingest_execution_obj) + assert reingest_execution_obj.successful is True + + def test_scratch_directory_cleaned_up_on_success( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Scratch directory should be removed after successful reingest.""" + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = None + mock_result.series_filename = None + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results + mock_result.as_relative_path = pathlib.Path + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + scratch_dir = config.paths.scratch / reingest_execution_obj.output_fragment + + reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.additive, + ) + + assert not scratch_dir.exists(), f"Scratch directory was not cleaned up: {scratch_dir}" + + def test_scratch_directory_cleaned_up_on_failure( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Scratch directory should be removed even when reingest fails.""" + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mocker.patch.object( + mock_diagnostic, + "build_execution_result", + side_effect=RuntimeError("Extraction failed"), + ) + + scratch_dir = config.paths.scratch / reingest_execution_obj.output_fragment + + result = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + + assert result is False + assert not scratch_dir.exists(), f"Scratch directory was not cleaned up on failure: {scratch_dir}" + class TestGetExecutionsForReingest: def test_filters_by_success(self, db_seeded): From 7d629f1e808b041b87878c57870540964c3cec25 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Thu, 2 Apr 2026 12:26:52 +0900 Subject: [PATCH 06/19] fix(executor): remove eager reingest import from executor __init__ The reingest module imports pandas, CMECMetric, and other heavy dependencies that are not available when the package is installed without extras. The CLI imports directly from climate_ref.executor.reingest, so the re-export is unnecessary. --- packages/climate-ref/src/climate_ref/executor/__init__.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/executor/__init__.py b/packages/climate-ref/src/climate_ref/executor/__init__.py index 57cba3cfa..d5bd99ce3 100644 --- a/packages/climate-ref/src/climate_ref/executor/__init__.py +++ b/packages/climate-ref/src/climate_ref/executor/__init__.py @@ -18,15 +18,12 @@ HPCExecutor = exc # type: ignore from .local import LocalExecutor -from .reingest import ReingestMode, reingest_execution from .result_handling import handle_execution_result from .synchronous import SynchronousExecutor __all__ = [ "HPCExecutor", "LocalExecutor", - "ReingestMode", "SynchronousExecutor", "handle_execution_result", - "reingest_execution", ] From 08d8ddb5da0cca36b5a6282b19fa816145b31fe6 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Thu, 2 Apr 2026 12:51:53 +0900 Subject: [PATCH 07/19] chore: fix update to environs api --- .../src/climate_ref_core/env.py | 2 +- stubs/environs/__init__.pyi | 17 ----------------- 2 files changed, 1 insertion(+), 18 deletions(-) delete mode 100644 stubs/environs/__init__.pyi diff --git a/packages/climate-ref-core/src/climate_ref_core/env.py b/packages/climate-ref-core/src/climate_ref_core/env.py index 75b33d1dd..03edaf838 100644 --- a/packages/climate-ref-core/src/climate_ref_core/env.py +++ b/packages/climate-ref-core/src/climate_ref_core/env.py @@ -27,7 +27,7 @@ def get_env() -> Env: # Load the environment variables from the .env file # This will override any defaults set above - env.read_env(verbose=True) + env.read_env() return env diff --git a/stubs/environs/__init__.pyi b/stubs/environs/__init__.pyi deleted file mode 100644 index 42d589ebe..000000000 --- a/stubs/environs/__init__.pyi +++ /dev/null @@ -1,17 +0,0 @@ -from pathlib import Path - -class Env: - def __init__(self, expand_vars: bool) -> None: ... - def read_env( - self, - path: str | None = None, - recurse: bool = True, - verbose: bool = False, - override: bool = False, - ) -> bool: ... - def path(self, name: str, default: Path | str | None = None) -> Path: ... - def bool(self, name: str, default: bool | None = None) -> bool: ... - def list(self, name: str, default: list[str] | None) -> list[str]: ... - # Ordering matters here and I don't know why - # This `str` method must be declared last to avoid a conflict with the `str` types above - def str(self, name: str, default: str | None = None) -> str: ... From 557eee4a92fc37cf695f2a0d4af6529994c9e76e Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Thu, 2 Apr 2026 15:49:56 +0900 Subject: [PATCH 08/19] test(reingest): improve coverage to 74% with targeted unit tests Add tests for previously uncovered helper functions: - _copy_results_to_scratch: path traversal rejection and directory copy - _get_existing_metric_dimensions: empty and populated cases - _handle_reingest_output_bundle: output registration with real data - _ingest_metrics: scalar ingestion and additive dedup with real CMEC bundles - reconstruct_execution_definition: with linked CMIP6 datasets Add output_dir_with_data fixture with valid 2-level nested CMECMetric JSON and output bundles with plot entries. --- .../tests/unit/executor/test_reingest.py | 216 +++++++++++++++++- 1 file changed, 210 insertions(+), 6 deletions(-) diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index bdd3054f9..361244303 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -1,5 +1,6 @@ """Tests for the reingest module.""" +import json import pathlib import pytest @@ -8,8 +9,12 @@ from climate_ref.executor.reingest import ( ReingestMode, + _copy_results_to_scratch, _delete_execution_results, _extract_dataset_attributes, + _get_existing_metric_dimensions, + _handle_reingest_output_bundle, + _ingest_metrics, get_executions_for_reingest, reconstruct_execution_definition, reingest_execution, @@ -22,12 +27,15 @@ ExecutionGroup, ExecutionOutput, ResultOutputType, + execution_datasets, ) from climate_ref.models.metric_value import MetricValue from climate_ref.models.provider import Provider as ProviderModel from climate_ref.provider_registry import ProviderRegistry, _register_provider +from climate_ref_core.datasets import SourceDatasetType from climate_ref_core.diagnostics import ExecutionResult from climate_ref_core.metric_values import SeriesMetricValue as TSeries +from climate_ref_core.pycmec.controlled_vocabulary import CV from climate_ref_core.pycmec.metric import CMECMetric from climate_ref_core.pycmec.output import CMECOutput @@ -83,15 +91,12 @@ def mock_provider_registry(provider): @pytest.fixture def output_dir_with_results(config, reingest_execution_obj): - """Create a results directory with CMEC output files.""" + """Create a results directory with empty CMEC template files.""" results_dir = config.paths.results / reingest_execution_obj.output_fragment results_dir.mkdir(parents=True, exist_ok=True) - cmec_metric = CMECMetric(**CMECMetric.create_template()) - cmec_metric.dump_to_json(results_dir / "diagnostic.json") - - cmec_output = CMECOutput(**CMECOutput.create_template()) - cmec_output.dump_to_json(results_dir / "output.json") + CMECMetric(**CMECMetric.create_template()).dump_to_json(results_dir / "diagnostic.json") + CMECOutput(**CMECOutput.create_template()).dump_to_json(results_dir / "output.json") series_data = [ TSeries( @@ -107,6 +112,57 @@ def output_dir_with_results(config, reingest_execution_obj): return results_dir +@pytest.fixture +def output_dir_with_data(config, reingest_execution_obj): + """Create a results directory with CMEC files containing actual metric/output data.""" + results_dir = config.paths.results / reingest_execution_obj.output_fragment + results_dir.mkdir(parents=True, exist_ok=True) + + # 2-level nesting required by MetricResults schema + (results_dir / "diagnostic.json").write_text( + json.dumps( + { + "DIMENSIONS": { + "json_structure": ["source_id", "metric"], + "source_id": {"test-model": {}}, + "metric": {"rmse": {}}, + }, + "RESULTS": {"test-model": {"rmse": 42.0}}, + } + ) + ) + + (results_dir / "plot.png").write_bytes(b"fake png") + (results_dir / "output.json").write_text( + json.dumps( + { + "index": "index.html", + "provenance": {"environment": {}, "modeldata": [], "obsdata": {}, "log": "cmec_output.log"}, + "data": {}, + "plots": {"test_plot": {"filename": "plot.png", "long_name": "Test Plot", "description": ""}}, + "html": {}, + "metrics": None, + "diagnostics": {}, + } + ) + ) + + TSeries.dump_to_json( + results_dir / "series.json", + [ + TSeries( + dimensions={"source_id": "test-model"}, + values=[1.0, 2.0, 3.0], + index=[0, 1, 2], + index_name="time", + attributes={"units": "K"}, + ) + ], + ) + + return results_dir + + class TestReingestMode: def test_enum_values(self): assert ReingestMode.additive.value == "additive" @@ -715,6 +771,154 @@ def test_scratch_directory_cleaned_up_on_failure( assert not scratch_dir.exists(), f"Scratch directory was not cleaned up on failure: {scratch_dir}" +class TestCopyResultsToScratch: + def test_path_traversal_raises(self, config): + """Should reject output fragments that escape the base directory.""" + with pytest.raises(ValueError, match="Unsafe source path"): + _copy_results_to_scratch(config, "/etc/passwd") + + def test_copies_directory(self, config, reingest_execution_obj, output_dir_with_results): + """Should copy the results directory to scratch.""" + scratch = _copy_results_to_scratch(config, reingest_execution_obj.output_fragment) + assert scratch.exists() + assert (scratch / "diagnostic.json").exists() + assert (scratch / "output.json").exists() + + +class TestGetExistingMetricDimensions: + def test_returns_empty_for_no_values(self, reingest_db, reingest_execution_obj): + """Should return empty set when no metric values exist.""" + result = _get_existing_metric_dimensions(reingest_db, reingest_execution_obj) + assert result == set() + + def test_returns_dimension_signatures(self, reingest_db, reingest_execution_obj): + """Should return sorted dimension tuples for existing metric values.""" + reingest_db.session.add( + ScalarMetricValue( + execution_id=reingest_execution_obj.id, + value=1.0, + attributes={}, + source_id="model-a", + ) + ) + reingest_db.session.commit() + + result = _get_existing_metric_dimensions(reingest_db, reingest_execution_obj) + assert len(result) == 1 + sig = next(iter(result)) + # Should contain source_id dimension + assert any(k == "source_id" and v == "model-a" for k, v in sig) + + +class TestHandleReingestOutputBundle: + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_registers_outputs_in_db(self, config, reingest_db, reingest_execution_obj, output_dir_with_data): + """Should register output entries from the bundle into the database.""" + bundle_path = output_dir_with_data / "output.json" + + _handle_reingest_output_bundle(config, reingest_db, reingest_execution_obj, bundle_path) + reingest_db.session.commit() + + outputs = ( + reingest_db.session.query(ExecutionOutput).filter_by(execution_id=reingest_execution_obj.id).all() + ) + assert len(outputs) >= 1 + assert any(o.short_name == "test_plot" for o in outputs) + + +class TestIngestMetrics: + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_ingest_scalar_values( + self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mocker + ): + """Should ingest scalar metric values from a real CMEC bundle.""" + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.series_filename = pathlib.Path("series.json") + mock_result.to_output_path = lambda f: output_dir_with_data / f + + cv = CV.load_from_file(config.paths.dimensions_cv) + + _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) + reingest_db.session.commit() + + scalars = ( + reingest_db.session.query(ScalarMetricValue) + .filter_by(execution_id=reingest_execution_obj.id) + .all() + ) + assert len(scalars) >= 1 + assert scalars[0].value == 42.0 + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_ingest_additive_skips_existing( + self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mocker + ): + """Additive mode should skip values with existing dimension signatures.""" + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.series_filename = pathlib.Path("series.json") + mock_result.to_output_path = lambda f: output_dir_with_data / f + + cv = CV.load_from_file(config.paths.dimensions_cv) + + # First ingest + _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) + reingest_db.session.commit() + count_first = ( + reingest_db.session.query(MetricValue).filter_by(execution_id=reingest_execution_obj.id).count() + ) + + # Second ingest in additive mode should not duplicate + _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=True) + reingest_db.session.commit() + count_second = ( + reingest_db.session.query(MetricValue).filter_by(execution_id=reingest_execution_obj.id).count() + ) + + assert count_first == count_second + + +class TestReconstructWithDatasets: + def test_reconstruct_with_linked_datasets(self, config, db_seeded, provider): + """reconstruct_execution_definition should build dataset collections from linked datasets.""" + with db_seeded.session.begin(): + diag = db_seeded.session.query(DiagnosticModel).first() + eg = ExecutionGroup( + key="recon-test", + diagnostic_id=diag.id, + selectors={"cmip6": [["source_id", "ACCESS-ESM1-5"]]}, + ) + db_seeded.session.add(eg) + db_seeded.session.flush() + + ex = Execution( + execution_group_id=eg.id, + successful=True, + output_fragment="test/recon/abc", + dataset_hash="h1", + ) + db_seeded.session.add(ex) + db_seeded.session.flush() + + # Link a dataset to the execution + dataset = db_seeded.session.query(CMIP6Dataset).first() + if dataset: + db_seeded.session.execute( + execution_datasets.insert().values( + execution_id=ex.id, + dataset_id=dataset.id, + ) + ) + + diagnostic = provider.get("mock") + definition = reconstruct_execution_definition(config, ex, diagnostic) + + assert definition.key == "recon-test" + if dataset: + assert SourceDatasetType.CMIP6 in definition.datasets + + class TestGetExecutionsForReingest: def test_filters_by_success(self, db_seeded): """By default should only return successful executions.""" From 303a3fb5017ce14a7e46191a6e07ebd88a3bbcdc Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Thu, 2 Apr 2026 16:07:52 +0900 Subject: [PATCH 09/19] refactor(test): deduplicate reingest test fixtures and helpers Extract mock_result_factory fixture and _patch_build_result helper to eliminate 11 repetitions of 8-line mock ExecutionResult boilerplate. Consolidate shared TSeries data into SAMPLE_SERIES constant and results dir setup into _create_results_dir helper. Add autouse fixture for provider registration in TestGetExecutionsForReingest. --- .../tests/unit/executor/test_reingest.py | 247 ++++++++---------- 1 file changed, 103 insertions(+), 144 deletions(-) diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index 361244303..998bebf31 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -89,25 +89,32 @@ def mock_provider_registry(provider): return ProviderRegistry(providers=[provider]) +SAMPLE_SERIES = [ + TSeries( + dimensions={"source_id": "test-model"}, + values=[1.0, 2.0, 3.0], + index=[0, 1, 2], + index_name="time", + attributes={"units": "K"}, + ) +] + + +def _create_results_dir(config, execution): + """Create and return the results directory for an execution.""" + results_dir = config.paths.results / execution.output_fragment + results_dir.mkdir(parents=True, exist_ok=True) + return results_dir + + @pytest.fixture def output_dir_with_results(config, reingest_execution_obj): """Create a results directory with empty CMEC template files.""" - results_dir = config.paths.results / reingest_execution_obj.output_fragment - results_dir.mkdir(parents=True, exist_ok=True) + results_dir = _create_results_dir(config, reingest_execution_obj) CMECMetric(**CMECMetric.create_template()).dump_to_json(results_dir / "diagnostic.json") CMECOutput(**CMECOutput.create_template()).dump_to_json(results_dir / "output.json") - - series_data = [ - TSeries( - dimensions={"source_id": "test-model"}, - values=[1.0, 2.0, 3.0], - index=[0, 1, 2], - index_name="time", - attributes={"units": "K"}, - ) - ] - TSeries.dump_to_json(results_dir / "series.json", series_data) + TSeries.dump_to_json(results_dir / "series.json", SAMPLE_SERIES) return results_dir @@ -115,10 +122,8 @@ def output_dir_with_results(config, reingest_execution_obj): @pytest.fixture def output_dir_with_data(config, reingest_execution_obj): """Create a results directory with CMEC files containing actual metric/output data.""" - results_dir = config.paths.results / reingest_execution_obj.output_fragment - results_dir.mkdir(parents=True, exist_ok=True) + results_dir = _create_results_dir(config, reingest_execution_obj) - # 2-level nesting required by MetricResults schema (results_dir / "diagnostic.json").write_text( json.dumps( { @@ -147,22 +152,45 @@ def output_dir_with_data(config, reingest_execution_obj): ) ) - TSeries.dump_to_json( - results_dir / "series.json", - [ - TSeries( - dimensions={"source_id": "test-model"}, - values=[1.0, 2.0, 3.0], - index=[0, 1, 2], - index_name="time", - attributes={"units": "K"}, - ) - ], - ) + TSeries.dump_to_json(results_dir / "series.json", SAMPLE_SERIES) return results_dir +@pytest.fixture +def mock_result_factory(mocker): + """Factory to create mock ExecutionResult objects with sensible defaults. + + Accepts an output_dir and optional overrides for output_bundle_filename + and series_filename. + """ + + def _create( + output_dir, + *, + output_bundle_filename=pathlib.Path("output.json"), + series_filename=pathlib.Path("series.json"), + ): + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") + mock_result.output_bundle_filename = output_bundle_filename + mock_result.series_filename = series_filename + mock_result.retryable = False + mock_result.to_output_path = lambda f: output_dir / f if f else output_dir + mock_result.as_relative_path = pathlib.Path + return mock_result + + return _create + + +def _patch_build_result(mocker, registry, mock_result): + """Patch build_execution_result on the mock diagnostic to return mock_result.""" + diagnostic = registry.get_metric("mock_provider", "mock") + mocker.patch.object(diagnostic, "build_execution_result", return_value=mock_result) + return diagnostic + + class TestReingestMode: def test_enum_values(self): assert ReingestMode.additive.value == "additive" @@ -214,7 +242,6 @@ def test_deletes_metric_values_and_outputs(self, reingest_db, reingest_execution """Deleting results should remove all metric values and outputs.""" execution = reingest_execution_obj - # Add some metric values and outputs reingest_db.session.add( ScalarMetricValue( execution_id=execution.id, @@ -276,12 +303,12 @@ def test_reingest_replace_mode( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """Replace mode should delete existing values and re-ingest.""" execution = reingest_execution_obj - # Pre-populate some metric values reingest_db.session.add( ScalarMetricValue( execution_id=execution.id, @@ -294,17 +321,8 @@ def test_reingest_replace_mode( old_count = reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() assert old_count == 1 - # Mock build_execution_result to return a controlled result - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = pathlib.Path("output.json") - mock_result.series_filename = pathlib.Path("series.json") - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory(output_dir_with_results) + _patch_build_result(mocker, mock_provider_registry, mock_result) ok = reingest_execution( config=config, @@ -316,7 +334,6 @@ def test_reingest_replace_mode( reingest_db.session.commit() assert ok is True - # Old value (99.0) should be gone old_vals = ( reingest_db.session.query(ScalarMetricValue) .filter_by(execution_id=execution.id) @@ -333,22 +350,15 @@ def test_reingest_versioned_creates_new_execution( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """Versioned mode should create a new Execution record.""" original_id = reingest_execution_obj.id original_count = reingest_db.session.query(Execution).count() - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = pathlib.Path("output.json") - mock_result.series_filename = pathlib.Path("series.json") - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory(output_dir_with_results) + _patch_build_result(mocker, mock_provider_registry, mock_result) ok = reingest_execution( config=config, @@ -401,6 +411,7 @@ def test_reingest_does_not_touch_dirty_flag( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """The dirty flag should remain unchanged after reingest. @@ -413,16 +424,10 @@ def test_reingest_does_not_touch_dirty_flag( eg.dirty = True reingest_db.session.commit() - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = None - mock_result.series_filename = None - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory( + output_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) reingest_execution( config=config, @@ -444,12 +449,12 @@ def test_additive_preserves_existing_values( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """Additive mode should keep pre-existing metric values untouched.""" execution = reingest_execution_obj - # Pre-populate with a value that has a unique dimension signature reingest_db.session.add( ScalarMetricValue( execution_id=execution.id, @@ -460,16 +465,10 @@ def test_additive_preserves_existing_values( ) reingest_db.session.commit() - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = None - mock_result.series_filename = None - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory( + output_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) reingest_execution( config=config, @@ -497,21 +496,14 @@ def test_additive_reingest_is_idempotent( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """Running additive reingest twice should not create duplicate rows.""" execution = reingest_execution_obj - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = pathlib.Path("output.json") - mock_result.series_filename = pathlib.Path("series.json") - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory(output_dir_with_results) + _patch_build_result(mocker, mock_provider_registry, mock_result) # First reingest reingest_execution( @@ -562,12 +554,12 @@ def test_replace_preserves_data_on_ingestion_failure( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """If ingestion fails in replace mode, original data should be preserved.""" execution = reingest_execution_obj - # Pre-populate with known values reingest_db.session.add( ScalarMetricValue( execution_id=execution.id, @@ -582,17 +574,10 @@ def test_replace_preserves_data_on_ingestion_failure( ) assert original_count == 1 - # Mock build_execution_result to return a result that will fail during scalar ingestion - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = None - mock_result.series_filename = None - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory( + output_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) # Make the scalar ingestion fail by corrupting the metric bundle (output_dir_with_results / "diagnostic.json").write_text("not valid json") @@ -624,19 +609,12 @@ def test_versioned_reingest_twice_creates_unique_fragments( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """Running versioned reingest twice should create distinct output fragments.""" - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = pathlib.Path("output.json") - mock_result.series_filename = pathlib.Path("series.json") - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory(output_dir_with_results) + _patch_build_result(mocker, mock_provider_registry, mock_result) # First versioned reingest ok1 = reingest_execution( @@ -676,6 +654,7 @@ def test_reingest_marks_failed_execution_as_successful( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """Reingest should mark a previously-failed execution as successful.""" @@ -683,16 +662,10 @@ def test_reingest_marks_failed_execution_as_successful( reingest_db.session.commit() assert reingest_execution_obj.successful is False - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = None - mock_result.series_filename = None - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory( + output_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) ok = reingest_execution( config=config, @@ -714,19 +687,14 @@ def test_scratch_directory_cleaned_up_on_success( reingest_execution_obj, mock_provider_registry, output_dir_with_results, + mock_result_factory, mocker, ): """Scratch directory should be removed after successful reingest.""" - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.successful = True - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.output_bundle_filename = None - mock_result.series_filename = None - mock_result.retryable = False - mock_result.to_output_path = lambda f: output_dir_with_results / f if f else output_dir_with_results - mock_result.as_relative_path = pathlib.Path - mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + mock_result = mock_result_factory( + output_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) scratch_dir = config.paths.scratch / reingest_execution_obj.output_fragment @@ -829,14 +797,10 @@ def test_registers_outputs_in_db(self, config, reingest_db, reingest_execution_o class TestIngestMetrics: @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") def test_ingest_scalar_values( - self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mocker + self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mock_result_factory ): """Should ingest scalar metric values from a real CMEC bundle.""" - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.series_filename = pathlib.Path("series.json") - mock_result.to_output_path = lambda f: output_dir_with_data / f - + mock_result = mock_result_factory(output_dir_with_data) cv = CV.load_from_file(config.paths.dimensions_cv) _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) @@ -852,14 +816,10 @@ def test_ingest_scalar_values( @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") def test_ingest_additive_skips_existing( - self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mocker + self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mock_result_factory ): """Additive mode should skip values with existing dimension signatures.""" - mock_result = mocker.Mock(spec=ExecutionResult) - mock_result.metric_bundle_filename = pathlib.Path("diagnostic.json") - mock_result.series_filename = pathlib.Path("series.json") - mock_result.to_output_path = lambda f: output_dir_with_data / f - + mock_result = mock_result_factory(output_dir_with_data) cv = CV.load_from_file(config.paths.dimensions_cv) # First ingest @@ -920,12 +880,16 @@ def test_reconstruct_with_linked_datasets(self, config, db_seeded, provider): class TestGetExecutionsForReingest: - def test_filters_by_success(self, db_seeded): - """By default should only return successful executions.""" + @pytest.fixture(autouse=True) + def _register_providers(self, db_seeded): + """Register providers once for all tests in this class.""" with db_seeded.session.begin(): _register_provider(db_seeded, pmp_provider) _register_provider(db_seeded, esmvaltool_provider) + def test_filters_by_success(self, db_seeded): + """By default should only return successful executions.""" + with db_seeded.session.begin(): diag = db_seeded.session.query(DiagnosticModel).first() eg = ExecutionGroup(key="test-filter", diagnostic_id=diag.id, selectors={}) db_seeded.session.add(eg) @@ -947,9 +911,6 @@ def test_filters_by_success(self, db_seeded): def test_include_failed(self, db_seeded): """With include_failed=True, should also return failed executions.""" with db_seeded.session.begin(): - _register_provider(db_seeded, pmp_provider) - _register_provider(db_seeded, esmvaltool_provider) - diag = db_seeded.session.query(DiagnosticModel).first() eg = ExecutionGroup(key="test-failed", diagnostic_id=diag.id, selectors={}) db_seeded.session.add(eg) @@ -970,8 +931,6 @@ def test_include_failed(self, db_seeded): def test_filters_by_group_ids(self, db_seeded): """Should only return executions for specified group IDs.""" with db_seeded.session.begin(): - _register_provider(db_seeded, pmp_provider) - diag = db_seeded.session.query(DiagnosticModel).first() eg1 = ExecutionGroup(key="group-a", diagnostic_id=diag.id, selectors={}) eg2 = ExecutionGroup(key="group-b", diagnostic_id=diag.id, selectors={}) From 3403780dc2de6c324018a4c246c4d9ab5370f1bd Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Fri, 3 Apr 2026 15:48:10 +0900 Subject: [PATCH 10/19] test(reingest): add CLI and unit test coverage for reingest command Add 22 new tests covering the reingest CLI command and additional reingest module paths. Fix transaction handling bug in the CLI reingest command where db.session.begin() failed when a transaction was already active. Make --mode a required option so the user must explicitly choose the reingest strategy (additive, replace, or versioned). CLI tests (10): no-filter guard, no-results, dry-run, cancellation, force mode, group IDs, mode parametrized, include-failed, success path, missing mode error. Unit tests (13): unsuccessful result, scratch fallback, copy helpers, series ingestion, provider/diagnostic filters, empty datasets. --- .../src/climate_ref/cli/executions.py | 13 +- .../tests/unit/cli/test_executions.py | 133 ++++++++ .../tests/unit/executor/test_reingest.py | 309 +++++++++++++++++- 3 files changed, 447 insertions(+), 8 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/cli/executions.py b/packages/climate-ref/src/climate_ref/cli/executions.py index ae2a48f5f..d26a2286d 100644 --- a/packages/climate-ref/src/climate_ref/cli/executions.py +++ b/packages/climate-ref/src/climate_ref/cli/executions.py @@ -745,10 +745,6 @@ def stats( @app.command() def reingest( # noqa: PLR0913 ctx: typer.Context, - group_ids: Annotated[ - list[int] | None, - typer.Argument(help="Execution group IDs to reingest. If omitted, uses filters."), - ] = None, mode: Annotated[ ReingestMode, typer.Option( @@ -756,7 +752,11 @@ def reingest( # noqa: PLR0913 "'replace' (delete existing, re-ingest), " "'versioned' (create new execution record)." ), - ] = ReingestMode.additive, + ], + group_ids: Annotated[ + list[int] | None, + typer.Argument(help="Execution group IDs to reingest. If omitted, uses filters."), + ] = None, provider: Annotated[ list[str] | None, typer.Option( @@ -868,8 +868,9 @@ def reingest( # noqa: PLR0913 # Process each execution in a separate transaction success_count = 0 skip_count = 0 + session = db.session for eg, ex in results: - with db.session.begin(): + with session.begin_nested() if session.in_transaction() else session.begin(): ok = reingest_execution( config=config, database=db, diff --git a/packages/climate-ref/tests/unit/cli/test_executions.py b/packages/climate-ref/tests/unit/cli/test_executions.py index 177be0344..d0860c0c5 100644 --- a/packages/climate-ref/tests/unit/cli/test_executions.py +++ b/packages/climate-ref/tests/unit/cli/test_executions.py @@ -1038,3 +1038,136 @@ def test_fail_running_with_confirmation(self, db_with_running, invoke_cli): assert result.exit_code == 0 assert "Successfully marked 2 execution(s) as failed" in result.stdout + + +class TestReingestCLI: + """Tests for the `executions reingest` CLI command.""" + + @pytest.fixture + def db_with_executions(self, db_seeded, config): + """Create a DB with execution groups that have output directories on disk.""" + with db_seeded.session.begin(): + _register_provider(db_seeded, pmp_provider) + _register_provider(db_seeded, esmvaltool_provider) + + diag_pmp = db_seeded.session.query(Diagnostic).filter_by(slug="enso_tel").first() + diag_esm = db_seeded.session.query(Diagnostic).filter_by(slug="enso-characteristics").first() + + eg1 = ExecutionGroup( + key="reingest-1", + diagnostic_id=diag_pmp.id, + selectors={"cmip6": [["source_id", "GFDL-ESM4"]]}, + ) + eg2 = ExecutionGroup( + key="reingest-2", + diagnostic_id=diag_esm.id, + selectors={"cmip6": [["source_id", "ACCESS-ESM1-5"]]}, + ) + db_seeded.session.add_all([eg1, eg2]) + db_seeded.session.flush() + + ex1 = Execution( + execution_group_id=eg1.id, + successful=True, + output_fragment="pmp/enso_tel/out1", + dataset_hash="h1", + ) + ex2 = Execution( + execution_group_id=eg2.id, + successful=False, + output_fragment="esmvaltool/enso-char/out2", + dataset_hash="h2", + ) + db_seeded.session.add_all([ex1, ex2]) + + db_seeded.session.commit() + return db_seeded + + def test_reingest_no_filters_error(self, invoke_cli, db_seeded): + """Calling reingest with no filters should exit with code 1.""" + result = invoke_cli(["executions", "reingest", "--mode", "additive"], expected_exit_code=1) + assert "At least one filter is required" in result.stderr + + def test_reingest_no_results(self, db_with_executions, invoke_cli): + """When filters match nothing, should print a message and exit cleanly.""" + result = invoke_cli( + ["executions", "reingest", "--mode", "additive", "--provider", "nonexistent", "--force"] + ) + assert result.exit_code == 0 + assert "No executions found" in result.stdout + + def test_reingest_dry_run(self, db_with_executions, invoke_cli): + """Dry run should show preview without making changes.""" + result = invoke_cli( + ["executions", "reingest", "--mode", "additive", "--provider", "pmp", "--dry-run", "--force"] + ) + assert result.exit_code == 0 + assert "Dry run" in result.stdout + assert "enso_tel" in result.stdout + + def test_reingest_cancellation(self, db_with_executions, invoke_cli): + """User declining confirmation should cancel reingest.""" + with patch("climate_ref.cli.executions.typer.confirm", return_value=False): + result = invoke_cli(["executions", "reingest", "--mode", "additive", "--provider", "pmp"]) + assert result.exit_code == 0 + assert "Reingest cancelled" in result.stdout + + def test_reingest_force_runs(self, db_with_executions, invoke_cli, config): + """Force mode should skip confirmation. Even if reingest_execution returns False + (no output dirs), we exercise the CLI loop and get skip counts.""" + result = invoke_cli(["executions", "reingest", "--mode", "additive", "--provider", "pmp", "--force"]) + assert result.exit_code == 0 + assert "Reingest complete" in result.stdout + # Output dir doesn't exist so reingest_execution returns False -> skipped + assert "skipped" in result.stdout + + def test_reingest_by_group_ids(self, db_with_executions, invoke_cli): + """Passing group IDs directly should work.""" + eg = db_with_executions.session.query(ExecutionGroup).filter_by(key="reingest-1").first() + result = invoke_cli(["executions", "reingest", "--mode", "additive", str(eg.id), "--dry-run"]) + assert result.exit_code == 0 + assert "Dry run" in result.stdout + assert "reingest-1" in result.stdout + + @pytest.mark.parametrize("mode", ["additive", "replace", "versioned"]) + def test_reingest_mode_option(self, db_with_executions, invoke_cli, mode): + """Each valid mode should be accepted and displayed.""" + result = invoke_cli(["executions", "reingest", "--mode", mode, "--provider", "pmp", "--dry-run"]) + assert result.exit_code == 0 + assert mode in result.stdout + + def test_reingest_include_failed(self, db_with_executions, invoke_cli): + """--include-failed should include failed executions in preview.""" + result = invoke_cli( + [ + "executions", + "reingest", + "--mode", + "additive", + "--provider", + "esmvaltool", + "--include-failed", + "--dry-run", + ] + ) + assert result.exit_code == 0 + assert "enso-characteristics" in result.stdout + + def test_reingest_success_path(self, db_with_executions, invoke_cli, config): + """When reingest_execution succeeds, success_count should increment.""" + # Create output directory so reingest_execution can find it + eg = db_with_executions.session.query(ExecutionGroup).filter_by(key="reingest-1").first() + ex = eg.executions[0] + results_dir = config.paths.results / ex.output_fragment + results_dir.mkdir(parents=True, exist_ok=True) + + # Mock reingest_execution to return True (success) + with patch("climate_ref.executor.reingest.reingest_execution", return_value=True): + result = invoke_cli(["executions", "reingest", "--mode", "additive", str(eg.id), "--force"]) + assert result.exit_code == 0 + assert "1 succeeded" in result.stdout + assert "0 skipped" in result.stdout + + def test_reingest_missing_mode_fails(self, invoke_cli, db_seeded): + result = invoke_cli(["executions", "reingest", "--provider", "pmp"], expected_exit_code=2) + assert "Missing option '--mode'" in result.stderr diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index 998bebf31..14c1e8ff8 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -10,16 +10,18 @@ from climate_ref.executor.reingest import ( ReingestMode, _copy_results_to_scratch, + _copy_scratch_to_results, _delete_execution_results, _extract_dataset_attributes, _get_existing_metric_dimensions, _handle_reingest_output_bundle, + _handle_reingest_outputs, _ingest_metrics, get_executions_for_reingest, reconstruct_execution_definition, reingest_execution, ) -from climate_ref.models import ScalarMetricValue +from climate_ref.models import ScalarMetricValue, SeriesMetricValue from climate_ref.models.dataset import CMIP6Dataset from climate_ref.models.diagnostic import Diagnostic as DiagnosticModel from climate_ref.models.execution import ( @@ -37,7 +39,7 @@ from climate_ref_core.metric_values import SeriesMetricValue as TSeries from climate_ref_core.pycmec.controlled_vocabulary import CV from climate_ref_core.pycmec.metric import CMECMetric -from climate_ref_core.pycmec.output import CMECOutput +from climate_ref_core.pycmec.output import CMECOutput, OutputDict @pytest.fixture @@ -839,6 +841,93 @@ def test_ingest_additive_skips_existing( assert count_first == count_second +class TestIngestMetricsWithSeries: + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_ingest_series_values( + self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mock_result_factory + ): + """Should ingest series metric values from a real series file.""" + mock_result = mock_result_factory(output_dir_with_data) + cv = CV.load_from_file(config.paths.dimensions_cv) + + _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) + reingest_db.session.commit() + + series = ( + reingest_db.session.query(SeriesMetricValue) + .filter_by(execution_id=reingest_execution_obj.id) + .all() + ) + assert len(series) >= 1 + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_ingest_series_additive_skips_existing( + self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mock_result_factory + ): + """Additive mode should skip series with existing dimension signatures.""" + mock_result = mock_result_factory(output_dir_with_data) + cv = CV.load_from_file(config.paths.dimensions_cv) + + # First ingest + _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) + reingest_db.session.commit() + + count_first = ( + reingest_db.session.query(SeriesMetricValue) + .filter_by(execution_id=reingest_execution_obj.id) + .count() + ) + + # Second ingest in additive mode + _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=True) + reingest_db.session.commit() + + count_second = ( + reingest_db.session.query(SeriesMetricValue) + .filter_by(execution_id=reingest_execution_obj.id) + .count() + ) + + assert count_first == count_second + + +class TestReconstructEmptyDataset: + def test_reconstruct_dataset_with_no_files(self, config, db_seeded, provider): + """Datasets with no files should produce an empty DataFrame.""" + with db_seeded.session.begin(): + diag = db_seeded.session.query(DiagnosticModel).first() + eg = ExecutionGroup( + key="empty-files", + diagnostic_id=diag.id, + selectors={"cmip6": [["source_id", "NONEXISTENT"]]}, + ) + db_seeded.session.add(eg) + db_seeded.session.flush() + + # Link a dataset but it has no files + dataset = db_seeded.session.query(CMIP6Dataset).first() + ex = Execution( + execution_group_id=eg.id, + successful=True, + output_fragment="test/empty-files/abc", + dataset_hash="h1", + ) + db_seeded.session.add(ex) + db_seeded.session.flush() + + if dataset: + db_seeded.session.execute( + execution_datasets.insert().values( + execution_id=ex.id, + dataset_id=dataset.id, + ) + ) + + diagnostic = provider.get("mock") + definition = reconstruct_execution_definition(config, ex, diagnostic) + assert definition.key == "empty-files" + + class TestReconstructWithDatasets: def test_reconstruct_with_linked_datasets(self, config, db_seeded, provider): """reconstruct_execution_definition should build dataset collections from linked datasets.""" @@ -879,6 +968,146 @@ def test_reconstruct_with_linked_datasets(self, config, db_seeded, provider): assert SourceDatasetType.CMIP6 in definition.datasets +class TestReingestUnsuccessfulResult: + def test_reingest_unsuccessful_build_result( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Should return False when build_execution_result returns unsuccessful.""" + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = False + mock_result.metric_bundle_filename = None + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + assert ok is False + + def test_reingest_successful_but_no_metric_bundle( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + output_dir_with_results, + mocker, + ): + """Should return False when result is successful but metric_bundle_filename is None.""" + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mock_result = mocker.Mock(spec=ExecutionResult) + mock_result.successful = True + mock_result.metric_bundle_filename = None + mocker.patch.object(mock_diagnostic, "build_execution_result", return_value=mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + mode=ReingestMode.replace, + ) + assert ok is False + + +class TestHandleReingestOutputsScratchFallback: + def test_scratch_path_fallback(self, config, reingest_db, reingest_execution_obj): + """When output filename is absolute under scratch, should fall back to scratch base.""" + execution = reingest_execution_obj + + # Create a scratch directory with a plot file + scratch_base = config.paths.scratch / execution.output_fragment + scratch_base.mkdir(parents=True, exist_ok=True) + (scratch_base / "plot.png").write_bytes(b"fake png") + + # Build an OutputDict with an absolute path under scratch (not under results) + outputs = { + "test_plot": OutputDict( + filename=str(scratch_base / "plot.png"), + long_name="Test Plot", + description="A test plot", + ), + } + + _handle_reingest_outputs( + outputs=outputs, + output_type=ResultOutputType.Plot, + config=config, + database=reingest_db, + execution=execution, + ) + reingest_db.session.commit() + + db_outputs = reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).all() + assert len(db_outputs) == 1 + assert db_outputs[0].short_name == "test_plot" + + +class TestCopyResultsToScratchExistingDst: + def test_existing_scratch_dir_is_replaced(self, config, reingest_execution_obj, output_dir_with_results): + """When scratch destination already exists, it should be removed and replaced.""" + fragment = reingest_execution_obj.output_fragment + dst = config.paths.scratch / fragment + dst.mkdir(parents=True, exist_ok=True) + (dst / "stale_file.txt").write_text("stale") + + result = _copy_results_to_scratch(config, fragment) + assert result.exists() + assert not (result / "stale_file.txt").exists() + assert (result / "diagnostic.json").exists() + + +class TestCopyScratchToResults: + def test_versioned_copies_to_new_dir(self, config, reingest_db, reingest_execution_obj): + """Versioned mode should copy scratch to a new results directory.""" + execution = reingest_execution_obj + scratch_dir = config.paths.scratch / execution.output_fragment + scratch_dir.mkdir(parents=True, exist_ok=True) + (scratch_dir / "metric.json").write_text("{}") + + original_results = config.paths.results / execution.output_fragment + original_results.mkdir(parents=True, exist_ok=True) + + _copy_scratch_to_results(config, scratch_dir, execution, ReingestMode.versioned, original_results) + + target = config.paths.results / execution.output_fragment + assert target.exists() + assert (target / "metric.json").exists() + + def test_non_versioned_copies_in_place(self, config, reingest_db, reingest_execution_obj): + """Non-versioned mode should copy scratch into original results dir.""" + execution = reingest_execution_obj + scratch_dir = config.paths.scratch / execution.output_fragment + scratch_dir.mkdir(parents=True, exist_ok=True) + (scratch_dir / "new_file.json").write_text("{}") + + original_results = config.paths.results / execution.output_fragment + original_results.mkdir(parents=True, exist_ok=True) + (original_results / "existing.txt").write_text("keep") + + _copy_scratch_to_results(config, scratch_dir, execution, ReingestMode.additive, original_results) + + assert (original_results / "existing.txt").exists() + assert (original_results / "new_file.json").exists() + + +class TestCopyResultsToScratchMissing: + def test_source_not_exists(self, config): + """Should raise FileNotFoundError when source directory doesn't exist.""" + with pytest.raises(FileNotFoundError): + _copy_results_to_scratch(config, "nonexistent/path/fragment") + + class TestGetExecutionsForReingest: @pytest.fixture(autouse=True) def _register_providers(self, db_seeded): @@ -957,3 +1186,79 @@ def test_filters_by_group_ids(self, db_seeded): results = get_executions_for_reingest(db_seeded, execution_group_ids=[eg1.id]) assert len(results) == 1 assert results[0][0].id == eg1.id + + def test_filters_by_provider(self, db_seeded): + """Should filter executions by provider slug.""" + with db_seeded.session.begin(): + diag_pmp = db_seeded.session.query(DiagnosticModel).filter_by(slug="enso_tel").first() + diag_esm = db_seeded.session.query(DiagnosticModel).filter_by(slug="enso-characteristics").first() + + eg1 = ExecutionGroup(key="prov-pmp", diagnostic_id=diag_pmp.id, selectors={}) + eg2 = ExecutionGroup(key="prov-esm", diagnostic_id=diag_esm.id, selectors={}) + db_seeded.session.add_all([eg1, eg2]) + db_seeded.session.flush() + + db_seeded.session.add( + Execution( + execution_group_id=eg1.id, + successful=True, + output_fragment="out-pmp", + dataset_hash="hp", + ) + ) + db_seeded.session.add( + Execution( + execution_group_id=eg2.id, + successful=True, + output_fragment="out-esm", + dataset_hash="he", + ) + ) + + results = get_executions_for_reingest(db_seeded, provider_filters=["pmp"]) + provider_slugs = {eg.diagnostic.provider.slug for eg, _ in results} + assert "pmp" in provider_slugs + assert "esmvaltool" not in provider_slugs + + def test_filters_by_diagnostic(self, db_seeded): + """Should filter executions by diagnostic slug.""" + with db_seeded.session.begin(): + diag_pmp = db_seeded.session.query(DiagnosticModel).filter_by(slug="enso_tel").first() + diag_esm = db_seeded.session.query(DiagnosticModel).filter_by(slug="enso-characteristics").first() + + eg1 = ExecutionGroup(key="diag-pmp", diagnostic_id=diag_pmp.id, selectors={}) + eg2 = ExecutionGroup(key="diag-esm", diagnostic_id=diag_esm.id, selectors={}) + db_seeded.session.add_all([eg1, eg2]) + db_seeded.session.flush() + + db_seeded.session.add( + Execution( + execution_group_id=eg1.id, + successful=True, + output_fragment="out-d-pmp", + dataset_hash="hdp", + ) + ) + db_seeded.session.add( + Execution( + execution_group_id=eg2.id, + successful=True, + output_fragment="out-d-esm", + dataset_hash="hde", + ) + ) + + results = get_executions_for_reingest(db_seeded, diagnostic_filters=["enso_tel"]) + diag_slugs = {eg.diagnostic.slug for eg, _ in results} + assert "enso_tel" in diag_slugs + assert "enso-characteristics" not in diag_slugs + + def test_filters_out_none_executions(self, db_seeded): + """Should filter out execution groups that have no executions.""" + with db_seeded.session.begin(): + diag = db_seeded.session.query(DiagnosticModel).first() + eg = ExecutionGroup(key="no-exec", diagnostic_id=diag.id, selectors={}) + db_seeded.session.add(eg) + + results = get_executions_for_reingest(db_seeded, execution_group_ids=[eg.id]) + assert len(results) == 0 From c02beabd7417b00c9c667cccb72c2e2401970393 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Fri, 3 Apr 2026 17:05:15 +0900 Subject: [PATCH 11/19] fix: clean up finalized->finalised --- packages/climate-ref/src/climate_ref/data_catalog.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/data_catalog.py b/packages/climate-ref/src/climate_ref/data_catalog.py index e64425104..00ba2b1ec 100644 --- a/packages/climate-ref/src/climate_ref/data_catalog.py +++ b/packages/climate-ref/src/climate_ref/data_catalog.py @@ -81,13 +81,13 @@ def finalise(self, subset: pd.DataFrame) -> pd.DataFrame: Finalise unfinalised datasets in the given subset. If the adapter supports finalization (implements FinaliseableDatasetAdapterMixin), - unfinalised datasets in the subset are finalized by opening their files. + unfinalised datasets in the subset are finalised by opening their files. The internal cache and database are updated accordingly. Parameters ---------- subset - DataFrame subset to finalize (typically after filter+group_by) + DataFrame subset to finalise (typically after filter+group_by) Returns ------- From d5a4218d3888c861b06ff2d5b1e29782b28a8c9e Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Fri, 3 Apr 2026 17:36:55 +0900 Subject: [PATCH 12/19] refactor(executor): deduplicate reingest and result_handling ingestion logic Extract shared functions (ingest_scalar_values, ingest_series_values, register_execution_outputs) into result_handling.py and have reingest.py import them instead of maintaining parallel implementations. --- .../src/climate_ref/cli/executions.py | 16 +- .../src/climate_ref/executor/reingest.py | 401 ++++++------------ .../climate_ref/executor/result_handling.py | 330 +++++++++----- .../tests/unit/executor/test_reingest.py | 339 ++++++++------- 4 files changed, 533 insertions(+), 553 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/cli/executions.py b/packages/climate-ref/src/climate_ref/cli/executions.py index d26a2286d..6b0f6851b 100644 --- a/packages/climate-ref/src/climate_ref/cli/executions.py +++ b/packages/climate-ref/src/climate_ref/cli/executions.py @@ -865,12 +865,16 @@ def reingest( # noqa: PLR0913 console.print("Reingest cancelled.") return + # Ensure any autobegun transaction from the preview queries is closed + # so each reingest runs in its own top-level transaction (not a savepoint). + if db.session.in_transaction(): + db.session.commit() + # Process each execution in a separate transaction success_count = 0 skip_count = 0 - session = db.session for eg, ex in results: - with session.begin_nested() if session.in_transaction() else session.begin(): + with db.session.begin(): ok = reingest_execution( config=config, database=db, @@ -879,10 +883,10 @@ def reingest( # noqa: PLR0913 mode=mode, ) - if ok: - success_count += 1 - else: - skip_count += 1 + if ok: + success_count += 1 + else: + skip_count += 1 console.print(f"\n[green]Reingest complete:[/] {success_count} succeeded, {skip_count} skipped.") diff --git a/packages/climate-ref/src/climate_ref/executor/reingest.py b/packages/climate-ref/src/climate_ref/executor/reingest.py index 7e683aead..332afe379 100644 --- a/packages/climate-ref/src/climate_ref/executor/reingest.py +++ b/packages/climate-ref/src/climate_ref/executor/reingest.py @@ -25,10 +25,14 @@ import pandas as pd from loguru import logger -from sqlalchemy import delete, insert +from sqlalchemy import delete from climate_ref.datasets import get_slug_column -from climate_ref.models import ScalarMetricValue, SeriesMetricValue +from climate_ref.executor.result_handling import ( + ingest_scalar_values, + ingest_series_values, + register_execution_outputs, +) from climate_ref.models.execution import ( Execution, ExecutionGroup, @@ -43,12 +47,9 @@ ExecutionDatasetCollection, SourceDatasetType, ) -from climate_ref_core.diagnostics import ExecutionDefinition, ExecutionResult, ensure_relative_path -from climate_ref_core.exceptions import ResultValidationError -from climate_ref_core.metric_values import SeriesMetricValue as TSeries +from climate_ref_core.diagnostics import ExecutionDefinition, ExecutionResult from climate_ref_core.pycmec.controlled_vocabulary import CV -from climate_ref_core.pycmec.metric import CMECMetric -from climate_ref_core.pycmec.output import CMECOutput, OutputDict +from climate_ref_core.pycmec.output import CMECOutput if TYPE_CHECKING: from climate_ref.config import Config @@ -171,72 +172,6 @@ def _extract_dataset_attributes(dataset: "Dataset") -> dict[str, object]: return attrs -def _handle_reingest_output_bundle( - config: "Config", - database: "Database", - execution: Execution, - cmec_output_bundle_filename: pathlib.Path, -) -> None: - """ - Process the output bundle for reingest (no file copy, files already in results dir). - """ - cmec_output_bundle = CMECOutput.load_from_json(cmec_output_bundle_filename) - _handle_reingest_outputs( - cmec_output_bundle.plots, - output_type=ResultOutputType.Plot, - config=config, - database=database, - execution=execution, - ) - _handle_reingest_outputs( - cmec_output_bundle.data, - output_type=ResultOutputType.Data, - config=config, - database=database, - execution=execution, - ) - _handle_reingest_outputs( - cmec_output_bundle.html, - output_type=ResultOutputType.HTML, - config=config, - database=database, - execution=execution, - ) - - -def _handle_reingest_outputs( - outputs: dict[str, OutputDict] | None, - output_type: "ResultOutputType", - config: "Config", - database: "Database", - execution: Execution, -) -> None: - """Register outputs in the DB without copying files (they are already in place).""" - outputs = outputs or {} - results_base = config.paths.results / execution.output_fragment - scratch_base = config.paths.scratch / execution.output_fragment - - for key, output_info in outputs.items(): - # Output bundles produced during re-extraction may contain absolute - # paths under the scratch directory. Fall back to scratch_base when - # the filename is not relative to results_base. - try: - filename = ensure_relative_path(output_info.filename, results_base) - except ValueError: - filename = ensure_relative_path(output_info.filename, scratch_base) - database.session.add( - ExecutionOutput.build( - execution_id=execution.id, - output_type=output_type, - filename=str(filename), - description=output_info.description, - short_name=key, - long_name=output_info.long_name, - dimensions=output_info.dimensions or {}, - ) - ) - - def _get_existing_metric_dimensions( database: "Database", execution: Execution ) -> set[tuple[tuple[str, str], ...]]: @@ -253,147 +188,51 @@ def _get_existing_metric_dimensions( return sigs -def _process_reingest_series( - database: "Database", - result: ExecutionResult, - execution: Execution, - cv: CV, - *, - existing: set[tuple[tuple[str, str], ...]] | None = None, -) -> None: - """ - Process series values for reingest. - - When ``existing`` is provided, only inserts series whose dimension - signatures are not already present (additive mode). - """ - assert result.series_filename, "Series filename must be set in the result" - - series_values_path = result.to_output_path(result.series_filename) - series_values = TSeries.load_from_json(series_values_path) - - try: - cv.validate_metrics(series_values) - except (ResultValidationError, AssertionError): - logger.exception("Diagnostic values do not conform with the controlled vocabulary") - - new_values = [] - for series_result in series_values: - if existing is not None: - dims = tuple(sorted(series_result.dimensions.items())) - if dims in existing: - continue - new_values.append( - { - "execution_id": execution.id, - "values": series_result.values, - "attributes": series_result.attributes, - "index": series_result.index, - "index_name": series_result.index_name, - **series_result.dimensions, - } - ) - - skipped = len(series_values) - len(new_values) - if existing is not None: - logger.debug( - f"Additive: {len(new_values)} new series values " - f"(skipped {skipped} existing) for execution {execution.id}" - ) - else: - logger.debug(f"Ingesting {len(new_values)} series values for execution {execution.id}") - - if new_values: - database.session.execute(insert(SeriesMetricValue), new_values) +def _copy_with_backup(src: pathlib.Path, dst: pathlib.Path) -> None: + """Copy *src* to *dst*, backing up *dst* first if it already exists.""" + dst.parent.mkdir(parents=True, exist_ok=True) + if dst.exists(): + backup = dst.with_suffix(dst.suffix + ".bak") + shutil.copy2(dst, backup) + shutil.copyfile(src, dst) -def _process_reingest_scalar( - database: "Database", +def _sync_reingest_files_to_results( + config: "Config", + src_fragment: str, + dst_fragment: str, result: ExecutionResult, - execution: Execution, - cv: CV, - *, - existing: set[tuple[tuple[str, str], ...]] | None = None, + mode: ReingestMode, ) -> None: """ - Process scalar values for reingest. + Copy re-extracted CMEC bundles from scratch to the results directory. - When ``existing`` is provided, only inserts values whose dimension - signatures are not already present (additive mode). + For **versioned** mode the entire scratch tree is copied to a new results + directory (no backup needed). For other modes only the CMEC bundle files + are overwritten, with ``.bak`` copies of the previous versions. """ - cmec_metric_bundle = CMECMetric.load_from_json(result.to_output_path(result.metric_bundle_filename)) + src_dir = config.paths.scratch / src_fragment + dst_dir = config.paths.results / dst_fragment - try: - cv.validate_metrics(cmec_metric_bundle) - except (ResultValidationError, AssertionError): - logger.exception("Diagnostic values do not conform with the controlled vocabulary") - - new_values = [] - total_count = 0 - for metric_result in cmec_metric_bundle.iter_results(): - total_count += 1 - if existing is not None: - dims = tuple(sorted(metric_result.dimensions.items())) - if dims in existing: - continue - new_values.append( - { - "execution_id": execution.id, - "value": metric_result.value, - "attributes": metric_result.attributes, - **metric_result.dimensions, - } - ) - - skipped = total_count - len(new_values) - if existing is not None: - logger.debug( - f"Additive: {len(new_values)} new scalar values " - f"(skipped {skipped} existing) for execution {execution.id}" - ) + if mode == ReingestMode.versioned: + dst_dir.parent.mkdir(parents=True, exist_ok=True) + if dst_dir.exists(): + shutil.rmtree(dst_dir) + shutil.copytree(src_dir, dst_dir) else: - logger.debug(f"Ingesting {len(new_values)} scalar values for execution {execution.id}") - - if new_values: - database.session.execute(insert(ScalarMetricValue), new_values) - - -def _copy_results_to_scratch( - config: "Config", - output_fragment: str, -) -> pathlib.Path: - """ - Copy an execution's results directory to scratch for safe re-extraction. - - ``build_execution_result()`` may write files (CMEC bundles) into its - ``definition.output_directory``. Running it directly against the live - results tree would mutate the original outputs before the DB savepoint - has a chance to roll back on failure. Copying to scratch first keeps - the results tree unchanged until we decide to commit. - - Returns the scratch output directory. - - Note: the scratch path is deterministic (scratch/output_fragment), so - concurrent reingest of the same execution is not supported. The CLI - is single-threaded, so this is not a practical concern. - """ - src = config.paths.results / output_fragment - dst = config.paths.scratch / output_fragment - - if not src.is_relative_to(config.paths.results): - raise ValueError(f"Unsafe source path: {src} is not under {config.paths.results}") - if not dst.is_relative_to(config.paths.scratch): - raise ValueError(f"Unsafe destination path: {dst} is not under {config.paths.scratch}") - - if dst.exists(): - shutil.rmtree(dst) - shutil.copytree(src, dst) - return dst + dst_dir.mkdir(parents=True, exist_ok=True) + assert result.metric_bundle_filename is not None + _copy_with_backup(src_dir / result.metric_bundle_filename, dst_dir / result.metric_bundle_filename) + if result.output_bundle_filename: + _copy_with_backup( + src_dir / result.output_bundle_filename, dst_dir / result.output_bundle_filename + ) + if result.series_filename: + _copy_with_backup(src_dir / result.series_filename, dst_dir / result.series_filename) -def _delete_execution_results(database: "Database", execution: Execution) -> None: - """Delete all metric values and outputs for an execution.""" - database.session.execute(delete(ExecutionOutput).where(ExecutionOutput.execution_id == execution.id)) +def _delete_execution_metric_values(database: "Database", execution: Execution) -> None: + """Delete all metric values for an execution (outputs are handled separately).""" # MetricValue uses single-table inheritance so we delete from the base table database.session.execute(delete(MetricValue).where(MetricValue.execution_id == execution.id)) @@ -416,6 +255,9 @@ def _apply_reingest_mode( # noqa: PLR0913 with database.session.begin_nested(): if mode == ReingestMode.versioned: + # Hash is intentionally deterministic for a given execution: + # re-running versioned reingest on the same execution produces the + # same base fragment, with _n{count} suffixes for repeats. version_hash = hashlib.sha1( # noqa: S324 f"{execution.output_fragment}-reingest-{execution.id}".encode() ).hexdigest()[:12] @@ -442,18 +284,33 @@ def _apply_reingest_mode( # noqa: PLR0913 ) ) elif mode == ReingestMode.replace: - _delete_execution_results(database, target_execution) + _delete_execution_metric_values(database, target_execution) + # Outputs are always refreshed regardless of mode. + # "Additive" only applies to metric values as keeping stale entries + # would be more confusing than helpful. if result.output_bundle_filename: database.session.execute( delete(ExecutionOutput).where(ExecutionOutput.execution_id == target_execution.id) ) - _handle_reingest_output_bundle( - config, - database, - target_execution, - result.to_output_path(result.output_bundle_filename), + cmec_output_bundle = CMECOutput.load_from_json( + result.to_output_path(result.output_bundle_filename) ) + results_base = config.paths.results / target_execution.output_fragment + scratch_base = config.paths.scratch / execution.output_fragment + for attr, output_type in [ + ("plots", ResultOutputType.Plot), + ("data", ResultOutputType.Data), + ("html", ResultOutputType.HTML), + ]: + register_execution_outputs( + database, + target_execution, + getattr(cmec_output_bundle, attr), + output_type=output_type, + base_path=results_base, + fallback_path=scratch_base, + ) _ingest_metrics(database, result, target_execution, cv, additive=mode == ReingestMode.additive) @@ -475,30 +332,9 @@ def _ingest_metrics( existing = _get_existing_metric_dimensions(database, execution) if additive else None if result.series_filename: - _process_reingest_series( - database=database, result=result, execution=execution, cv=cv, existing=existing - ) - - _process_reingest_scalar(database=database, result=result, execution=execution, cv=cv, existing=existing) + ingest_series_values(database=database, result=result, execution=execution, cv=cv, existing=existing) - -def _copy_scratch_to_results( - config: "Config", - scratch_dir: pathlib.Path, - target_execution: Execution, - mode: ReingestMode, - original_results_dir: pathlib.Path, -) -> None: - """Copy re-extracted files from scratch to the results tree after DB success.""" - target_results_dir = config.paths.results / target_execution.output_fragment - if not target_results_dir.is_relative_to(config.paths.results): - raise ValueError(f"Unsafe target path: {target_results_dir} is not under {config.paths.results}") - if mode == ReingestMode.versioned: - if target_results_dir.exists(): - shutil.rmtree(target_results_dir) - shutil.copytree(scratch_dir, target_results_dir) - else: - shutil.copytree(scratch_dir, original_results_dir, dirs_exist_ok=True) + ingest_scalar_values(database=database, result=result, execution=execution, cv=cv, existing=existing) def reingest_execution( @@ -511,7 +347,10 @@ def reingest_execution( """ Reingest an existing execution. - Re-runs ``build_execution_result()`` and processes the results into the database. + Re-runs ``build_execution_result()`` against the scratch directory (which + contains the raw outputs from the original diagnostic run) and re-ingests + the results into the database. Updated CMEC bundles are then copied from + scratch to the results directory, backing up previous versions. Parameters ---------- @@ -545,64 +384,60 @@ def reingest_execution( ) return False - results_dir = config.paths.results / execution.output_fragment - - # Verify output directory exists - if not results_dir.exists(): - logger.error(f"Output directory does not exist: {results_dir}. Skipping execution {execution.id}.") + # The scratch directory contains the raw outputs from the original + # diagnostic run and is never cleaned up. build_execution_result() + # writes CMEC bundles here. + scratch_dir = config.paths.scratch / execution.output_fragment + if not scratch_dir.exists(): + logger.error(f"Scratch directory does not exist: {scratch_dir}. Skipping execution {execution.id}.") return False - # Copy the results directory to scratch so that build_execution_result() - # can write CMEC bundles without mutating the live results tree. - # If anything fails, the original files remain untouched. - scratch_dir = _copy_results_to_scratch(config, execution.output_fragment) + definition = reconstruct_execution_definition(config, execution, diagnostic) try: - definition = reconstruct_execution_definition(config, execution, diagnostic) - - try: - result = diagnostic.build_execution_result(definition) - except Exception: - logger.exception( - f"build_execution_result failed for execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}). Skipping." - ) - return False + result = diagnostic.build_execution_result(definition) + except Exception: + logger.exception( + f"build_execution_result failed for execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}). Skipping." + ) + return False - if not result.successful or result.metric_bundle_filename is None: - logger.warning( - f"build_execution_result returned unsuccessful result for execution {execution.id}. Skipping." - ) - return False - - cv = CV.load_from_file(config.paths.dimensions_cv) - - try: - target_execution = _apply_reingest_mode( - config=config, - database=database, - execution=execution, - result=result, - mode=mode, - cv=cv, - ) - except Exception: - logger.exception( - f"Ingestion failed for execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}). Rolling back changes." - ) - return False + if not result.successful or result.metric_bundle_filename is None: + logger.warning( + f"build_execution_result returned unsuccessful result for execution {execution.id}. Skipping." + ) + return False - _copy_scratch_to_results(config, scratch_dir, target_execution, mode, results_dir) + cv = CV.load_from_file(config.paths.dimensions_cv) - logger.info( - f"Successfully reingested execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}) in {mode.value} mode" + try: + target_execution = _apply_reingest_mode( + config=config, + database=database, + execution=execution, + result=result, + mode=mode, + cv=cv, + ) + except Exception: + logger.exception( + f"Ingestion failed for execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}). Rolling back changes." ) - return True - finally: - if scratch_dir.exists(): - shutil.rmtree(scratch_dir) + return False + + # Copy re-extracted CMEC bundles from scratch to the results tree. + # For non-versioned modes, existing files are backed up with a .bak suffix. + _sync_reingest_files_to_results( + config, execution.output_fragment, target_execution.output_fragment, result, mode + ) + + logger.info( + f"Successfully reingested execution {execution.id} " + f"({provider_slug}/{diagnostic_slug}) in {mode.value} mode" + ) + return True def get_executions_for_reingest( diff --git a/packages/climate-ref/src/climate_ref/executor/result_handling.py b/packages/climate-ref/src/climate_ref/executor/result_handling.py index 49279db2b..7702c76fa 100644 --- a/packages/climate-ref/src/climate_ref/executor/result_handling.py +++ b/packages/climate-ref/src/climate_ref/executor/result_handling.py @@ -66,49 +66,223 @@ def _copy_file_to_results( shutil.copy(input_directory / filename, output_filename) -def _process_execution_scalar( +def ingest_scalar_values( database: Database, result: "ExecutionResult", execution: Execution, cv: CV, + *, + existing: "set[tuple[tuple[str, str], ...]] | None" = None, ) -> None: """ - Process the scalar values from the execution result and store them in the database + Load, validate, and bulk-insert scalar metric values. - This also validates the scalar values against the controlled vocabulary + Parameters + ---------- + database + The active database session to use + result + The execution result containing the metric bundle filename + execution + The execution record to associate values with + cv + The controlled vocabulary to validate against + existing + When provided, dimension signatures already in the DB; values whose + signature is in this set are skipped (additive dedup). When None, + all values are inserted unconditionally. + + Notes + ----- + Callers are responsible for transaction boundaries; this function does not + open a nested transaction or catch exceptions. """ - # Load the metric bundle from the file cmec_metric_bundle = CMECMetric.load_from_json(result.to_output_path(result.metric_bundle_filename)) - # Check that the diagnostic values conform with the controlled vocabulary try: cv.validate_metrics(cmec_metric_bundle) except (ResultValidationError, AssertionError): # TODO: Remove once we have settled on a controlled vocabulary - logger.exception("Diagnostic values do not conform with the controlled vocabulary") - # execution.mark_failed() + logger.warning( + "Diagnostic scalar values do not conform with the controlled vocabulary", exc_info=True + ) + + new_values = [] + total_count = 0 + for metric_result in cmec_metric_bundle.iter_results(): + total_count += 1 + if existing is not None: + dims = tuple(sorted(metric_result.dimensions.items())) + if dims in existing: + continue + new_values.append( + { + "execution_id": execution.id, + "value": metric_result.value, + "attributes": metric_result.attributes, + **metric_result.dimensions, + } + ) + + skipped = total_count - len(new_values) + if existing is not None: + logger.debug( + f"Additive: {len(new_values)} new scalar values " + f"(skipped {skipped} existing) for execution {execution.id}" + ) + else: + logger.debug(f"Ingesting {len(new_values)} scalar values for execution {execution.id}") + + if new_values: + database.session.execute(insert(ScalarMetricValue), new_values) + + +def ingest_series_values( + database: Database, + result: "ExecutionResult", + execution: Execution, + cv: CV, + *, + existing: "set[tuple[tuple[str, str], ...]] | None" = None, +) -> None: + """ + Load, validate, and bulk-insert series metric values. + + Parameters + ---------- + database + The active database session to use + result + The execution result containing the series filename + execution + The execution record to associate values with + cv + The controlled vocabulary to validate against + existing + When provided, dimension signatures already in the DB; series whose + signature is in this set are skipped (additive dedup). When None, + all values are inserted unconditionally. + + Notes + ----- + Callers are responsible for transaction boundaries; this function does not + open a nested transaction or catch exceptions. + """ + assert result.series_filename, "Series filename must be set in the result" + + series_values_path = result.to_output_path(result.series_filename) + series_values = TSeries.load_from_json(series_values_path) - # Perform a bulk insert of scalar values - # The current implementation will swallow the exception, but display a log message try: - scalar_values = [ + cv.validate_metrics(series_values) + except (ResultValidationError, AssertionError): + # TODO: Remove once we have settled on a controlled vocabulary + logger.warning( + "Diagnostic series values do not conform with the controlled vocabulary", exc_info=True + ) + + new_values = [] + for series_result in series_values: + if existing is not None: + dims = tuple(sorted(series_result.dimensions.items())) + if dims in existing: + continue + new_values.append( { "execution_id": execution.id, - "value": result.value, - "attributes": result.attributes, - **result.dimensions, + "values": series_result.values, + "attributes": series_result.attributes, + "index": series_result.index, + "index_name": series_result.index_name, + **series_result.dimensions, } - for result in cmec_metric_bundle.iter_results() - ] - logger.debug(f"Ingesting {len(scalar_values)} scalar values for execution {execution.id}") - if scalar_values: - # Perform this in a nested transaction to rollback if something goes wrong - # We will lose the metric values for a given execution, but not the whole execution - with database.session.begin_nested(): - database.session.execute( - insert(ScalarMetricValue), - scalar_values, - ) + ) + + skipped = len(series_values) - len(new_values) + if existing is not None: + logger.debug( + f"Additive: {len(new_values)} new series values " + f"(skipped {skipped} existing) for execution {execution.id}" + ) + else: + logger.debug(f"Ingesting {len(new_values)} series values for execution {execution.id}") + + if new_values: + database.session.execute(insert(SeriesMetricValue), new_values) + + +def register_execution_outputs( # noqa: PLR0913 + database: Database, + execution: Execution, + outputs: "dict[str, OutputDict] | None", + output_type: ResultOutputType, + *, + base_path: pathlib.Path, + fallback_path: "pathlib.Path | None" = None, +) -> None: + """ + Register output entries in the database. + + Each entry in ``outputs`` is resolved relative to ``base_path``. When a + filename is not relative to ``base_path`` (raises ``ValueError``), it is + resolved relative to ``fallback_path`` instead (if provided). + + Parameters + ---------- + database + The active database session to use + execution + The execution record to associate outputs with + outputs + Mapping of short name to ``OutputDict`` (may be None) + output_type + The type of output being registered + base_path + Primary base directory for resolving relative filenames + fallback_path + Secondary base directory used when ``ensure_relative_path`` raises + ``ValueError`` against ``base_path`` + + Notes + ----- + Callers are responsible for transaction boundaries. + """ + for key, output_info in (outputs or {}).items(): + try: + filename = ensure_relative_path(output_info.filename, base_path) + except ValueError: + if fallback_path is None: + raise + filename = ensure_relative_path(output_info.filename, fallback_path) + database.session.add( + ExecutionOutput.build( + execution_id=execution.id, + output_type=output_type, + filename=str(filename), + description=output_info.description, + short_name=key, + long_name=output_info.long_name, + dimensions=output_info.dimensions or {}, + ) + ) + + +def _process_execution_scalar( + database: Database, + result: "ExecutionResult", + execution: Execution, + cv: CV, +) -> None: + """ + Process the scalar values from the execution result and store them in the database + + This also validates the scalar values against the controlled vocabulary + """ + # Perform a bulk insert of scalar values + # The current implementation will swallow the exception, but display a log message + try: + with database.session.begin_nested(): + ingest_scalar_values(database=database, result=result, execution=execution, cv=cv) # This is a broad exception catch to ensure we log any issues except Exception: logger.exception("Something went wrong when ingesting diagnostic values") @@ -136,39 +310,9 @@ def _process_execution_series( result.series_filename, ) - # Load the series values from the file - series_values_path = result.to_output_path(result.series_filename) - series_values = TSeries.load_from_json(series_values_path) - - try: - cv.validate_metrics(series_values) - except (ResultValidationError, AssertionError): - # TODO: Remove once we have settled on a controlled vocabulary - logger.exception("Diagnostic values do not conform with the controlled vocabulary") - # execution.mark_failed() - - # Perform a bulk insert of series values try: - series_values_content = [ - { - "execution_id": execution.id, - "values": series_result.values, - "attributes": series_result.attributes, - "index": series_result.index, - "index_name": series_result.index_name, - **series_result.dimensions, - } - for series_result in series_values - ] - logger.debug(f"Ingesting {len(series_values)} series values for execution {execution.id}") - if series_values: - # Perform this in a nested transaction to rollback if something goes wrong - # We will lose the metric values for a given execution, but not the whole execution - with database.session.begin_nested(): - database.session.execute( - insert(SeriesMetricValue), - series_values_content, - ) + with database.session.begin_nested(): + ingest_series_values(database=database, result=result, execution=execution, cv=cv) except Exception: logger.exception("Something went wrong when ingesting diagnostic series values") @@ -277,57 +421,27 @@ def _handle_output_bundle( # Copy the content to the output directory # Track in the db cmec_output_bundle = CMECOutput.load_from_json(cmec_output_bundle_filename) - _handle_outputs( - cmec_output_bundle.plots, - output_type=ResultOutputType.Plot, - config=config, - database=database, - execution=execution, - ) - _handle_outputs( - cmec_output_bundle.data, - output_type=ResultOutputType.Data, - config=config, - database=database, - execution=execution, - ) - _handle_outputs( - cmec_output_bundle.html, - output_type=ResultOutputType.HTML, - config=config, - database=database, - execution=execution, - ) - - -def _handle_outputs( - outputs: dict[str, OutputDict] | None, - output_type: ResultOutputType, - config: "Config", - database: Database, - execution: Execution, -) -> None: - outputs = outputs or {} - - for key, output_info in outputs.items(): - filename = ensure_relative_path( - output_info.filename, config.paths.scratch / execution.output_fragment - ) - - _copy_file_to_results( - config.paths.scratch, - config.paths.results, - execution.output_fragment, - filename, - ) - database.session.add( - ExecutionOutput.build( - execution_id=execution.id, - output_type=output_type, - filename=str(filename), - description=output_info.description, - short_name=key, - long_name=output_info.long_name, - dimensions=output_info.dimensions or {}, + scratch_base = config.paths.scratch / execution.output_fragment + results_base = config.paths.results / execution.output_fragment + + for attr, output_type in [ + ("plots", ResultOutputType.Plot), + ("data", ResultOutputType.Data), + ("html", ResultOutputType.HTML), + ]: + outputs = getattr(cmec_output_bundle, attr) or {} + for output_info in outputs.values(): + filename = ensure_relative_path(output_info.filename, scratch_base) + _copy_file_to_results( + config.paths.scratch, + config.paths.results, + execution.output_fragment, + filename, ) + register_execution_outputs( + database, + execution, + outputs, + output_type=output_type, + base_path=results_base, ) diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index 14c1e8ff8..5246d3725 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -9,18 +9,17 @@ from climate_ref.executor.reingest import ( ReingestMode, - _copy_results_to_scratch, - _copy_scratch_to_results, - _delete_execution_results, + _copy_with_backup, + _delete_execution_metric_values, _extract_dataset_attributes, _get_existing_metric_dimensions, - _handle_reingest_output_bundle, - _handle_reingest_outputs, _ingest_metrics, + _sync_reingest_files_to_results, get_executions_for_reingest, reconstruct_execution_definition, reingest_execution, ) +from climate_ref.executor.result_handling import register_execution_outputs from climate_ref.models import ScalarMetricValue, SeriesMetricValue from climate_ref.models.dataset import CMIP6Dataset from climate_ref.models.diagnostic import Diagnostic as DiagnosticModel @@ -102,31 +101,31 @@ def mock_provider_registry(provider): ] -def _create_results_dir(config, execution): - """Create and return the results directory for an execution.""" - results_dir = config.paths.results / execution.output_fragment - results_dir.mkdir(parents=True, exist_ok=True) - return results_dir +def _create_scratch_dir(config, execution): + """Create and return the scratch directory for an execution.""" + scratch_dir = config.paths.scratch / execution.output_fragment + scratch_dir.mkdir(parents=True, exist_ok=True) + return scratch_dir @pytest.fixture -def output_dir_with_results(config, reingest_execution_obj): - """Create a results directory with empty CMEC template files.""" - results_dir = _create_results_dir(config, reingest_execution_obj) +def scratch_dir_with_results(config, reingest_execution_obj): + """Create a scratch directory with empty CMEC template files (simulates raw diagnostic output).""" + scratch_dir = _create_scratch_dir(config, reingest_execution_obj) - CMECMetric(**CMECMetric.create_template()).dump_to_json(results_dir / "diagnostic.json") - CMECOutput(**CMECOutput.create_template()).dump_to_json(results_dir / "output.json") - TSeries.dump_to_json(results_dir / "series.json", SAMPLE_SERIES) + CMECMetric(**CMECMetric.create_template()).dump_to_json(scratch_dir / "diagnostic.json") + CMECOutput(**CMECOutput.create_template()).dump_to_json(scratch_dir / "output.json") + TSeries.dump_to_json(scratch_dir / "series.json", SAMPLE_SERIES) - return results_dir + return scratch_dir @pytest.fixture -def output_dir_with_data(config, reingest_execution_obj): - """Create a results directory with CMEC files containing actual metric/output data.""" - results_dir = _create_results_dir(config, reingest_execution_obj) +def scratch_dir_with_data(config, reingest_execution_obj): + """Create a scratch directory with CMEC files containing actual metric/output data.""" + scratch_dir = _create_scratch_dir(config, reingest_execution_obj) - (results_dir / "diagnostic.json").write_text( + (scratch_dir / "diagnostic.json").write_text( json.dumps( { "DIMENSIONS": { @@ -139,8 +138,8 @@ def output_dir_with_data(config, reingest_execution_obj): ) ) - (results_dir / "plot.png").write_bytes(b"fake png") - (results_dir / "output.json").write_text( + (scratch_dir / "plot.png").write_bytes(b"fake png") + (scratch_dir / "output.json").write_text( json.dumps( { "index": "index.html", @@ -154,9 +153,9 @@ def output_dir_with_data(config, reingest_execution_obj): ) ) - TSeries.dump_to_json(results_dir / "series.json", SAMPLE_SERIES) + TSeries.dump_to_json(scratch_dir / "series.json", SAMPLE_SERIES) - return results_dir + return scratch_dir @pytest.fixture @@ -239,9 +238,9 @@ def test_reconstruct_output_directory_under_scratch( assert str(definition.output_directory).startswith(str(config.paths.scratch)) -class TestDeleteExecutionResults: - def test_deletes_metric_values_and_outputs(self, reingest_db, reingest_execution_obj): - """Deleting results should remove all metric values and outputs.""" +class TestDeleteExecutionMetricValues: + def test_deletes_metric_values_only(self, reingest_db, reingest_execution_obj): + """Should remove metric values but leave outputs untouched.""" execution = reingest_execution_obj reingest_db.session.add( @@ -264,18 +263,19 @@ def test_deletes_metric_values_and_outputs(self, reingest_db, reingest_execution assert reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() == 1 assert reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).count() == 1 - _delete_execution_results(reingest_db, execution) + _delete_execution_metric_values(reingest_db, execution) reingest_db.session.commit() assert reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() == 0 - assert reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).count() == 0 + # Outputs are not deleted by this function (handled separately in _apply_reingest_mode) + assert reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).count() == 1 class TestReingestExecution: def test_reingest_missing_output_dir( self, config, reingest_db, reingest_execution_obj, mock_provider_registry ): - """Should return False when output directory doesn't exist.""" + """Should return unsuccessful result when output directory doesn't exist.""" result = reingest_execution( config=config, database=reingest_db, @@ -286,7 +286,7 @@ def test_reingest_missing_output_dir( assert result is False def test_reingest_unresolvable_diagnostic(self, config, reingest_db, reingest_execution_obj): - """Should return False when provider registry can't resolve diagnostic.""" + """Should return unsuccessful result when provider registry can't resolve diagnostic.""" empty_registry = ProviderRegistry(providers=[]) result = reingest_execution( config=config, @@ -304,7 +304,7 @@ def test_reingest_replace_mode( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): @@ -323,7 +323,7 @@ def test_reingest_replace_mode( old_count = reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() assert old_count == 1 - mock_result = mock_result_factory(output_dir_with_results) + mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) ok = reingest_execution( @@ -334,7 +334,6 @@ def test_reingest_replace_mode( mode=ReingestMode.replace, ) reingest_db.session.commit() - assert ok is True old_vals = ( reingest_db.session.query(ScalarMetricValue) @@ -351,7 +350,7 @@ def test_reingest_versioned_creates_new_execution( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): @@ -359,7 +358,7 @@ def test_reingest_versioned_creates_new_execution( original_id = reingest_execution_obj.id original_count = reingest_db.session.query(Execution).count() - mock_result = mock_result_factory(output_dir_with_results) + mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) ok = reingest_execution( @@ -370,7 +369,6 @@ def test_reingest_versioned_creates_new_execution( mode=ReingestMode.versioned, ) reingest_db.session.commit() - assert ok is True new_count = reingest_db.session.query(Execution).count() assert new_count == original_count + 1 @@ -385,7 +383,7 @@ def test_reingest_build_execution_result_failure( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mocker, ): """Should return False and skip when build_execution_result raises.""" @@ -412,7 +410,7 @@ def test_reingest_does_not_touch_dirty_flag( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): @@ -427,7 +425,7 @@ def test_reingest_does_not_touch_dirty_flag( reingest_db.session.commit() mock_result = mock_result_factory( - output_dir_with_results, output_bundle_filename=None, series_filename=None + scratch_dir_with_results, output_bundle_filename=None, series_filename=None ) _patch_build_result(mocker, mock_provider_registry, mock_result) @@ -439,7 +437,6 @@ def test_reingest_does_not_touch_dirty_flag( mode=ReingestMode.replace, ) reingest_db.session.commit() - reingest_db.session.refresh(eg) assert eg.dirty is True @@ -450,7 +447,7 @@ def test_additive_preserves_existing_values( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): @@ -468,7 +465,7 @@ def test_additive_preserves_existing_values( reingest_db.session.commit() mock_result = mock_result_factory( - output_dir_with_results, output_bundle_filename=None, series_filename=None + scratch_dir_with_results, output_bundle_filename=None, series_filename=None ) _patch_build_result(mocker, mock_provider_registry, mock_result) @@ -480,7 +477,6 @@ def test_additive_preserves_existing_values( mode=ReingestMode.additive, ) reingest_db.session.commit() - # The pre-existing value should still be there pre_existing = ( reingest_db.session.query(ScalarMetricValue) @@ -497,14 +493,14 @@ def test_additive_reingest_is_idempotent( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): """Running additive reingest twice should not create duplicate rows.""" execution = reingest_execution_obj - mock_result = mock_result_factory(output_dir_with_results) + mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) # First reingest @@ -516,7 +512,6 @@ def test_additive_reingest_is_idempotent( mode=ReingestMode.additive, ) reingest_db.session.commit() - count_after_first = ( reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() ) @@ -533,7 +528,6 @@ def test_additive_reingest_is_idempotent( mode=ReingestMode.additive, ) reingest_db.session.commit() - count_after_second = ( reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() ) @@ -555,7 +549,7 @@ def test_replace_preserves_data_on_ingestion_failure( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): @@ -577,12 +571,12 @@ def test_replace_preserves_data_on_ingestion_failure( assert original_count == 1 mock_result = mock_result_factory( - output_dir_with_results, output_bundle_filename=None, series_filename=None + scratch_dir_with_results, output_bundle_filename=None, series_filename=None ) _patch_build_result(mocker, mock_provider_registry, mock_result) # Make the scalar ingestion fail by corrupting the metric bundle - (output_dir_with_results / "diagnostic.json").write_text("not valid json") + (scratch_dir_with_results / "diagnostic.json").write_text("not valid json") ok = reingest_execution( config=config, @@ -592,7 +586,6 @@ def test_replace_preserves_data_on_ingestion_failure( mode=ReingestMode.replace, ) reingest_db.session.commit() - assert ok is False # Original data should be preserved since the savepoint rolled back @@ -610,12 +603,12 @@ def test_versioned_reingest_twice_creates_unique_fragments( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): """Running versioned reingest twice should create distinct output fragments.""" - mock_result = mock_result_factory(output_dir_with_results) + mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) # First versioned reingest @@ -655,7 +648,7 @@ def test_reingest_marks_failed_execution_as_successful( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): @@ -665,7 +658,7 @@ def test_reingest_marks_failed_execution_as_successful( assert reingest_execution_obj.successful is False mock_result = mock_result_factory( - output_dir_with_results, output_bundle_filename=None, series_filename=None + scratch_dir_with_results, output_bundle_filename=None, series_filename=None ) _patch_build_result(mocker, mock_provider_registry, mock_result) @@ -677,24 +670,23 @@ def test_reingest_marks_failed_execution_as_successful( mode=ReingestMode.replace, ) reingest_db.session.commit() - assert ok is True reingest_db.session.refresh(reingest_execution_obj) assert reingest_execution_obj.successful is True - def test_scratch_directory_cleaned_up_on_success( + def test_scratch_directory_preserved_after_success( self, config, reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mock_result_factory, mocker, ): - """Scratch directory should be removed after successful reingest.""" + """Scratch directory should be preserved after reingest (contains raw outputs).""" mock_result = mock_result_factory( - output_dir_with_results, output_bundle_filename=None, series_filename=None + scratch_dir_with_results, output_bundle_filename=None, series_filename=None ) _patch_build_result(mocker, mock_provider_registry, mock_result) @@ -708,18 +700,18 @@ def test_scratch_directory_cleaned_up_on_success( mode=ReingestMode.additive, ) - assert not scratch_dir.exists(), f"Scratch directory was not cleaned up: {scratch_dir}" + assert scratch_dir.exists(), "Scratch directory should be preserved after reingest" - def test_scratch_directory_cleaned_up_on_failure( + def test_scratch_directory_preserved_after_failure( self, config, reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mocker, ): - """Scratch directory should be removed even when reingest fails.""" + """Scratch directory should be preserved even when reingest fails.""" mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") mocker.patch.object( mock_diagnostic, @@ -738,21 +730,99 @@ def test_scratch_directory_cleaned_up_on_failure( ) assert result is False - assert not scratch_dir.exists(), f"Scratch directory was not cleaned up on failure: {scratch_dir}" + assert scratch_dir.exists(), "Scratch directory should be preserved after failure" + + +class TestCopyWithBackup: + def test_copies_file(self, tmp_path): + """Should copy file to destination.""" + src = tmp_path / "src" / "file.json" + src.parent.mkdir() + src.write_text('{"key": "new"}') + dst = tmp_path / "dst" / "file.json" + + _copy_with_backup(src, dst) + + assert dst.exists() + assert dst.read_text() == '{"key": "new"}' + def test_backs_up_existing_file(self, tmp_path): + """Should create .bak copy when overwriting an existing file.""" + src = tmp_path / "src" / "file.json" + src.parent.mkdir() + src.write_text('{"key": "new"}') + dst = tmp_path / "dst" / "file.json" + dst.parent.mkdir() + dst.write_text('{"key": "old"}') -class TestCopyResultsToScratch: - def test_path_traversal_raises(self, config): - """Should reject output fragments that escape the base directory.""" - with pytest.raises(ValueError, match="Unsafe source path"): - _copy_results_to_scratch(config, "/etc/passwd") + _copy_with_backup(src, dst) - def test_copies_directory(self, config, reingest_execution_obj, output_dir_with_results): - """Should copy the results directory to scratch.""" - scratch = _copy_results_to_scratch(config, reingest_execution_obj.output_fragment) - assert scratch.exists() - assert (scratch / "diagnostic.json").exists() - assert (scratch / "output.json").exists() + assert dst.read_text() == '{"key": "new"}' + backup = dst.with_suffix(".json.bak") + assert backup.exists() + assert backup.read_text() == '{"key": "old"}' + + def test_no_backup_when_no_existing(self, tmp_path): + """Should not create .bak when destination doesn't exist.""" + src = tmp_path / "src" / "file.json" + src.parent.mkdir() + src.write_text('{"key": "value"}') + dst = tmp_path / "dst" / "file.json" + + _copy_with_backup(src, dst) + + assert dst.exists() + assert not dst.with_suffix(".json.bak").exists() + + +class TestSyncReingestFilesToResults: + def test_non_versioned_copies_with_backup(self, config, reingest_execution_obj, scratch_dir_with_results): + """Non-versioned mode should copy CMEC bundles with backup of existing files.""" + fragment = reingest_execution_obj.output_fragment + results_dir = config.paths.results / fragment + results_dir.mkdir(parents=True, exist_ok=True) + (results_dir / "diagnostic.json").write_text('{"old": true}') + + mock_result = type( + "R", + (), + { + "metric_bundle_filename": pathlib.Path("diagnostic.json"), + "output_bundle_filename": pathlib.Path("output.json"), + "series_filename": pathlib.Path("series.json"), + }, + )() + + _sync_reingest_files_to_results(config, fragment, fragment, mock_result, ReingestMode.additive) + + assert (results_dir / "diagnostic.json").exists() + assert (results_dir / "diagnostic.json.bak").exists() + assert (results_dir / "diagnostic.json.bak").read_text() == '{"old": true}' + + def test_versioned_copies_entire_tree(self, config, reingest_execution_obj, scratch_dir_with_results): + """Versioned mode should copy entire scratch tree to new results directory.""" + src_fragment = reingest_execution_obj.output_fragment + dst_fragment = src_fragment + "_v123abc" + + mock_result = type( + "R", + (), + { + "metric_bundle_filename": pathlib.Path("diagnostic.json"), + "output_bundle_filename": None, + "series_filename": None, + }, + )() + + _sync_reingest_files_to_results( + config, src_fragment, dst_fragment, mock_result, ReingestMode.versioned + ) + + dst_dir = config.paths.results / dst_fragment + assert dst_dir.exists() + assert (dst_dir / "diagnostic.json").exists() + assert (dst_dir / "output.json").exists() + assert (dst_dir / "series.json").exists() class TestGetExistingMetricDimensions: @@ -780,18 +850,28 @@ def test_returns_dimension_signatures(self, reingest_db, reingest_execution_obj) assert any(k == "source_id" and v == "model-a" for k, v in sig) -class TestHandleReingestOutputBundle: +class TestRegisterExecutionOutputs: @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_registers_outputs_in_db(self, config, reingest_db, reingest_execution_obj, output_dir_with_data): + def test_registers_outputs_in_db( + self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data + ): """Should register output entries from the bundle into the database.""" - bundle_path = output_dir_with_data / "output.json" - - _handle_reingest_output_bundle(config, reingest_db, reingest_execution_obj, bundle_path) + execution = reingest_execution_obj + bundle_path = scratch_dir_with_data / "output.json" + cmec_output_bundle = CMECOutput.load_from_json(bundle_path) + + results_base = config.paths.results / execution.output_fragment + register_execution_outputs( + reingest_db, + execution, + cmec_output_bundle.plots, + output_type=ResultOutputType.Plot, + base_path=results_base, + fallback_path=scratch_dir_with_data, + ) reingest_db.session.commit() - outputs = ( - reingest_db.session.query(ExecutionOutput).filter_by(execution_id=reingest_execution_obj.id).all() - ) + outputs = reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).all() assert len(outputs) >= 1 assert any(o.short_name == "test_plot" for o in outputs) @@ -799,10 +879,10 @@ def test_registers_outputs_in_db(self, config, reingest_db, reingest_execution_o class TestIngestMetrics: @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") def test_ingest_scalar_values( - self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mock_result_factory + self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data, mock_result_factory ): """Should ingest scalar metric values from a real CMEC bundle.""" - mock_result = mock_result_factory(output_dir_with_data) + mock_result = mock_result_factory(scratch_dir_with_data) cv = CV.load_from_file(config.paths.dimensions_cv) _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) @@ -818,10 +898,10 @@ def test_ingest_scalar_values( @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") def test_ingest_additive_skips_existing( - self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mock_result_factory + self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data, mock_result_factory ): """Additive mode should skip values with existing dimension signatures.""" - mock_result = mock_result_factory(output_dir_with_data) + mock_result = mock_result_factory(scratch_dir_with_data) cv = CV.load_from_file(config.paths.dimensions_cv) # First ingest @@ -844,10 +924,10 @@ def test_ingest_additive_skips_existing( class TestIngestMetricsWithSeries: @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") def test_ingest_series_values( - self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mock_result_factory + self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data, mock_result_factory ): """Should ingest series metric values from a real series file.""" - mock_result = mock_result_factory(output_dir_with_data) + mock_result = mock_result_factory(scratch_dir_with_data) cv = CV.load_from_file(config.paths.dimensions_cv) _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) @@ -862,10 +942,10 @@ def test_ingest_series_values( @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") def test_ingest_series_additive_skips_existing( - self, config, reingest_db, reingest_execution_obj, output_dir_with_data, mock_result_factory + self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data, mock_result_factory ): """Additive mode should skip series with existing dimension signatures.""" - mock_result = mock_result_factory(output_dir_with_data) + mock_result = mock_result_factory(scratch_dir_with_data) cv = CV.load_from_file(config.paths.dimensions_cv) # First ingest @@ -975,7 +1055,7 @@ def test_reingest_unsuccessful_build_result( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mocker, ): """Should return False when build_execution_result returns unsuccessful.""" @@ -1000,7 +1080,7 @@ def test_reingest_successful_but_no_metric_bundle( reingest_db, reingest_execution_obj, mock_provider_registry, - output_dir_with_results, + scratch_dir_with_results, mocker, ): """Should return False when result is successful but metric_bundle_filename is None.""" @@ -1039,12 +1119,14 @@ def test_scratch_path_fallback(self, config, reingest_db, reingest_execution_obj ), } - _handle_reingest_outputs( - outputs=outputs, + results_base = config.paths.results / execution.output_fragment + register_execution_outputs( + reingest_db, + execution, + outputs, output_type=ResultOutputType.Plot, - config=config, - database=reingest_db, - execution=execution, + base_path=results_base, + fallback_path=scratch_base, ) reingest_db.session.commit() @@ -1053,61 +1135,6 @@ def test_scratch_path_fallback(self, config, reingest_db, reingest_execution_obj assert db_outputs[0].short_name == "test_plot" -class TestCopyResultsToScratchExistingDst: - def test_existing_scratch_dir_is_replaced(self, config, reingest_execution_obj, output_dir_with_results): - """When scratch destination already exists, it should be removed and replaced.""" - fragment = reingest_execution_obj.output_fragment - dst = config.paths.scratch / fragment - dst.mkdir(parents=True, exist_ok=True) - (dst / "stale_file.txt").write_text("stale") - - result = _copy_results_to_scratch(config, fragment) - assert result.exists() - assert not (result / "stale_file.txt").exists() - assert (result / "diagnostic.json").exists() - - -class TestCopyScratchToResults: - def test_versioned_copies_to_new_dir(self, config, reingest_db, reingest_execution_obj): - """Versioned mode should copy scratch to a new results directory.""" - execution = reingest_execution_obj - scratch_dir = config.paths.scratch / execution.output_fragment - scratch_dir.mkdir(parents=True, exist_ok=True) - (scratch_dir / "metric.json").write_text("{}") - - original_results = config.paths.results / execution.output_fragment - original_results.mkdir(parents=True, exist_ok=True) - - _copy_scratch_to_results(config, scratch_dir, execution, ReingestMode.versioned, original_results) - - target = config.paths.results / execution.output_fragment - assert target.exists() - assert (target / "metric.json").exists() - - def test_non_versioned_copies_in_place(self, config, reingest_db, reingest_execution_obj): - """Non-versioned mode should copy scratch into original results dir.""" - execution = reingest_execution_obj - scratch_dir = config.paths.scratch / execution.output_fragment - scratch_dir.mkdir(parents=True, exist_ok=True) - (scratch_dir / "new_file.json").write_text("{}") - - original_results = config.paths.results / execution.output_fragment - original_results.mkdir(parents=True, exist_ok=True) - (original_results / "existing.txt").write_text("keep") - - _copy_scratch_to_results(config, scratch_dir, execution, ReingestMode.additive, original_results) - - assert (original_results / "existing.txt").exists() - assert (original_results / "new_file.json").exists() - - -class TestCopyResultsToScratchMissing: - def test_source_not_exists(self, config): - """Should raise FileNotFoundError when source directory doesn't exist.""" - with pytest.raises(FileNotFoundError): - _copy_results_to_scratch(config, "nonexistent/path/fragment") - - class TestGetExecutionsForReingest: @pytest.fixture(autouse=True) def _register_providers(self, db_seeded): From 10a19d1e61b9b74597200f335a908d23698e9bd3 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Fri, 3 Apr 2026 20:12:23 +0900 Subject: [PATCH 13/19] refactor(executor): extract shared ingest_execution_result and add equivalence tests Both handle_execution_result and reingest now delegate to a single ingest_execution_result() function for output registration and metric ingestion, preventing the two paths from drifting. Adds unit test verifying versioned reingest produces equivalent DB state to fresh ingestion, and integration tests that solve with the example provider then reingest in all three modes (additive, replace, versioned). --- .../src/climate_ref/executor/reingest.py | 72 ++---- .../climate_ref/executor/result_handling.py | 182 ++++++++------ .../tests/integration/test_reingest.py | 233 ++++++++++++++++++ .../tests/unit/executor/test_reingest.py | 129 +++++++++- 4 files changed, 476 insertions(+), 140 deletions(-) create mode 100644 packages/climate-ref/tests/integration/test_reingest.py diff --git a/packages/climate-ref/src/climate_ref/executor/reingest.py b/packages/climate-ref/src/climate_ref/executor/reingest.py index 332afe379..48ddbaa8c 100644 --- a/packages/climate-ref/src/climate_ref/executor/reingest.py +++ b/packages/climate-ref/src/climate_ref/executor/reingest.py @@ -28,16 +28,11 @@ from sqlalchemy import delete from climate_ref.datasets import get_slug_column -from climate_ref.executor.result_handling import ( - ingest_scalar_values, - ingest_series_values, - register_execution_outputs, -) +from climate_ref.executor.result_handling import ingest_execution_result from climate_ref.models.execution import ( Execution, ExecutionGroup, ExecutionOutput, - ResultOutputType, execution_datasets, get_execution_group_and_latest_filtered, ) @@ -49,7 +44,6 @@ ) from climate_ref_core.diagnostics import ExecutionDefinition, ExecutionResult from climate_ref_core.pycmec.controlled_vocabulary import CV -from climate_ref_core.pycmec.output import CMECOutput if TYPE_CHECKING: from climate_ref.config import Config @@ -233,7 +227,6 @@ def _sync_reingest_files_to_results( def _delete_execution_metric_values(database: "Database", execution: Execution) -> None: """Delete all metric values for an execution (outputs are handled separately).""" - # MetricValue uses single-table inheritance so we delete from the base table database.session.execute(delete(MetricValue).where(MetricValue.execution_id == execution.id)) @@ -293,26 +286,22 @@ def _apply_reingest_mode( # noqa: PLR0913 database.session.execute( delete(ExecutionOutput).where(ExecutionOutput.execution_id == target_execution.id) ) - cmec_output_bundle = CMECOutput.load_from_json( - result.to_output_path(result.output_bundle_filename) - ) - results_base = config.paths.results / target_execution.output_fragment - scratch_base = config.paths.scratch / execution.output_fragment - for attr, output_type in [ - ("plots", ResultOutputType.Plot), - ("data", ResultOutputType.Data), - ("html", ResultOutputType.HTML), - ]: - register_execution_outputs( - database, - target_execution, - getattr(cmec_output_bundle, attr), - output_type=output_type, - base_path=results_base, - fallback_path=scratch_base, - ) - _ingest_metrics(database, result, target_execution, cv, additive=mode == ReingestMode.additive) + existing = ( + _get_existing_metric_dimensions(database, target_execution) + if mode == ReingestMode.additive + else None + ) + + ingest_execution_result( + database, + target_execution, + result, + cv, + output_base_path=config.paths.results / target_execution.output_fragment, + output_fallback_path=config.paths.scratch / execution.output_fragment, + existing_metrics=existing, + ) assert result.metric_bundle_filename is not None target_execution.mark_successful(result.as_relative_path(result.metric_bundle_filename)) @@ -320,23 +309,6 @@ def _apply_reingest_mode( # noqa: PLR0913 return target_execution -def _ingest_metrics( - database: "Database", - result: ExecutionResult, - execution: Execution, - cv: CV, - *, - additive: bool, -) -> None: - """Ingest scalar and series metric values, using additive dedup when requested.""" - existing = _get_existing_metric_dimensions(database, execution) if additive else None - - if result.series_filename: - ingest_series_values(database=database, result=result, execution=execution, cv=cv, existing=existing) - - ingest_scalar_values(database=database, result=result, execution=execution, cv=cv, existing=existing) - - def reingest_execution( config: "Config", database: "Database", @@ -347,10 +319,11 @@ def reingest_execution( """ Reingest an existing execution. - Re-runs ``build_execution_result()`` against the scratch directory (which - contains the raw outputs from the original diagnostic run) and re-ingests - the results into the database. Updated CMEC bundles are then copied from - scratch to the results directory, backing up previous versions. + Re-runs ``build_execution_result()`` against the scratch directory + (which contains the raw outputs from the original diagnostic run) and re-ingests + the results into the database. + Updated CMEC bundles are then copied from scratch to the results directory, + backing up previous versions. Parameters ---------- @@ -384,9 +357,6 @@ def reingest_execution( ) return False - # The scratch directory contains the raw outputs from the original - # diagnostic run and is never cleaned up. build_execution_result() - # writes CMEC bundles here. scratch_dir = config.paths.scratch / execution.output_fragment if not scratch_dir.exists(): logger.error(f"Scratch directory does not exist: {scratch_dir}. Skipping execution {execution.id}.") diff --git a/packages/climate-ref/src/climate_ref/executor/result_handling.py b/packages/climate-ref/src/climate_ref/executor/result_handling.py index 7702c76fa..8c17fda69 100644 --- a/packages/climate-ref/src/climate_ref/executor/result_handling.py +++ b/packages/climate-ref/src/climate_ref/executor/result_handling.py @@ -211,6 +211,82 @@ def ingest_series_values( database.session.execute(insert(SeriesMetricValue), new_values) +def ingest_execution_result( # noqa: PLR0913 + database: Database, + execution: Execution, + result: "ExecutionResult", + cv: CV, + *, + output_base_path: pathlib.Path, + output_fallback_path: "pathlib.Path | None" = None, + existing_metrics: "set[tuple[tuple[str, str], ...]] | None" = None, +) -> None: + """ + Ingest a successful execution result into the database. + + Registers output entries and ingests scalar and series metric values. + + Parameters + ---------- + database + The active database session to use + execution + The execution record to associate results with + result + The successful execution result + cv + The controlled vocabulary to validate metrics against + output_base_path + Primary base directory for resolving output filenames + output_fallback_path + Secondary base directory when filenames aren't relative to ``output_base_path`` + existing_metrics + When provided, metric values whose dimension signatures are in this set + are skipped (additive dedup). When ``None``, all values are inserted. + + Notes + ----- + Callers are responsible for: + + * File copying (scratch -> results) + * Transaction boundaries + * Marking the execution as successful (``execution.mark_successful()``) + * Setting the dirty flag on the execution group + """ + if result.output_bundle_filename: + cmec_output_bundle = CMECOutput.load_from_json(result.to_output_path(result.output_bundle_filename)) + for attr, output_type in [ + ("plots", ResultOutputType.Plot), + ("data", ResultOutputType.Data), + ("html", ResultOutputType.HTML), + ]: + register_execution_outputs( + database, + execution, + getattr(cmec_output_bundle, attr), + output_type=output_type, + base_path=output_base_path, + fallback_path=output_fallback_path, + ) + + if result.series_filename: + ingest_series_values( + database=database, + result=result, + execution=execution, + cv=cv, + existing=existing_metrics, + ) + + ingest_scalar_values( + database=database, + result=result, + execution=execution, + cv=cv, + existing=existing_metrics, + ) + + def register_execution_outputs( # noqa: PLR0913 database: Database, execution: Execution, @@ -267,56 +343,6 @@ def register_execution_outputs( # noqa: PLR0913 ) -def _process_execution_scalar( - database: Database, - result: "ExecutionResult", - execution: Execution, - cv: CV, -) -> None: - """ - Process the scalar values from the execution result and store them in the database - - This also validates the scalar values against the controlled vocabulary - """ - # Perform a bulk insert of scalar values - # The current implementation will swallow the exception, but display a log message - try: - with database.session.begin_nested(): - ingest_scalar_values(database=database, result=result, execution=execution, cv=cv) - # This is a broad exception catch to ensure we log any issues - except Exception: - logger.exception("Something went wrong when ingesting diagnostic values") - - -def _process_execution_series( - config: "Config", - database: Database, - result: "ExecutionResult", - execution: Execution, - cv: CV, -) -> None: - """ - Process the series values from the execution result and store them in the database - - This also copies the series values file from the scratch directory to the results directory - and validates the series values against the controlled vocabulary. - """ - assert result.series_filename, "Series filename must be set in the result" - - _copy_file_to_results( - config.paths.scratch, - config.paths.results, - execution.output_fragment, - result.series_filename, - ) - - try: - with database.session.begin_nested(): - ingest_series_values(database=database, result=result, execution=execution, cv=cv) - except Exception: - logger.exception("Something went wrong when ingesting diagnostic series values") - - def handle_execution_result( config: "Config", database: Database, @@ -384,23 +410,33 @@ def handle_execution_result( execution.output_fragment, result.output_bundle_filename, ) - _handle_output_bundle( + _copy_output_bundle_files( config, - database, execution, result.to_output_path(result.output_bundle_filename), ) - cv = CV.load_from_file(config.paths.dimensions_cv) - if result.series_filename: - # Process the series values if they are present - # This will ingest the series values into the database - _process_execution_series(config=config, database=database, result=result, execution=execution, cv=cv) + _copy_file_to_results( + config.paths.scratch, + config.paths.results, + execution.output_fragment, + result.series_filename, + ) - # Process the scalar values - # This will ingest the scalar values into the database - _process_execution_scalar(database=database, result=result, execution=execution, cv=cv) + # Ingest outputs and metrics into the database via the shared ingestion path + cv = CV.load_from_file(config.paths.dimensions_cv) + try: + with database.session.begin_nested(): + ingest_execution_result( + database, + execution, + result, + cv, + output_base_path=config.paths.scratch / execution.output_fragment, + ) + except Exception: + logger.exception("Something went wrong when ingesting execution result") # TODO: This should check if the result is the most recent for the execution, # if so then update the dirty fields @@ -411,26 +447,17 @@ def handle_execution_result( execution.mark_successful(result.as_relative_path(result.metric_bundle_filename)) -def _handle_output_bundle( +def _copy_output_bundle_files( config: "Config", - database: Database, execution: Execution, cmec_output_bundle_filename: pathlib.Path, ) -> None: - # Extract the registered outputs - # Copy the content to the output directory - # Track in the db + """Copy output bundle referenced files (plots, data, html) from scratch to results.""" cmec_output_bundle = CMECOutput.load_from_json(cmec_output_bundle_filename) scratch_base = config.paths.scratch / execution.output_fragment - results_base = config.paths.results / execution.output_fragment - - for attr, output_type in [ - ("plots", ResultOutputType.Plot), - ("data", ResultOutputType.Data), - ("html", ResultOutputType.HTML), - ]: - outputs = getattr(cmec_output_bundle, attr) or {} - for output_info in outputs.values(): + + for attr in ("plots", "data", "html"): + for output_info in (getattr(cmec_output_bundle, attr) or {}).values(): filename = ensure_relative_path(output_info.filename, scratch_base) _copy_file_to_results( config.paths.scratch, @@ -438,10 +465,3 @@ def _handle_output_bundle( execution.output_fragment, filename, ) - register_execution_outputs( - database, - execution, - outputs, - output_type=output_type, - base_path=results_base, - ) diff --git a/packages/climate-ref/tests/integration/test_reingest.py b/packages/climate-ref/tests/integration/test_reingest.py new file mode 100644 index 000000000..d944533d4 --- /dev/null +++ b/packages/climate-ref/tests/integration/test_reingest.py @@ -0,0 +1,233 @@ +""" +Integration tests for reingest functionality +""" + +import pytest + +from climate_ref.executor.reingest import ( + ReingestMode, + reconstruct_execution_definition, + reingest_execution, +) +from climate_ref.models import ScalarMetricValue, SeriesMetricValue +from climate_ref.models.dataset import CMIP6Dataset +from climate_ref.models.diagnostic import Diagnostic as DiagnosticModel +from climate_ref.models.execution import Execution, ExecutionGroup, ExecutionOutput, execution_datasets +from climate_ref.models.metric_value import MetricValue +from climate_ref.provider_registry import ProviderRegistry +from climate_ref.solver import solve_required_executions +from climate_ref_core.datasets import SourceDatasetType + + +def test_definition_round_trip(config, db_seeded, provider): + with db_seeded.session.begin(): + datasets = db_seeded.session.query(CMIP6Dataset).limit(2).all() + assert len(datasets) >= 1 + + selector = (("source_id", datasets[0].source_id),) + + diag = db_seeded.session.query(DiagnosticModel).first() + eg = ExecutionGroup( + key="round-trip-test", + diagnostic_id=diag.id, + selectors={SourceDatasetType.CMIP6.value: [list(pair) for pair in selector]}, + ) + db_seeded.session.add(eg) + db_seeded.session.flush() + + ex = Execution( + execution_group_id=eg.id, + successful=True, + output_fragment="test/round-trip/abc", + dataset_hash="h1", + ) + db_seeded.session.add(ex) + db_seeded.session.flush() + + for dataset in datasets: + db_seeded.session.execute( + execution_datasets.insert().values( + execution_id=ex.id, + dataset_id=dataset.id, + ) + ) + + diagnostic = provider.get("mock") + definition = reconstruct_execution_definition(config, ex, diagnostic) + + assert definition.key == "round-trip-test" + assert definition.diagnostic is diagnostic + assert SourceDatasetType.CMIP6 in definition.datasets + + collection = definition.datasets[SourceDatasetType.CMIP6] + + # Dataset IDs should match what was linked + expected_ids = sorted(d.id for d in datasets) + actual_ids = sorted(collection.datasets.index.unique()) + assert expected_ids == actual_ids, f"Dataset IDs: expected {expected_ids}, got {actual_ids}" + + # File paths should be present for all linked datasets + expected_paths = sorted(f.path for d in datasets for f in d.files) + actual_paths = sorted(collection.datasets["path"].tolist()) + assert expected_paths == actual_paths, "File paths not preserved through round-trip" + + # Key facets should survive the round-trip + for facet in ("variable_id", "source_id", "experiment_id"): + assert facet in collection.datasets.columns, f"Missing facet column: {facet}" + + # Selector should match what was stored on the execution group + assert collection.selector == selector + + +def _snapshot_scalars(db, execution): + """Snapshot scalar metrics as a set of (value, dimensions) for comparison.""" + values = db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).all() + return {(v.value, tuple(sorted(v.dimensions.items()))) for v in values} + + +def _snapshot_series(db, execution): + """Snapshot series metrics as a set of (values_tuple, dimensions) for comparison.""" + values = db.session.query(SeriesMetricValue).filter_by(execution_id=execution.id).all() + return {(tuple(v.values), tuple(sorted(v.dimensions.items()))) for v in values} + + +def _snapshot_outputs(db, execution): + """Snapshot outputs as a set of (short_name, output_type, filename) for comparison.""" + outputs = db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).all() + return {(o.short_name, o.output_type, o.filename) for o in outputs} + + +@pytest.fixture +def _solved_example(config, db_seeded): + """Run the example provider's diagnostics via solve, producing real executions.""" + solve_required_executions(db=db_seeded, config=config, one_per_diagnostic=True) + + # Verify at least one successful execution was produced + successful = db_seeded.session.query(Execution).filter_by(successful=True).all() + assert len(successful) >= 1, "solve_required_executions should produce at least one successful execution" + + # Close any lingering transaction so test methods start clean + if db_seeded.session.in_transaction(): + db_seeded.session.commit() + + +@pytest.fixture +def provider_registry(config, db_seeded): + """Build a ProviderRegistry from the seeded database.""" + return ProviderRegistry.build_from_config(config, db_seeded) + + +@pytest.mark.usefixtures("_solved_example") +class TestReingestAfterSolve: + """End-to-end: solve with example provider, then reingest in each mode.""" + + def _get_successful_execution(self, db_seeded): + """Get the first successful execution with metric values.""" + executions = db_seeded.session.query(Execution).filter_by(successful=True).all() + for ex in executions: + if db_seeded.session.query(MetricValue).filter_by(execution_id=ex.id).count() > 0: + return ex + pytest.skip("No successful execution with metric values found") + + def test_reingest_replace_preserves_metric_count(self, config, db_seeded, provider_registry): + """Replace reingest should produce the same number of metrics as the original.""" + execution = self._get_successful_execution(db_seeded) + + original_scalars = _snapshot_scalars(db_seeded, execution) + original_series = _snapshot_series(db_seeded, execution) + assert original_scalars, "Execution should have scalar metric values" + + if db_seeded.session.in_transaction(): + db_seeded.session.commit() + + with db_seeded.session.begin(): + ok = reingest_execution( + config=config, + database=db_seeded, + execution=execution, + provider_registry=provider_registry, + mode=ReingestMode.replace, + ) + assert ok is True + + replaced_scalars = _snapshot_scalars(db_seeded, execution) + replaced_series = _snapshot_series(db_seeded, execution) + + assert original_scalars == replaced_scalars, "Replace reingest should produce identical scalars" + assert original_series == replaced_series, "Replace reingest should produce identical series" + + def test_reingest_additive_is_idempotent(self, config, db_seeded, provider_registry): + """Running additive reingest twice should not create duplicate metrics.""" + execution = self._get_successful_execution(db_seeded) + + count_before = db_seeded.session.query(MetricValue).filter_by(execution_id=execution.id).count() + + if db_seeded.session.in_transaction(): + db_seeded.session.commit() + + with db_seeded.session.begin(): + ok = reingest_execution( + config=config, + database=db_seeded, + execution=execution, + provider_registry=provider_registry, + mode=ReingestMode.additive, + ) + assert ok is True + + count_after = db_seeded.session.query(MetricValue).filter_by(execution_id=execution.id).count() + assert count_before == count_after, ( + f"Additive reingest created duplicates: {count_before} -> {count_after}" + ) + + def test_reingest_versioned_creates_equivalent_execution(self, config, db_seeded, provider_registry): + """Versioned reingest should create a new execution with equivalent metrics and outputs.""" + execution = self._get_successful_execution(db_seeded) + + original_scalars = _snapshot_scalars(db_seeded, execution) + original_series = _snapshot_series(db_seeded, execution) + original_outputs = _snapshot_outputs(db_seeded, execution) + + execution_count_before = db_seeded.session.query(Execution).count() + + if db_seeded.session.in_transaction(): + db_seeded.session.commit() + + with db_seeded.session.begin(): + ok = reingest_execution( + config=config, + database=db_seeded, + execution=execution, + provider_registry=provider_registry, + mode=ReingestMode.versioned, + ) + assert ok is True + + # A new execution should exist + execution_count_after = db_seeded.session.query(Execution).count() + assert execution_count_after == execution_count_before + 1 + + # Find the versioned execution + eg = execution.execution_group + new_execution = ( + db_seeded.session.query(Execution) + .filter( + Execution.execution_group_id == eg.id, + Execution.id != execution.id, + Execution.successful.is_(True), + ) + .order_by(Execution.id.desc()) + .first() + ) + assert new_execution is not None + + versioned_scalars = _snapshot_scalars(db_seeded, new_execution) + versioned_series = _snapshot_series(db_seeded, new_execution) + versioned_outputs = _snapshot_outputs(db_seeded, new_execution) + + assert original_scalars == versioned_scalars, "Versioned scalars should match original" + assert original_series == versioned_series, "Versioned series should match original" + assert original_outputs == versioned_outputs, "Versioned outputs should match original" + + # Original execution should be untouched + assert _snapshot_scalars(db_seeded, execution) == original_scalars diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index 5246d3725..69e587408 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -13,13 +13,17 @@ _delete_execution_metric_values, _extract_dataset_attributes, _get_existing_metric_dimensions, - _ingest_metrics, _sync_reingest_files_to_results, get_executions_for_reingest, reconstruct_execution_definition, reingest_execution, ) -from climate_ref.executor.result_handling import register_execution_outputs +from climate_ref.executor.result_handling import ( + ingest_execution_result, + ingest_scalar_values, + ingest_series_values, + register_execution_outputs, +) from climate_ref.models import ScalarMetricValue, SeriesMetricValue from climate_ref.models.dataset import CMIP6Dataset from climate_ref.models.diagnostic import Diagnostic as DiagnosticModel @@ -885,7 +889,9 @@ def test_ingest_scalar_values( mock_result = mock_result_factory(scratch_dir_with_data) cv = CV.load_from_file(config.paths.dimensions_cv) - _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) + ingest_scalar_values( + database=reingest_db, result=mock_result, execution=reingest_execution_obj, cv=cv + ) reingest_db.session.commit() scalars = ( @@ -905,14 +911,23 @@ def test_ingest_additive_skips_existing( cv = CV.load_from_file(config.paths.dimensions_cv) # First ingest - _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) + ingest_scalar_values( + database=reingest_db, result=mock_result, execution=reingest_execution_obj, cv=cv + ) reingest_db.session.commit() count_first = ( reingest_db.session.query(MetricValue).filter_by(execution_id=reingest_execution_obj.id).count() ) # Second ingest in additive mode should not duplicate - _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=True) + existing = _get_existing_metric_dimensions(reingest_db, reingest_execution_obj) + ingest_scalar_values( + database=reingest_db, + result=mock_result, + execution=reingest_execution_obj, + cv=cv, + existing=existing, + ) reingest_db.session.commit() count_second = ( reingest_db.session.query(MetricValue).filter_by(execution_id=reingest_execution_obj.id).count() @@ -930,7 +945,9 @@ def test_ingest_series_values( mock_result = mock_result_factory(scratch_dir_with_data) cv = CV.load_from_file(config.paths.dimensions_cv) - _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) + ingest_series_values( + database=reingest_db, result=mock_result, execution=reingest_execution_obj, cv=cv + ) reingest_db.session.commit() series = ( @@ -949,7 +966,9 @@ def test_ingest_series_additive_skips_existing( cv = CV.load_from_file(config.paths.dimensions_cv) # First ingest - _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=False) + ingest_series_values( + database=reingest_db, result=mock_result, execution=reingest_execution_obj, cv=cv + ) reingest_db.session.commit() count_first = ( @@ -959,7 +978,14 @@ def test_ingest_series_additive_skips_existing( ) # Second ingest in additive mode - _ingest_metrics(reingest_db, mock_result, reingest_execution_obj, cv, additive=True) + existing = _get_existing_metric_dimensions(reingest_db, reingest_execution_obj) + ingest_series_values( + database=reingest_db, + result=mock_result, + execution=reingest_execution_obj, + cv=cv, + existing=existing, + ) reingest_db.session.commit() count_second = ( @@ -1289,3 +1315,90 @@ def test_filters_out_none_executions(self, db_seeded): results = get_executions_for_reingest(db_seeded, execution_group_ids=[eg.id]) assert len(results) == 0 + + +def _snapshot_scalars(db, execution): + """Snapshot scalar metrics as a set of (value, dimensions) for comparison.""" + values = db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).all() + return {(v.value, tuple(sorted(v.dimensions.items()))) for v in values} + + +def _snapshot_series(db, execution): + """Snapshot series metrics as a set of (values_tuple, dimensions) for comparison.""" + values = db.session.query(SeriesMetricValue).filter_by(execution_id=execution.id).all() + return {(tuple(v.values), tuple(sorted(v.dimensions.items()))) for v in values} + + +def _snapshot_outputs(db, execution): + """Snapshot outputs as a set of (short_name, output_type, filename) for comparison.""" + outputs = db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).all() + return {(o.short_name, o.output_type, o.filename) for o in outputs} + + +class TestVersionedReingestEquivalence: + """Versioned reingest should produce the same DB state as fresh ingestion.""" + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_versioned_reingest_matches_original( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + scratch_dir_with_data, + mock_result_factory, + mocker, + ): + """Versioned reingest should produce equivalent metrics and outputs to the original.""" + execution = reingest_execution_obj + mock_result = mock_result_factory(scratch_dir_with_data) + _patch_build_result(mocker, mock_provider_registry, mock_result) + + # Original ingestion via the shared path + cv = CV.load_from_file(config.paths.dimensions_cv) + ingest_execution_result( + reingest_db, + execution, + mock_result, + cv, + output_base_path=config.paths.scratch / execution.output_fragment, + ) + execution.mark_successful(mock_result.as_relative_path(mock_result.metric_bundle_filename)) + reingest_db.session.commit() + + # Snapshot original DB state + original_scalars = _snapshot_scalars(reingest_db, execution) + original_series = _snapshot_series(reingest_db, execution) + original_outputs = _snapshot_outputs(reingest_db, execution) + + assert original_scalars, "Original ingestion should produce scalar values" + + # Versioned reingest creates a new execution from the same data + ok = reingest_execution( + config=config, + database=reingest_db, + execution=execution, + provider_registry=mock_provider_registry, + mode=ReingestMode.versioned, + ) + reingest_db.session.commit() + assert ok is True + + # Find the new execution created by versioned reingest + new_execution = reingest_db.session.query(Execution).filter(Execution.id != execution.id).one() + + # Snapshot reingest DB state + reingest_scalars = _snapshot_scalars(reingest_db, new_execution) + reingest_series = _snapshot_series(reingest_db, new_execution) + reingest_outputs = _snapshot_outputs(reingest_db, new_execution) + + # Both paths go through ingest_execution_result, so results must match + assert original_scalars == reingest_scalars, ( + f"Scalar values differ: original={original_scalars}, reingest={reingest_scalars}" + ) + assert original_series == reingest_series, ( + f"Series values differ: original={original_series}, reingest={reingest_series}" + ) + assert original_outputs == reingest_outputs, ( + f"Output entries differ: original={original_outputs}, reingest={reingest_outputs}" + ) From 9f05ba0b5987d20cb16af063298caf287a6a96b1 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Fri, 3 Apr 2026 20:30:25 +0900 Subject: [PATCH 14/19] fix(test): handle ANSI escape codes in typer missing option error The '--mode' text in typer's error output includes ANSI bold escape codes around the dashes, causing the exact string match to fail in CI. Split into two assertions that match the key parts independently. --- packages/climate-ref/tests/unit/cli/test_executions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/climate-ref/tests/unit/cli/test_executions.py b/packages/climate-ref/tests/unit/cli/test_executions.py index d0860c0c5..f1356bc4f 100644 --- a/packages/climate-ref/tests/unit/cli/test_executions.py +++ b/packages/climate-ref/tests/unit/cli/test_executions.py @@ -1170,4 +1170,5 @@ def test_reingest_success_path(self, db_with_executions, invoke_cli, config): def test_reingest_missing_mode_fails(self, invoke_cli, db_seeded): result = invoke_cli(["executions", "reingest", "--provider", "pmp"], expected_exit_code=2) - assert "Missing option '--mode'" in result.stderr + assert "Missing option" in result.stderr + assert "mode" in result.stderr From df3350d3469d5d1a917656d71da283c04ed7bdcb Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sat, 4 Apr 2026 22:25:34 +0900 Subject: [PATCH 15/19] refactor(reingest): simplify to always-versioned immutable flow Reingest now always creates a new Execution record with a unique output fragment, leaving the original execution untouched. This replaces the previous 3-mode system (additive/replace/versioned) with a single immutable flow that treats results as append-only. Key changes: - Add allocate_output_fragment() helper for non-colliding fragments - Wire fragment allocation into solver for fresh executions too - Remove ReingestMode enum and --mode CLI option - Remove dedup/fallback params from ingest_execution_result() - Delete ~560 net lines of code and tests --- .../src/climate_ref/cli/executions.py | 24 +- .../src/climate_ref/executor/fragment.py | 44 ++ .../src/climate_ref/executor/reingest.py | 246 ++----- .../climate_ref/executor/result_handling.py | 71 +- .../climate-ref/src/climate_ref/solver.py | 47 +- .../tests/integration/test_reingest.py | 73 +- .../tests/unit/cli/test_executions.py | 32 +- .../tests/unit/executor/test_reingest.py | 647 +++++------------- 8 files changed, 332 insertions(+), 852 deletions(-) create mode 100644 packages/climate-ref/src/climate_ref/executor/fragment.py diff --git a/packages/climate-ref/src/climate_ref/cli/executions.py b/packages/climate-ref/src/climate_ref/cli/executions.py index 6b0f6851b..32f6d567b 100644 --- a/packages/climate-ref/src/climate_ref/cli/executions.py +++ b/packages/climate-ref/src/climate_ref/cli/executions.py @@ -21,7 +21,6 @@ from climate_ref.cli._utils import df_to_table, parse_facet_filters, pretty_print_df from climate_ref.config import Config -from climate_ref.executor.reingest import ReingestMode from climate_ref.models import Execution, ExecutionGroup from climate_ref.models.diagnostic import Diagnostic from climate_ref.models.execution import execution_datasets, get_execution_group_and_latest_filtered @@ -745,14 +744,6 @@ def stats( @app.command() def reingest( # noqa: PLR0913 ctx: typer.Context, - mode: Annotated[ - ReingestMode, - typer.Option( - help="Reingest mode: 'additive' (keep existing, add new), " - "'replace' (delete existing, re-ingest), " - "'versioned' (create new execution record)." - ), - ], group_ids: Annotated[ list[int] | None, typer.Argument(help="Execution group IDs to reingest. If omitted, uses filters."), @@ -794,14 +785,8 @@ def reingest( # noqa: PLR0913 the results into the database. Useful when new series definitions or metadata extraction logic has been added. - Three modes are available: - - - additive: Keep existing metric values, add any new ones. Outputs are replaced. - - - replace: Delete all existing metric values and outputs, then re-ingest from scratch. - - - versioned: Create a new Execution record under the same ExecutionGroup, - leaving the original execution untouched. + A new Execution record is always created under the same ExecutionGroup, + leaving the original execution untouched. Results are treated as immutable. The dirty flag is never modified by this command. """ @@ -853,11 +838,11 @@ def reingest( # noqa: PLR0913 ) if dry_run: - console.print(f"[bold]Dry run:[/] would reingest {len(results)} execution(s) in {mode.value} mode:") + console.print(f"[bold]Dry run:[/] would reingest {len(results)} execution(s):") pretty_print_df(preview_df, console=console) return - console.print(f"Will reingest {len(results)} execution(s) in [bold]{mode.value}[/] mode:") + console.print(f"Will reingest {len(results)} execution(s):") pretty_print_df(preview_df, console=console) if not force: @@ -880,7 +865,6 @@ def reingest( # noqa: PLR0913 database=db, execution=ex, provider_registry=provider_registry, - mode=mode, ) if ok: diff --git a/packages/climate-ref/src/climate_ref/executor/fragment.py b/packages/climate-ref/src/climate_ref/executor/fragment.py new file mode 100644 index 000000000..5251ff804 --- /dev/null +++ b/packages/climate-ref/src/climate_ref/executor/fragment.py @@ -0,0 +1,44 @@ +""" +Helpers for allocating non-colliding output fragment paths. +""" + +import pathlib + + +def allocate_output_fragment( + base_fragment: str, + existing_fragments: set[str], + results_dir: pathlib.Path, +) -> str: + """ + Return a non-colliding output fragment path. + + If *base_fragment* is not already used (neither in *existing_fragments* nor + on disk under *results_dir*), it is returned unchanged. Otherwise ``_v2``, + ``_v3``, ... suffixes are tried until a free slot is found. + + Parameters + ---------- + base_fragment + The natural fragment, e.g. ``provider/diagnostic/dataset_hash`` + existing_fragments + Set of ``output_fragment`` values already recorded in the database + for the relevant execution group + results_dir + Root results directory; used to check for orphaned directories on disk + + Returns + ------- + : + A fragment string guaranteed not to collide with *existing_fragments* + or any directory under *results_dir* + """ + if base_fragment not in existing_fragments and not (results_dir / base_fragment).exists(): + return base_fragment + + version = 2 + while True: + candidate = f"{base_fragment}_v{version}" + if candidate not in existing_fragments and not (results_dir / candidate).exists(): + return candidate + version += 1 diff --git a/packages/climate-ref/src/climate_ref/executor/reingest.py b/packages/climate-ref/src/climate_ref/executor/reingest.py index 48ddbaa8c..d9b1588d3 100644 --- a/packages/climate-ref/src/climate_ref/executor/reingest.py +++ b/packages/climate-ref/src/climate_ref/executor/reingest.py @@ -6,18 +6,11 @@ This is useful when new series definitions or metadata extraction logic has been added and you want to apply it to existing outputs without re-executing the diagnostics. -Three reingest modes are supported: - -* **additive** -- preserve existing metric values, insert only values with - dimension signatures not already present for the execution -* **replace** -- delete all existing metric values and outputs, then re-ingest -* **versioned** -- create a new ``Execution`` record under the same ``ExecutionGroup`` - with its own output directory, leaving the original execution untouched +Reingest always creates a new ``Execution`` record under the same ``ExecutionGroup`` +with its own output directory, leaving the original execution untouched. +Results are treated as immutable. """ -import enum -import hashlib -import pathlib import shutil from collections import defaultdict from collections.abc import Sequence @@ -25,24 +18,22 @@ import pandas as pd from loguru import logger -from sqlalchemy import delete from climate_ref.datasets import get_slug_column +from climate_ref.executor.fragment import allocate_output_fragment from climate_ref.executor.result_handling import ingest_execution_result from climate_ref.models.execution import ( Execution, ExecutionGroup, - ExecutionOutput, execution_datasets, get_execution_group_and_latest_filtered, ) -from climate_ref.models.metric_value import MetricValue from climate_ref_core.datasets import ( DatasetCollection, ExecutionDatasetCollection, SourceDatasetType, ) -from climate_ref_core.diagnostics import ExecutionDefinition, ExecutionResult +from climate_ref_core.diagnostics import ExecutionDefinition from climate_ref_core.pycmec.controlled_vocabulary import CV if TYPE_CHECKING: @@ -53,14 +44,6 @@ from climate_ref_core.diagnostics import Diagnostic -class ReingestMode(enum.Enum): - """Mode for reingesting execution results.""" - - additive = "additive" - replace = "replace" - versioned = "versioned" - - def reconstruct_execution_definition( config: "Config", execution: Execution, @@ -85,7 +68,7 @@ def reconstruct_execution_definition( Returns ------- : - A reconstructed ``ExecutionDefinition`` pointing at the results directory + A reconstructed ``ExecutionDefinition`` pointing at the scratch directory """ execution_group = execution.execution_group @@ -166,164 +149,21 @@ def _extract_dataset_attributes(dataset: "Dataset") -> dict[str, object]: return attrs -def _get_existing_metric_dimensions( - database: "Database", execution: Execution -) -> set[tuple[tuple[str, str], ...]]: - """ - Get the dimension signatures of all existing metric values for an execution. - - Each signature is a sorted tuple of (dimension_name, value) pairs, making - it suitable for set-based deduplication. - """ - sigs: set[tuple[tuple[str, str], ...]] = set() - for mv in database.session.query(MetricValue).filter(MetricValue.execution_id == execution.id).all(): - dims = tuple(sorted(mv.dimensions.items())) - sigs.add(dims) - return sigs - - -def _copy_with_backup(src: pathlib.Path, dst: pathlib.Path) -> None: - """Copy *src* to *dst*, backing up *dst* first if it already exists.""" - dst.parent.mkdir(parents=True, exist_ok=True) - if dst.exists(): - backup = dst.with_suffix(dst.suffix + ".bak") - shutil.copy2(dst, backup) - shutil.copyfile(src, dst) - - -def _sync_reingest_files_to_results( - config: "Config", - src_fragment: str, - dst_fragment: str, - result: ExecutionResult, - mode: ReingestMode, -) -> None: - """ - Copy re-extracted CMEC bundles from scratch to the results directory. - - For **versioned** mode the entire scratch tree is copied to a new results - directory (no backup needed). For other modes only the CMEC bundle files - are overwritten, with ``.bak`` copies of the previous versions. - """ - src_dir = config.paths.scratch / src_fragment - dst_dir = config.paths.results / dst_fragment - - if mode == ReingestMode.versioned: - dst_dir.parent.mkdir(parents=True, exist_ok=True) - if dst_dir.exists(): - shutil.rmtree(dst_dir) - shutil.copytree(src_dir, dst_dir) - else: - dst_dir.mkdir(parents=True, exist_ok=True) - assert result.metric_bundle_filename is not None - _copy_with_backup(src_dir / result.metric_bundle_filename, dst_dir / result.metric_bundle_filename) - if result.output_bundle_filename: - _copy_with_backup( - src_dir / result.output_bundle_filename, dst_dir / result.output_bundle_filename - ) - if result.series_filename: - _copy_with_backup(src_dir / result.series_filename, dst_dir / result.series_filename) - - -def _delete_execution_metric_values(database: "Database", execution: Execution) -> None: - """Delete all metric values for an execution (outputs are handled separately).""" - database.session.execute(delete(MetricValue).where(MetricValue.execution_id == execution.id)) - - -def _apply_reingest_mode( # noqa: PLR0913 - config: "Config", - database: "Database", - execution: Execution, - result: ExecutionResult, - mode: ReingestMode, - cv: CV, -) -> Execution: - """ - Apply mode-specific DB mutations inside a savepoint. - - Returns the target execution (may differ from ``execution`` in versioned mode). - """ - target_execution = execution - execution_group = execution.execution_group - - with database.session.begin_nested(): - if mode == ReingestMode.versioned: - # Hash is intentionally deterministic for a given execution: - # re-running versioned reingest on the same execution produces the - # same base fragment, with _n{count} suffixes for repeats. - version_hash = hashlib.sha1( # noqa: S324 - f"{execution.output_fragment}-reingest-{execution.id}".encode() - ).hexdigest()[:12] - - base_fragment = f"{execution.output_fragment}_v{version_hash}" - existing_count = sum( - 1 for e in execution_group.executions if e.output_fragment.startswith(base_fragment) - ) - version_suffix = f"_n{existing_count + 1}" if existing_count > 0 else "" - - target_execution = Execution( - execution_group=execution_group, - dataset_hash=execution.dataset_hash, - output_fragment=f"{base_fragment}{version_suffix}", - ) - database.session.add(target_execution) - database.session.flush() - - for dataset in execution.datasets: - database.session.execute( - execution_datasets.insert().values( - execution_id=target_execution.id, - dataset_id=dataset.id, - ) - ) - elif mode == ReingestMode.replace: - _delete_execution_metric_values(database, target_execution) - - # Outputs are always refreshed regardless of mode. - # "Additive" only applies to metric values as keeping stale entries - # would be more confusing than helpful. - if result.output_bundle_filename: - database.session.execute( - delete(ExecutionOutput).where(ExecutionOutput.execution_id == target_execution.id) - ) - - existing = ( - _get_existing_metric_dimensions(database, target_execution) - if mode == ReingestMode.additive - else None - ) - - ingest_execution_result( - database, - target_execution, - result, - cv, - output_base_path=config.paths.results / target_execution.output_fragment, - output_fallback_path=config.paths.scratch / execution.output_fragment, - existing_metrics=existing, - ) - - assert result.metric_bundle_filename is not None - target_execution.mark_successful(result.as_relative_path(result.metric_bundle_filename)) - - return target_execution - - def reingest_execution( config: "Config", database: "Database", execution: Execution, provider_registry: "ProviderRegistry", - mode: ReingestMode = ReingestMode.additive, ) -> bool: """ Reingest an existing execution. Re-runs ``build_execution_result()`` against the scratch directory - (which contains the raw outputs from the original diagnostic run) and re-ingests - the results into the database. - Updated CMEC bundles are then copied from scratch to the results directory, - backing up previous versions. + (which contains the raw outputs from the original diagnostic run), + creates a new ``Execution`` record with a unique output fragment, + copies results to the new location, and ingests metrics into the database. + + The original execution is left untouched. Parameters ---------- @@ -335,8 +175,6 @@ def reingest_execution( The ``Execution`` record to reingest provider_registry Registry of active providers (used to resolve the live diagnostic) - mode - Reingest mode: additive, replace, or versioned Returns ------- @@ -379,33 +217,65 @@ def reingest_execution( ) return False + # Allocate a new, non-colliding output fragment + existing_fragments = {e.output_fragment for e in execution_group.executions} + new_fragment = allocate_output_fragment( + execution.output_fragment, existing_fragments, config.paths.results + ) + + # Copy scratch tree to the new results location + dst_dir = config.paths.results / new_fragment + dst_dir.parent.mkdir(parents=True, exist_ok=True) + if dst_dir.exists(): + shutil.rmtree(dst_dir) + shutil.copytree(scratch_dir, dst_dir) + cv = CV.load_from_file(config.paths.dimensions_cv) try: - target_execution = _apply_reingest_mode( - config=config, - database=database, - execution=execution, - result=result, - mode=mode, - cv=cv, - ) + with database.session.begin_nested(): + # Create new Execution record + new_execution = Execution( + execution_group=execution_group, + dataset_hash=execution.dataset_hash, + output_fragment=new_fragment, + ) + database.session.add(new_execution) + database.session.flush() + + # Copy dataset links from the original execution + for dataset in execution.datasets: + database.session.execute( + execution_datasets.insert().values( + execution_id=new_execution.id, + dataset_id=dataset.id, + ) + ) + + # Standard ingestion (same path as fresh executions) + ingest_execution_result( + database, + new_execution, + result, + cv, + output_base_path=config.paths.results / new_fragment, + ) + + assert result.metric_bundle_filename is not None + new_execution.mark_successful(result.as_relative_path(result.metric_bundle_filename)) except Exception: logger.exception( f"Ingestion failed for execution {execution.id} " f"({provider_slug}/{diagnostic_slug}). Rolling back changes." ) + # Clean up the copied directory on failure + if dst_dir.exists(): + shutil.rmtree(dst_dir) return False - # Copy re-extracted CMEC bundles from scratch to the results tree. - # For non-versioned modes, existing files are backed up with a .bak suffix. - _sync_reingest_files_to_results( - config, execution.output_fragment, target_execution.output_fragment, result, mode - ) - logger.info( f"Successfully reingested execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}) in {mode.value} mode" + f"({provider_slug}/{diagnostic_slug}) -> new execution {new_execution.id}" ) return True diff --git a/packages/climate-ref/src/climate_ref/executor/result_handling.py b/packages/climate-ref/src/climate_ref/executor/result_handling.py index 8c17fda69..9f935360b 100644 --- a/packages/climate-ref/src/climate_ref/executor/result_handling.py +++ b/packages/climate-ref/src/climate_ref/executor/result_handling.py @@ -71,8 +71,6 @@ def ingest_scalar_values( result: "ExecutionResult", execution: Execution, cv: CV, - *, - existing: "set[tuple[tuple[str, str], ...]] | None" = None, ) -> None: """ Load, validate, and bulk-insert scalar metric values. @@ -87,10 +85,6 @@ def ingest_scalar_values( The execution record to associate values with cv The controlled vocabulary to validate against - existing - When provided, dimension signatures already in the DB; values whose - signature is in this set are skipped (additive dedup). When None, - all values are inserted unconditionally. Notes ----- @@ -108,13 +102,7 @@ def ingest_scalar_values( ) new_values = [] - total_count = 0 for metric_result in cmec_metric_bundle.iter_results(): - total_count += 1 - if existing is not None: - dims = tuple(sorted(metric_result.dimensions.items())) - if dims in existing: - continue new_values.append( { "execution_id": execution.id, @@ -124,14 +112,7 @@ def ingest_scalar_values( } ) - skipped = total_count - len(new_values) - if existing is not None: - logger.debug( - f"Additive: {len(new_values)} new scalar values " - f"(skipped {skipped} existing) for execution {execution.id}" - ) - else: - logger.debug(f"Ingesting {len(new_values)} scalar values for execution {execution.id}") + logger.debug(f"Ingesting {len(new_values)} scalar values for execution {execution.id}") if new_values: database.session.execute(insert(ScalarMetricValue), new_values) @@ -142,8 +123,6 @@ def ingest_series_values( result: "ExecutionResult", execution: Execution, cv: CV, - *, - existing: "set[tuple[tuple[str, str], ...]] | None" = None, ) -> None: """ Load, validate, and bulk-insert series metric values. @@ -158,10 +137,6 @@ def ingest_series_values( The execution record to associate values with cv The controlled vocabulary to validate against - existing - When provided, dimension signatures already in the DB; series whose - signature is in this set are skipped (additive dedup). When None, - all values are inserted unconditionally. Notes ----- @@ -183,10 +158,6 @@ def ingest_series_values( new_values = [] for series_result in series_values: - if existing is not None: - dims = tuple(sorted(series_result.dimensions.items())) - if dims in existing: - continue new_values.append( { "execution_id": execution.id, @@ -198,28 +169,19 @@ def ingest_series_values( } ) - skipped = len(series_values) - len(new_values) - if existing is not None: - logger.debug( - f"Additive: {len(new_values)} new series values " - f"(skipped {skipped} existing) for execution {execution.id}" - ) - else: - logger.debug(f"Ingesting {len(new_values)} series values for execution {execution.id}") + logger.debug(f"Ingesting {len(new_values)} series values for execution {execution.id}") if new_values: database.session.execute(insert(SeriesMetricValue), new_values) -def ingest_execution_result( # noqa: PLR0913 +def ingest_execution_result( database: Database, execution: Execution, result: "ExecutionResult", cv: CV, *, output_base_path: pathlib.Path, - output_fallback_path: "pathlib.Path | None" = None, - existing_metrics: "set[tuple[tuple[str, str], ...]] | None" = None, ) -> None: """ Ingest a successful execution result into the database. @@ -238,11 +200,6 @@ def ingest_execution_result( # noqa: PLR0913 The controlled vocabulary to validate metrics against output_base_path Primary base directory for resolving output filenames - output_fallback_path - Secondary base directory when filenames aren't relative to ``output_base_path`` - existing_metrics - When provided, metric values whose dimension signatures are in this set - are skipped (additive dedup). When ``None``, all values are inserted. Notes ----- @@ -266,7 +223,6 @@ def ingest_execution_result( # noqa: PLR0913 getattr(cmec_output_bundle, attr), output_type=output_type, base_path=output_base_path, - fallback_path=output_fallback_path, ) if result.series_filename: @@ -275,7 +231,6 @@ def ingest_execution_result( # noqa: PLR0913 result=result, execution=execution, cv=cv, - existing=existing_metrics, ) ingest_scalar_values( @@ -283,25 +238,21 @@ def ingest_execution_result( # noqa: PLR0913 result=result, execution=execution, cv=cv, - existing=existing_metrics, ) -def register_execution_outputs( # noqa: PLR0913 +def register_execution_outputs( database: Database, execution: Execution, outputs: "dict[str, OutputDict] | None", output_type: ResultOutputType, *, base_path: pathlib.Path, - fallback_path: "pathlib.Path | None" = None, ) -> None: """ Register output entries in the database. - Each entry in ``outputs`` is resolved relative to ``base_path``. When a - filename is not relative to ``base_path`` (raises ``ValueError``), it is - resolved relative to ``fallback_path`` instead (if provided). + Each entry in ``outputs`` is resolved relative to ``base_path``. Parameters ---------- @@ -314,22 +265,14 @@ def register_execution_outputs( # noqa: PLR0913 output_type The type of output being registered base_path - Primary base directory for resolving relative filenames - fallback_path - Secondary base directory used when ``ensure_relative_path`` raises - ``ValueError`` against ``base_path`` + Base directory for resolving relative filenames Notes ----- Callers are responsible for transaction boundaries. """ for key, output_info in (outputs or {}).items(): - try: - filename = ensure_relative_path(output_info.filename, base_path) - except ValueError: - if fallback_path is None: - raise - filename = ensure_relative_path(output_info.filename, fallback_path) + filename = ensure_relative_path(output_info.filename, base_path) database.session.add( ExecutionOutput.build( execution_id=execution.id, diff --git a/packages/climate-ref/src/climate_ref/solver.py b/packages/climate-ref/src/climate_ref/solver.py index b92d11dab..6822529ce 100644 --- a/packages/climate-ref/src/climate_ref/solver.py +++ b/packages/climate-ref/src/climate_ref/solver.py @@ -23,6 +23,7 @@ PMPClimatologyDatasetAdapter, get_slug_column, ) +from climate_ref.executor.fragment import allocate_output_fragment from climate_ref.models import Diagnostic as DiagnosticModel from climate_ref.models import ExecutionGroup from climate_ref.models import Provider as ProviderModel @@ -95,15 +96,39 @@ def selectors(self) -> dict[str, Selector]: """ return self.datasets.selectors - def build_execution_definition(self, output_root: pathlib.Path) -> ExecutionDefinition: + def build_execution_definition( + self, + output_root: pathlib.Path, + *, + existing_fragments: set[str] | None = None, + results_dir: pathlib.Path | None = None, + ) -> ExecutionDefinition: """ Build the execution definition for the current diagnostic execution + + Parameters + ---------- + output_root + Root directory for storing execution output (typically scratch) + existing_fragments + Output fragments already recorded in the database for this + execution group. When provided together with *results_dir*, + the fragment is passed through + :func:`~climate_ref.executor.fragment.allocate_output_fragment` + to avoid collisions. + results_dir + Root results directory, used for on-disk collision checks """ # Ensure that the output root is always an absolute path output_root = output_root.resolve() # This is the desired path relative to the output directory - fragment = pathlib.Path() / self.provider.slug / self.diagnostic.slug / self.datasets.hash + base_fragment = str(pathlib.Path() / self.provider.slug / self.diagnostic.slug / self.datasets.hash) + + if existing_fragments is not None and results_dir is not None: + fragment = allocate_output_fragment(base_fragment, existing_fragments, results_dir) + else: + fragment = base_fragment return ExecutionDefinition( diagnostic=self.diagnostic, @@ -553,11 +578,8 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 total_count = 0 for potential_execution in solver.solve(filters): - # The diagnostic output is first written to the scratch directory - definition = potential_execution.build_execution_definition(output_root=config.paths.scratch) - logger.debug( - f"Identified candidate execution {definition.key} " + f"Identified candidate execution {potential_execution.dataset_key} " f"for {potential_execution.diagnostic.full_slug()}" ) @@ -580,7 +602,7 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 ) execution_group, created = db.get_or_create( ExecutionGroup, - key=definition.key, + key=potential_execution.dataset_key, diagnostic_id=diagnostic.id, defaults={ "selectors": potential_execution.selectors, @@ -604,7 +626,7 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 f"provider_count={provider_count}" ) - if execution_group.should_run(definition.datasets.hash, rerun_failed=rerun_failed): + if execution_group.should_run(potential_execution.datasets.hash, rerun_failed=rerun_failed): if (one_per_provider or one_per_diagnostic) and one_of_check_failed: logger.info( f"Skipping execution due to one-of check: {potential_execution.execution_slug()!r}" @@ -620,9 +642,18 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 break continue + # Build the definition now that we know fragment collision info + existing_fragments = {e.output_fragment for e in execution_group.executions} + definition = potential_execution.build_execution_definition( + output_root=config.paths.scratch, + existing_fragments=existing_fragments, + results_dir=config.paths.results, + ) + logger.info( f"Running new execution for execution group: {potential_execution.execution_slug()!r}" ) + execution = Execution( execution_group=execution_group, dataset_hash=definition.datasets.hash, diff --git a/packages/climate-ref/tests/integration/test_reingest.py b/packages/climate-ref/tests/integration/test_reingest.py index d944533d4..b0ee862d8 100644 --- a/packages/climate-ref/tests/integration/test_reingest.py +++ b/packages/climate-ref/tests/integration/test_reingest.py @@ -5,7 +5,6 @@ import pytest from climate_ref.executor.reingest import ( - ReingestMode, reconstruct_execution_definition, reingest_execution, ) @@ -119,7 +118,7 @@ def provider_registry(config, db_seeded): @pytest.mark.usefixtures("_solved_example") class TestReingestAfterSolve: - """End-to-end: solve with example provider, then reingest in each mode.""" + """End-to-end: solve with example provider, then reingest.""" def _get_successful_execution(self, db_seeded): """Get the first successful execution with metric values.""" @@ -129,59 +128,8 @@ def _get_successful_execution(self, db_seeded): return ex pytest.skip("No successful execution with metric values found") - def test_reingest_replace_preserves_metric_count(self, config, db_seeded, provider_registry): - """Replace reingest should produce the same number of metrics as the original.""" - execution = self._get_successful_execution(db_seeded) - - original_scalars = _snapshot_scalars(db_seeded, execution) - original_series = _snapshot_series(db_seeded, execution) - assert original_scalars, "Execution should have scalar metric values" - - if db_seeded.session.in_transaction(): - db_seeded.session.commit() - - with db_seeded.session.begin(): - ok = reingest_execution( - config=config, - database=db_seeded, - execution=execution, - provider_registry=provider_registry, - mode=ReingestMode.replace, - ) - assert ok is True - - replaced_scalars = _snapshot_scalars(db_seeded, execution) - replaced_series = _snapshot_series(db_seeded, execution) - - assert original_scalars == replaced_scalars, "Replace reingest should produce identical scalars" - assert original_series == replaced_series, "Replace reingest should produce identical series" - - def test_reingest_additive_is_idempotent(self, config, db_seeded, provider_registry): - """Running additive reingest twice should not create duplicate metrics.""" - execution = self._get_successful_execution(db_seeded) - - count_before = db_seeded.session.query(MetricValue).filter_by(execution_id=execution.id).count() - - if db_seeded.session.in_transaction(): - db_seeded.session.commit() - - with db_seeded.session.begin(): - ok = reingest_execution( - config=config, - database=db_seeded, - execution=execution, - provider_registry=provider_registry, - mode=ReingestMode.additive, - ) - assert ok is True - - count_after = db_seeded.session.query(MetricValue).filter_by(execution_id=execution.id).count() - assert count_before == count_after, ( - f"Additive reingest created duplicates: {count_before} -> {count_after}" - ) - - def test_reingest_versioned_creates_equivalent_execution(self, config, db_seeded, provider_registry): - """Versioned reingest should create a new execution with equivalent metrics and outputs.""" + def test_reingest_creates_equivalent_execution(self, config, db_seeded, provider_registry): + """Reingest should create a new execution with equivalent metrics and outputs.""" execution = self._get_successful_execution(db_seeded) original_scalars = _snapshot_scalars(db_seeded, execution) @@ -199,7 +147,6 @@ def test_reingest_versioned_creates_equivalent_execution(self, config, db_seeded database=db_seeded, execution=execution, provider_registry=provider_registry, - mode=ReingestMode.versioned, ) assert ok is True @@ -207,7 +154,7 @@ def test_reingest_versioned_creates_equivalent_execution(self, config, db_seeded execution_count_after = db_seeded.session.query(Execution).count() assert execution_count_after == execution_count_before + 1 - # Find the versioned execution + # Find the new execution eg = execution.execution_group new_execution = ( db_seeded.session.query(Execution) @@ -221,13 +168,13 @@ def test_reingest_versioned_creates_equivalent_execution(self, config, db_seeded ) assert new_execution is not None - versioned_scalars = _snapshot_scalars(db_seeded, new_execution) - versioned_series = _snapshot_series(db_seeded, new_execution) - versioned_outputs = _snapshot_outputs(db_seeded, new_execution) + new_scalars = _snapshot_scalars(db_seeded, new_execution) + new_series = _snapshot_series(db_seeded, new_execution) + new_outputs = _snapshot_outputs(db_seeded, new_execution) - assert original_scalars == versioned_scalars, "Versioned scalars should match original" - assert original_series == versioned_series, "Versioned series should match original" - assert original_outputs == versioned_outputs, "Versioned outputs should match original" + assert original_scalars == new_scalars, "Reingested scalars should match original" + assert original_series == new_series, "Reingested series should match original" + assert original_outputs == new_outputs, "Reingested outputs should match original" # Original execution should be untouched assert _snapshot_scalars(db_seeded, execution) == original_scalars diff --git a/packages/climate-ref/tests/unit/cli/test_executions.py b/packages/climate-ref/tests/unit/cli/test_executions.py index f1356bc4f..b714f89dc 100644 --- a/packages/climate-ref/tests/unit/cli/test_executions.py +++ b/packages/climate-ref/tests/unit/cli/test_executions.py @@ -1085,22 +1085,18 @@ def db_with_executions(self, db_seeded, config): def test_reingest_no_filters_error(self, invoke_cli, db_seeded): """Calling reingest with no filters should exit with code 1.""" - result = invoke_cli(["executions", "reingest", "--mode", "additive"], expected_exit_code=1) + result = invoke_cli(["executions", "reingest"], expected_exit_code=1) assert "At least one filter is required" in result.stderr def test_reingest_no_results(self, db_with_executions, invoke_cli): """When filters match nothing, should print a message and exit cleanly.""" - result = invoke_cli( - ["executions", "reingest", "--mode", "additive", "--provider", "nonexistent", "--force"] - ) + result = invoke_cli(["executions", "reingest", "--provider", "nonexistent", "--force"]) assert result.exit_code == 0 assert "No executions found" in result.stdout def test_reingest_dry_run(self, db_with_executions, invoke_cli): """Dry run should show preview without making changes.""" - result = invoke_cli( - ["executions", "reingest", "--mode", "additive", "--provider", "pmp", "--dry-run", "--force"] - ) + result = invoke_cli(["executions", "reingest", "--provider", "pmp", "--dry-run", "--force"]) assert result.exit_code == 0 assert "Dry run" in result.stdout assert "enso_tel" in result.stdout @@ -1108,14 +1104,14 @@ def test_reingest_dry_run(self, db_with_executions, invoke_cli): def test_reingest_cancellation(self, db_with_executions, invoke_cli): """User declining confirmation should cancel reingest.""" with patch("climate_ref.cli.executions.typer.confirm", return_value=False): - result = invoke_cli(["executions", "reingest", "--mode", "additive", "--provider", "pmp"]) + result = invoke_cli(["executions", "reingest", "--provider", "pmp"]) assert result.exit_code == 0 assert "Reingest cancelled" in result.stdout def test_reingest_force_runs(self, db_with_executions, invoke_cli, config): """Force mode should skip confirmation. Even if reingest_execution returns False (no output dirs), we exercise the CLI loop and get skip counts.""" - result = invoke_cli(["executions", "reingest", "--mode", "additive", "--provider", "pmp", "--force"]) + result = invoke_cli(["executions", "reingest", "--provider", "pmp", "--force"]) assert result.exit_code == 0 assert "Reingest complete" in result.stdout # Output dir doesn't exist so reingest_execution returns False -> skipped @@ -1124,26 +1120,17 @@ def test_reingest_force_runs(self, db_with_executions, invoke_cli, config): def test_reingest_by_group_ids(self, db_with_executions, invoke_cli): """Passing group IDs directly should work.""" eg = db_with_executions.session.query(ExecutionGroup).filter_by(key="reingest-1").first() - result = invoke_cli(["executions", "reingest", "--mode", "additive", str(eg.id), "--dry-run"]) + result = invoke_cli(["executions", "reingest", str(eg.id), "--dry-run"]) assert result.exit_code == 0 assert "Dry run" in result.stdout assert "reingest-1" in result.stdout - @pytest.mark.parametrize("mode", ["additive", "replace", "versioned"]) - def test_reingest_mode_option(self, db_with_executions, invoke_cli, mode): - """Each valid mode should be accepted and displayed.""" - result = invoke_cli(["executions", "reingest", "--mode", mode, "--provider", "pmp", "--dry-run"]) - assert result.exit_code == 0 - assert mode in result.stdout - def test_reingest_include_failed(self, db_with_executions, invoke_cli): """--include-failed should include failed executions in preview.""" result = invoke_cli( [ "executions", "reingest", - "--mode", - "additive", "--provider", "esmvaltool", "--include-failed", @@ -1163,12 +1150,7 @@ def test_reingest_success_path(self, db_with_executions, invoke_cli, config): # Mock reingest_execution to return True (success) with patch("climate_ref.executor.reingest.reingest_execution", return_value=True): - result = invoke_cli(["executions", "reingest", "--mode", "additive", str(eg.id), "--force"]) + result = invoke_cli(["executions", "reingest", str(eg.id), "--force"]) assert result.exit_code == 0 assert "1 succeeded" in result.stdout assert "0 skipped" in result.stdout - - def test_reingest_missing_mode_fails(self, invoke_cli, db_seeded): - result = invoke_cli(["executions", "reingest", "--provider", "pmp"], expected_exit_code=2) - assert "Missing option" in result.stderr - assert "mode" in result.stderr diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index 69e587408..d2ebeaa0e 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -1,4 +1,4 @@ -"""Tests for the reingest module.""" +"""Tests for the reingest module and allocate_output_fragment helper.""" import json import pathlib @@ -7,13 +7,9 @@ from climate_ref_esmvaltool import provider as esmvaltool_provider from climate_ref_pmp import provider as pmp_provider +from climate_ref.executor.fragment import allocate_output_fragment from climate_ref.executor.reingest import ( - ReingestMode, - _copy_with_backup, - _delete_execution_metric_values, _extract_dataset_attributes, - _get_existing_metric_dimensions, - _sync_reingest_files_to_results, get_executions_for_reingest, reconstruct_execution_definition, reingest_execution, @@ -34,7 +30,6 @@ ResultOutputType, execution_datasets, ) -from climate_ref.models.metric_value import MetricValue from climate_ref.models.provider import Provider as ProviderModel from climate_ref.provider_registry import ProviderRegistry, _register_provider from climate_ref_core.datasets import SourceDatasetType @@ -42,7 +37,7 @@ from climate_ref_core.metric_values import SeriesMetricValue as TSeries from climate_ref_core.pycmec.controlled_vocabulary import CV from climate_ref_core.pycmec.metric import CMECMetric -from climate_ref_core.pycmec.output import CMECOutput, OutputDict +from climate_ref_core.pycmec.output import CMECOutput @pytest.fixture @@ -196,16 +191,48 @@ def _patch_build_result(mocker, registry, mock_result): return diagnostic -class TestReingestMode: - def test_enum_values(self): - assert ReingestMode.additive.value == "additive" - assert ReingestMode.replace.value == "replace" - assert ReingestMode.versioned.value == "versioned" +# --- allocate_output_fragment tests --- - def test_from_string(self): - assert ReingestMode("additive") == ReingestMode.additive - assert ReingestMode("replace") == ReingestMode.replace - assert ReingestMode("versioned") == ReingestMode.versioned + +class TestAllocateOutputFragment: + def test_returns_base_when_no_collision(self, tmp_path): + """Should return base_fragment unchanged when no collision exists.""" + result = allocate_output_fragment("provider/diag/abc123", set(), tmp_path) + assert result == "provider/diag/abc123" + + def test_appends_v2_on_db_collision(self, tmp_path): + """Should append _v2 when base_fragment exists in DB.""" + existing = {"provider/diag/abc123"} + result = allocate_output_fragment("provider/diag/abc123", existing, tmp_path) + assert result == "provider/diag/abc123_v2" + + def test_appends_v2_on_disk_collision(self, tmp_path): + """Should append _v2 when base_fragment directory exists on disk.""" + (tmp_path / "provider/diag/abc123").mkdir(parents=True) + result = allocate_output_fragment("provider/diag/abc123", set(), tmp_path) + assert result == "provider/diag/abc123_v2" + + def test_increments_version_on_multiple_collisions(self, tmp_path): + """Should increment to _v3 when both base and _v2 are taken.""" + existing = {"provider/diag/abc123", "provider/diag/abc123_v2"} + result = allocate_output_fragment("provider/diag/abc123", existing, tmp_path) + assert result == "provider/diag/abc123_v3" + + def test_mixed_db_and_disk_collisions(self, tmp_path): + """Should handle collisions from both DB and disk.""" + existing = {"provider/diag/abc123"} + (tmp_path / "provider/diag/abc123_v2").mkdir(parents=True) + result = allocate_output_fragment("provider/diag/abc123", existing, tmp_path) + assert result == "provider/diag/abc123_v3" + + def test_no_collision_returns_base_with_existing_other_fragments(self, tmp_path): + """Other fragments in DB should not cause collision.""" + existing = {"provider/diag/xyz789"} + result = allocate_output_fragment("provider/diag/abc123", existing, tmp_path) + assert result == "provider/diag/abc123" + + +# --- extract dataset attributes tests --- class TestExtractDatasetAttributes: @@ -220,6 +247,9 @@ def test_extracts_cmip6_attributes(self, db_seeded): assert "dataset_type" not in attrs +# --- reconstruct_execution_definition tests --- + + class TestReconstructExecutionDefinition: def test_reconstruct_basic(self, config, reingest_db, reingest_execution_obj, provider): """Test that a basic execution definition can be reconstructed.""" @@ -242,67 +272,35 @@ def test_reconstruct_output_directory_under_scratch( assert str(definition.output_directory).startswith(str(config.paths.scratch)) -class TestDeleteExecutionMetricValues: - def test_deletes_metric_values_only(self, reingest_db, reingest_execution_obj): - """Should remove metric values but leave outputs untouched.""" - execution = reingest_execution_obj - - reingest_db.session.add( - ScalarMetricValue( - execution_id=execution.id, - value=42.0, - attributes={"test": True}, - ) - ) - reingest_db.session.add( - ExecutionOutput( - execution_id=execution.id, - output_type=ResultOutputType.Plot, - filename="test.png", - short_name="test", - ) - ) - reingest_db.session.commit() - - assert reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() == 1 - assert reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).count() == 1 - - _delete_execution_metric_values(reingest_db, execution) - reingest_db.session.commit() - - assert reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() == 0 - # Outputs are not deleted by this function (handled separately in _apply_reingest_mode) - assert reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).count() == 1 +# --- reingest_execution tests --- class TestReingestExecution: def test_reingest_missing_output_dir( self, config, reingest_db, reingest_execution_obj, mock_provider_registry ): - """Should return unsuccessful result when output directory doesn't exist.""" + """Should return False when scratch directory doesn't exist.""" result = reingest_execution( config=config, database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) assert result is False def test_reingest_unresolvable_diagnostic(self, config, reingest_db, reingest_execution_obj): - """Should return unsuccessful result when provider registry can't resolve diagnostic.""" + """Should return False when provider registry can't resolve diagnostic.""" empty_registry = ProviderRegistry(providers=[]) result = reingest_execution( config=config, database=reingest_db, execution=reingest_execution_obj, provider_registry=empty_registry, - mode=ReingestMode.replace, ) assert result is False @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_reingest_replace_mode( + def test_reingest_creates_new_execution( self, config, reingest_db, @@ -312,20 +310,9 @@ def test_reingest_replace_mode( mock_result_factory, mocker, ): - """Replace mode should delete existing values and re-ingest.""" - execution = reingest_execution_obj - - reingest_db.session.add( - ScalarMetricValue( - execution_id=execution.id, - value=99.0, - attributes={"old": True}, - ) - ) - reingest_db.session.commit() - - old_count = reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() - assert old_count == 1 + """Reingest should always create a new Execution record.""" + original_id = reingest_execution_obj.id + original_count = reingest_db.session.query(Execution).count() mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) @@ -333,22 +320,21 @@ def test_reingest_replace_mode( ok = reingest_execution( config=config, database=reingest_db, - execution=execution, + execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) reingest_db.session.commit() assert ok is True - old_vals = ( - reingest_db.session.query(ScalarMetricValue) - .filter_by(execution_id=execution.id) - .filter(ScalarMetricValue.value == 99.0) - .all() - ) - assert len(old_vals) == 0 + new_count = reingest_db.session.query(Execution).count() + assert new_count == original_count + 1 + + # Original execution should be untouched + original = reingest_db.session.get(Execution, original_id) + assert original is not None + assert original.successful is True @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_reingest_versioned_creates_new_execution( + def test_reingest_creates_unique_fragment( self, config, reingest_db, @@ -358,10 +344,7 @@ def test_reingest_versioned_creates_new_execution( mock_result_factory, mocker, ): - """Versioned mode should create a new Execution record.""" - original_id = reingest_execution_obj.id - original_count = reingest_db.session.query(Execution).count() - + """New execution should have a different output_fragment from the original.""" mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) @@ -370,45 +353,57 @@ def test_reingest_versioned_creates_new_execution( database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.versioned, ) reingest_db.session.commit() assert ok is True - new_count = reingest_db.session.query(Execution).count() - assert new_count == original_count + 1 - # Original execution should be untouched - original = reingest_db.session.get(Execution, original_id) - assert original is not None + all_executions = reingest_db.session.query(Execution).all() + fragments = [e.output_fragment for e in all_executions] + assert len(set(fragments)) == len(fragments), f"Expected unique fragments, got: {fragments}" - def test_reingest_build_execution_result_failure( + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_reingest_twice_creates_distinct_fragments( self, config, reingest_db, reingest_execution_obj, mock_provider_registry, scratch_dir_with_results, + mock_result_factory, mocker, ): - """Should return False and skip when build_execution_result raises.""" - mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") - mocker.patch.object( - mock_diagnostic, - "build_execution_result", - side_effect=RuntimeError("Extraction failed"), + """Running reingest twice should create distinct output fragments.""" + mock_result = mock_result_factory(scratch_dir_with_results) + _patch_build_result(mocker, mock_provider_registry, mock_result) + + ok1 = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, ) + reingest_db.session.commit() + assert ok1 is True - result = reingest_execution( + reingest_db.session.refresh(reingest_execution_obj) + ok2 = reingest_execution( config=config, database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) - assert result is False + reingest_db.session.commit() + assert ok2 is True + + # Should have 3 executions total: original + 2 reingested + all_executions = reingest_db.session.query(Execution).all() + assert len(all_executions) == 3 + + fragments = [e.output_fragment for e in all_executions] + assert len(set(fragments)) == 3, f"Expected unique fragments, got: {fragments}" @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_reingest_does_not_touch_dirty_flag( + def test_reingest_copies_results_to_new_directory( self, config, reingest_db, @@ -418,80 +413,54 @@ def test_reingest_does_not_touch_dirty_flag( mock_result_factory, mocker, ): - """The dirty flag should remain unchanged after reingest. - - Note: reingest_execution itself doesn't manage the dirty flag -- - that's the CLI's responsibility. This test verifies that reingest_execution - does not set dirty=False (unlike handle_execution_result). - """ - eg = reingest_execution_obj.execution_group - eg.dirty = True - reingest_db.session.commit() - - mock_result = mock_result_factory( - scratch_dir_with_results, output_bundle_filename=None, series_filename=None - ) + """Reingest should copy scratch tree to a new results directory.""" + mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) - reingest_execution( + ok = reingest_execution( config=config, database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) reingest_db.session.commit() - reingest_db.session.refresh(eg) - assert eg.dirty is True + assert ok is True - @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_additive_preserves_existing_values( + new_execution = ( + reingest_db.session.query(Execution).filter(Execution.id != reingest_execution_obj.id).one() + ) + + results_dir = config.paths.results / new_execution.output_fragment + assert results_dir.exists(), "Results should be copied to new directory" + assert (results_dir / "diagnostic.json").exists() + + def test_reingest_build_execution_result_failure( self, config, reingest_db, reingest_execution_obj, mock_provider_registry, scratch_dir_with_results, - mock_result_factory, mocker, ): - """Additive mode should keep pre-existing metric values untouched.""" - execution = reingest_execution_obj - - reingest_db.session.add( - ScalarMetricValue( - execution_id=execution.id, - value=42.0, - attributes={"pre_existing": True}, - source_id="pre-existing-model", - ) - ) - reingest_db.session.commit() - - mock_result = mock_result_factory( - scratch_dir_with_results, output_bundle_filename=None, series_filename=None + """Should return False and skip when build_execution_result raises.""" + mock_diagnostic = mock_provider_registry.get_metric("mock_provider", "mock") + mocker.patch.object( + mock_diagnostic, + "build_execution_result", + side_effect=RuntimeError("Extraction failed"), ) - _patch_build_result(mocker, mock_provider_registry, mock_result) - reingest_execution( + result = reingest_execution( config=config, database=reingest_db, - execution=execution, + execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.additive, ) - reingest_db.session.commit() - # The pre-existing value should still be there - pre_existing = ( - reingest_db.session.query(ScalarMetricValue) - .filter_by(execution_id=execution.id) - .filter(ScalarMetricValue.value == 42.0) - .all() - ) - assert len(pre_existing) == 1, "Additive mode should preserve pre-existing values" + assert result is False @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_additive_reingest_is_idempotent( + def test_reingest_does_not_touch_dirty_flag( self, config, reingest_db, @@ -501,53 +470,28 @@ def test_additive_reingest_is_idempotent( mock_result_factory, mocker, ): - """Running additive reingest twice should not create duplicate rows.""" - execution = reingest_execution_obj - - mock_result = mock_result_factory(scratch_dir_with_results) - _patch_build_result(mocker, mock_provider_registry, mock_result) - - # First reingest - reingest_execution( - config=config, - database=reingest_db, - execution=execution, - provider_registry=mock_provider_registry, - mode=ReingestMode.additive, - ) + """The dirty flag should remain unchanged after reingest.""" + eg = reingest_execution_obj.execution_group + eg.dirty = True reingest_db.session.commit() - count_after_first = ( - reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() - ) - series_count_first = ( - reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() + + mock_result = mock_result_factory( + scratch_dir_with_results, output_bundle_filename=None, series_filename=None ) + _patch_build_result(mocker, mock_provider_registry, mock_result) - # Second reingest (should be idempotent) reingest_execution( config=config, database=reingest_db, - execution=execution, + execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.additive, ) reingest_db.session.commit() - count_after_second = ( - reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() - ) - series_count_second = ( - reingest_db.session.query(MetricValue).filter_by(execution_id=execution.id).count() - ) - - assert count_after_first == count_after_second, ( - f"Additive reingest created duplicates: {count_after_first} -> {count_after_second}" - ) - assert series_count_first == series_count_second, ( - f"Additive reingest created duplicate series: {series_count_first} -> {series_count_second}" - ) + reingest_db.session.refresh(eg) + assert eg.dirty is True @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_replace_preserves_data_on_ingestion_failure( + def test_reingest_preserves_original_execution( self, config, reingest_db, @@ -557,13 +501,13 @@ def test_replace_preserves_data_on_ingestion_failure( mock_result_factory, mocker, ): - """If ingestion fails in replace mode, original data should be preserved.""" + """Original execution should remain untouched after reingest.""" execution = reingest_execution_obj reingest_db.session.add( ScalarMetricValue( execution_id=execution.id, - value=42.0, + value=99.0, attributes={"original": True}, ) ) @@ -572,36 +516,27 @@ def test_replace_preserves_data_on_ingestion_failure( original_count = ( reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() ) - assert original_count == 1 - mock_result = mock_result_factory( - scratch_dir_with_results, output_bundle_filename=None, series_filename=None - ) + mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) - # Make the scalar ingestion fail by corrupting the metric bundle - (scratch_dir_with_results / "diagnostic.json").write_text("not valid json") - ok = reingest_execution( config=config, database=reingest_db, execution=execution, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) reingest_db.session.commit() - assert ok is False + assert ok is True - # Original data should be preserved since the savepoint rolled back + # Original execution's values should be unchanged preserved_count = ( reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).count() ) - assert preserved_count == original_count, ( - f"Replace mode lost data on failure: {original_count} -> {preserved_count}" - ) + assert preserved_count == original_count, "Original execution values should be untouched" @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_versioned_reingest_twice_creates_unique_fragments( + def test_reingest_ingestion_failure_rolls_back( self, config, reingest_db, @@ -611,72 +546,29 @@ def test_versioned_reingest_twice_creates_unique_fragments( mock_result_factory, mocker, ): - """Running versioned reingest twice should create distinct output fragments.""" - mock_result = mock_result_factory(scratch_dir_with_results) - _patch_build_result(mocker, mock_provider_registry, mock_result) - - # First versioned reingest - ok1 = reingest_execution( - config=config, - database=reingest_db, - execution=reingest_execution_obj, - provider_registry=mock_provider_registry, - mode=ReingestMode.versioned, - ) - reingest_db.session.commit() - assert ok1 is True - - # Second versioned reingest - reingest_db.session.refresh(reingest_execution_obj) - ok2 = reingest_execution( - config=config, - database=reingest_db, - execution=reingest_execution_obj, - provider_registry=mock_provider_registry, - mode=ReingestMode.versioned, - ) - reingest_db.session.commit() - assert ok2 is True - - # Should have 3 executions total: original + 2 versioned - all_executions = reingest_db.session.query(Execution).all() - assert len(all_executions) == 3 - - fragments = [e.output_fragment for e in all_executions] - assert len(set(fragments)) == 3, f"Expected unique fragments, got: {fragments}" - - @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_reingest_marks_failed_execution_as_successful( - self, - config, - reingest_db, - reingest_execution_obj, - mock_provider_registry, - scratch_dir_with_results, - mock_result_factory, - mocker, - ): - """Reingest should mark a previously-failed execution as successful.""" - reingest_execution_obj.successful = False - reingest_db.session.commit() - assert reingest_execution_obj.successful is False + """If ingestion fails, no new execution should be created and results dir cleaned up.""" + original_count = reingest_db.session.query(Execution).count() mock_result = mock_result_factory( scratch_dir_with_results, output_bundle_filename=None, series_filename=None ) _patch_build_result(mocker, mock_provider_registry, mock_result) + # Make the scalar ingestion fail by corrupting the metric bundle + (scratch_dir_with_results / "diagnostic.json").write_text("not valid json") + ok = reingest_execution( config=config, database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) reingest_db.session.commit() - assert ok is True - reingest_db.session.refresh(reingest_execution_obj) - assert reingest_execution_obj.successful is True + assert ok is False + + # No new execution should have been created + new_count = reingest_db.session.query(Execution).count() + assert new_count == original_count, "Failed reingest should not create new execution" def test_scratch_directory_preserved_after_success( self, @@ -701,7 +593,6 @@ def test_scratch_directory_preserved_after_success( database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.additive, ) assert scratch_dir.exists(), "Scratch directory should be preserved after reingest" @@ -730,128 +621,13 @@ def test_scratch_directory_preserved_after_failure( database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) assert result is False assert scratch_dir.exists(), "Scratch directory should be preserved after failure" -class TestCopyWithBackup: - def test_copies_file(self, tmp_path): - """Should copy file to destination.""" - src = tmp_path / "src" / "file.json" - src.parent.mkdir() - src.write_text('{"key": "new"}') - dst = tmp_path / "dst" / "file.json" - - _copy_with_backup(src, dst) - - assert dst.exists() - assert dst.read_text() == '{"key": "new"}' - - def test_backs_up_existing_file(self, tmp_path): - """Should create .bak copy when overwriting an existing file.""" - src = tmp_path / "src" / "file.json" - src.parent.mkdir() - src.write_text('{"key": "new"}') - dst = tmp_path / "dst" / "file.json" - dst.parent.mkdir() - dst.write_text('{"key": "old"}') - - _copy_with_backup(src, dst) - - assert dst.read_text() == '{"key": "new"}' - backup = dst.with_suffix(".json.bak") - assert backup.exists() - assert backup.read_text() == '{"key": "old"}' - - def test_no_backup_when_no_existing(self, tmp_path): - """Should not create .bak when destination doesn't exist.""" - src = tmp_path / "src" / "file.json" - src.parent.mkdir() - src.write_text('{"key": "value"}') - dst = tmp_path / "dst" / "file.json" - - _copy_with_backup(src, dst) - - assert dst.exists() - assert not dst.with_suffix(".json.bak").exists() - - -class TestSyncReingestFilesToResults: - def test_non_versioned_copies_with_backup(self, config, reingest_execution_obj, scratch_dir_with_results): - """Non-versioned mode should copy CMEC bundles with backup of existing files.""" - fragment = reingest_execution_obj.output_fragment - results_dir = config.paths.results / fragment - results_dir.mkdir(parents=True, exist_ok=True) - (results_dir / "diagnostic.json").write_text('{"old": true}') - - mock_result = type( - "R", - (), - { - "metric_bundle_filename": pathlib.Path("diagnostic.json"), - "output_bundle_filename": pathlib.Path("output.json"), - "series_filename": pathlib.Path("series.json"), - }, - )() - - _sync_reingest_files_to_results(config, fragment, fragment, mock_result, ReingestMode.additive) - - assert (results_dir / "diagnostic.json").exists() - assert (results_dir / "diagnostic.json.bak").exists() - assert (results_dir / "diagnostic.json.bak").read_text() == '{"old": true}' - - def test_versioned_copies_entire_tree(self, config, reingest_execution_obj, scratch_dir_with_results): - """Versioned mode should copy entire scratch tree to new results directory.""" - src_fragment = reingest_execution_obj.output_fragment - dst_fragment = src_fragment + "_v123abc" - - mock_result = type( - "R", - (), - { - "metric_bundle_filename": pathlib.Path("diagnostic.json"), - "output_bundle_filename": None, - "series_filename": None, - }, - )() - - _sync_reingest_files_to_results( - config, src_fragment, dst_fragment, mock_result, ReingestMode.versioned - ) - - dst_dir = config.paths.results / dst_fragment - assert dst_dir.exists() - assert (dst_dir / "diagnostic.json").exists() - assert (dst_dir / "output.json").exists() - assert (dst_dir / "series.json").exists() - - -class TestGetExistingMetricDimensions: - def test_returns_empty_for_no_values(self, reingest_db, reingest_execution_obj): - """Should return empty set when no metric values exist.""" - result = _get_existing_metric_dimensions(reingest_db, reingest_execution_obj) - assert result == set() - - def test_returns_dimension_signatures(self, reingest_db, reingest_execution_obj): - """Should return sorted dimension tuples for existing metric values.""" - reingest_db.session.add( - ScalarMetricValue( - execution_id=reingest_execution_obj.id, - value=1.0, - attributes={}, - source_id="model-a", - ) - ) - reingest_db.session.commit() - - result = _get_existing_metric_dimensions(reingest_db, reingest_execution_obj) - assert len(result) == 1 - sig = next(iter(result)) - # Should contain source_id dimension - assert any(k == "source_id" and v == "model-a" for k, v in sig) +# --- register_execution_outputs tests --- class TestRegisterExecutionOutputs: @@ -864,14 +640,12 @@ def test_registers_outputs_in_db( bundle_path = scratch_dir_with_data / "output.json" cmec_output_bundle = CMECOutput.load_from_json(bundle_path) - results_base = config.paths.results / execution.output_fragment register_execution_outputs( reingest_db, execution, cmec_output_bundle.plots, output_type=ResultOutputType.Plot, - base_path=results_base, - fallback_path=scratch_dir_with_data, + base_path=scratch_dir_with_data, ) reingest_db.session.commit() @@ -880,6 +654,9 @@ def test_registers_outputs_in_db( assert any(o.short_name == "test_plot" for o in outputs) +# --- ingest metrics tests --- + + class TestIngestMetrics: @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") def test_ingest_scalar_values( @@ -902,39 +679,6 @@ def test_ingest_scalar_values( assert len(scalars) >= 1 assert scalars[0].value == 42.0 - @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_ingest_additive_skips_existing( - self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data, mock_result_factory - ): - """Additive mode should skip values with existing dimension signatures.""" - mock_result = mock_result_factory(scratch_dir_with_data) - cv = CV.load_from_file(config.paths.dimensions_cv) - - # First ingest - ingest_scalar_values( - database=reingest_db, result=mock_result, execution=reingest_execution_obj, cv=cv - ) - reingest_db.session.commit() - count_first = ( - reingest_db.session.query(MetricValue).filter_by(execution_id=reingest_execution_obj.id).count() - ) - - # Second ingest in additive mode should not duplicate - existing = _get_existing_metric_dimensions(reingest_db, reingest_execution_obj) - ingest_scalar_values( - database=reingest_db, - result=mock_result, - execution=reingest_execution_obj, - cv=cv, - existing=existing, - ) - reingest_db.session.commit() - count_second = ( - reingest_db.session.query(MetricValue).filter_by(execution_id=reingest_execution_obj.id).count() - ) - - assert count_first == count_second - class TestIngestMetricsWithSeries: @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") @@ -957,44 +701,8 @@ def test_ingest_series_values( ) assert len(series) >= 1 - @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_ingest_series_additive_skips_existing( - self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data, mock_result_factory - ): - """Additive mode should skip series with existing dimension signatures.""" - mock_result = mock_result_factory(scratch_dir_with_data) - cv = CV.load_from_file(config.paths.dimensions_cv) - - # First ingest - ingest_series_values( - database=reingest_db, result=mock_result, execution=reingest_execution_obj, cv=cv - ) - reingest_db.session.commit() - count_first = ( - reingest_db.session.query(SeriesMetricValue) - .filter_by(execution_id=reingest_execution_obj.id) - .count() - ) - - # Second ingest in additive mode - existing = _get_existing_metric_dimensions(reingest_db, reingest_execution_obj) - ingest_series_values( - database=reingest_db, - result=mock_result, - execution=reingest_execution_obj, - cv=cv, - existing=existing, - ) - reingest_db.session.commit() - - count_second = ( - reingest_db.session.query(SeriesMetricValue) - .filter_by(execution_id=reingest_execution_obj.id) - .count() - ) - - assert count_first == count_second +# --- reconstruct with datasets tests --- class TestReconstructEmptyDataset: @@ -1074,6 +782,9 @@ def test_reconstruct_with_linked_datasets(self, config, db_seeded, provider): assert SourceDatasetType.CMIP6 in definition.datasets +# --- unsuccessful result tests --- + + class TestReingestUnsuccessfulResult: def test_reingest_unsuccessful_build_result( self, @@ -1096,7 +807,6 @@ def test_reingest_unsuccessful_build_result( database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) assert ok is False @@ -1121,44 +831,11 @@ def test_reingest_successful_but_no_metric_bundle( database=reingest_db, execution=reingest_execution_obj, provider_registry=mock_provider_registry, - mode=ReingestMode.replace, ) assert ok is False -class TestHandleReingestOutputsScratchFallback: - def test_scratch_path_fallback(self, config, reingest_db, reingest_execution_obj): - """When output filename is absolute under scratch, should fall back to scratch base.""" - execution = reingest_execution_obj - - # Create a scratch directory with a plot file - scratch_base = config.paths.scratch / execution.output_fragment - scratch_base.mkdir(parents=True, exist_ok=True) - (scratch_base / "plot.png").write_bytes(b"fake png") - - # Build an OutputDict with an absolute path under scratch (not under results) - outputs = { - "test_plot": OutputDict( - filename=str(scratch_base / "plot.png"), - long_name="Test Plot", - description="A test plot", - ), - } - - results_base = config.paths.results / execution.output_fragment - register_execution_outputs( - reingest_db, - execution, - outputs, - output_type=ResultOutputType.Plot, - base_path=results_base, - fallback_path=scratch_base, - ) - reingest_db.session.commit() - - db_outputs = reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution.id).all() - assert len(db_outputs) == 1 - assert db_outputs[0].short_name == "test_plot" +# --- get_executions_for_reingest tests --- class TestGetExecutionsForReingest: @@ -1317,6 +994,9 @@ def test_filters_out_none_executions(self, db_seeded): assert len(results) == 0 +# --- equivalence tests --- + + def _snapshot_scalars(db, execution): """Snapshot scalar metrics as a set of (value, dimensions) for comparison.""" values = db.session.query(ScalarMetricValue).filter_by(execution_id=execution.id).all() @@ -1335,11 +1015,11 @@ def _snapshot_outputs(db, execution): return {(o.short_name, o.output_type, o.filename) for o in outputs} -class TestVersionedReingestEquivalence: - """Versioned reingest should produce the same DB state as fresh ingestion.""" +class TestReingestEquivalence: + """Reingest should produce the same DB state as fresh ingestion.""" @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") - def test_versioned_reingest_matches_original( + def test_reingest_matches_original( self, config, reingest_db, @@ -1349,7 +1029,7 @@ def test_versioned_reingest_matches_original( mock_result_factory, mocker, ): - """Versioned reingest should produce equivalent metrics and outputs to the original.""" + """Reingest should produce equivalent metrics and outputs to the original.""" execution = reingest_execution_obj mock_result = mock_result_factory(scratch_dir_with_data) _patch_build_result(mocker, mock_provider_registry, mock_result) @@ -1373,18 +1053,17 @@ def test_versioned_reingest_matches_original( assert original_scalars, "Original ingestion should produce scalar values" - # Versioned reingest creates a new execution from the same data + # Reingest creates a new execution from the same data ok = reingest_execution( config=config, database=reingest_db, execution=execution, provider_registry=mock_provider_registry, - mode=ReingestMode.versioned, ) reingest_db.session.commit() assert ok is True - # Find the new execution created by versioned reingest + # Find the new execution created by reingest new_execution = reingest_db.session.query(Execution).filter(Execution.id != execution.id).one() # Snapshot reingest DB state From 375ef6c45bfdc84b5bf96c1142f82d870c1a94d7 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sat, 4 Apr 2026 22:41:23 +0900 Subject: [PATCH 16/19] test: add coverage for fragment collision, dataset links, and execution state - Test build_execution_definition with existing_fragments/results_dir - Move fragment allocation into build_execution_definition (cleaner API) - Test reingest copies dataset links to new execution - Test new execution is marked successful with correct path - Test results dir cleaned up on ingestion failure - Test ingest_execution_result standalone with simplified signature --- .../tests/unit/executor/test_reingest.py | 272 ++++++++++++++++++ .../climate-ref/tests/unit/test_solver.py | 66 +++++ 2 files changed, 338 insertions(+) diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index d2ebeaa0e..ff7ac1eff 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -1081,3 +1081,275 @@ def test_reingest_matches_original( assert original_outputs == reingest_outputs, ( f"Output entries differ: original={original_outputs}, reingest={reingest_outputs}" ) + + +class TestReingestDatasetLinks: + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_reingest_copies_dataset_links( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + scratch_dir_with_results, + mock_result_factory, + mocker, + ): + """New execution should have the same dataset links as the original.""" + execution = reingest_execution_obj + + # Create a dataset to link to the execution + dataset = CMIP6Dataset( + slug="test-dataset.tas.gn", + instance_id="CMIP6.test.tas", + variable_id="tas", + source_id="ACCESS-ESM1-5", + experiment_id="historical", + table_id="Amon", + grid_label="gn", + member_id="r1i1p1f1", + variant_label="r1i1p1f1", + version="v20200101", + activity_id="CMIP", + institution_id="CSIRO", + ) + reingest_db.session.add(dataset) + reingest_db.session.flush() + + reingest_db.session.execute( + execution_datasets.insert().values( + execution_id=execution.id, + dataset_id=dataset.id, + ) + ) + reingest_db.session.commit() + + original_dataset_ids = sorted(d.id for d in execution.datasets) + assert len(original_dataset_ids) >= 1 + + mock_result = mock_result_factory( + scratch_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=execution, + provider_registry=mock_provider_registry, + ) + reingest_db.session.commit() + assert ok is True + + new_execution = reingest_db.session.query(Execution).filter(Execution.id != execution.id).one() + new_dataset_ids = sorted(d.id for d in new_execution.datasets) + assert original_dataset_ids == new_dataset_ids, ( + f"Dataset links should be copied: original={original_dataset_ids}, new={new_dataset_ids}" + ) + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_reingest_with_no_datasets( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + scratch_dir_with_results, + mock_result_factory, + mocker, + ): + """Reingest should succeed even when original execution has no dataset links.""" + assert len(reingest_execution_obj.datasets) == 0 + + mock_result = mock_result_factory( + scratch_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + ) + reingest_db.session.commit() + assert ok is True + + new_execution = ( + reingest_db.session.query(Execution).filter(Execution.id != reingest_execution_obj.id).one() + ) + assert len(new_execution.datasets) == 0 + + +class TestReingestExecutionState: + """Verify the state of the new execution record after reingest.""" + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_new_execution_marked_successful_with_correct_path( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + scratch_dir_with_results, + mock_result_factory, + mocker, + ): + """New execution should be marked successful with the correct metric bundle path.""" + mock_result = mock_result_factory( + scratch_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + ) + reingest_db.session.commit() + assert ok is True + + new_execution = ( + reingest_db.session.query(Execution).filter(Execution.id != reingest_execution_obj.id).one() + ) + assert new_execution.successful is True + assert new_execution.path is not None + assert "diagnostic.json" in new_execution.path + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_new_execution_belongs_to_same_group( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + scratch_dir_with_results, + mock_result_factory, + mocker, + ): + """New execution should belong to the same execution group as the original.""" + mock_result = mock_result_factory( + scratch_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + ) + reingest_db.session.commit() + assert ok is True + + new_execution = ( + reingest_db.session.query(Execution).filter(Execution.id != reingest_execution_obj.id).one() + ) + assert new_execution.execution_group_id == reingest_execution_obj.execution_group_id + assert new_execution.dataset_hash == reingest_execution_obj.dataset_hash + + +class TestReingestFailureCleanup: + """Verify that failed reingest cleans up the results directory.""" + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_results_dir_cleaned_on_ingestion_failure( + self, + config, + reingest_db, + reingest_execution_obj, + mock_provider_registry, + scratch_dir_with_results, + mock_result_factory, + mocker, + ): + """If ingestion fails, the copied results directory should be removed.""" + mock_result = mock_result_factory( + scratch_dir_with_results, output_bundle_filename=None, series_filename=None + ) + _patch_build_result(mocker, mock_provider_registry, mock_result) + + # Corrupt the metric bundle so ingestion fails + (scratch_dir_with_results / "diagnostic.json").write_text("not valid json") + + ok = reingest_execution( + config=config, + database=reingest_db, + execution=reingest_execution_obj, + provider_registry=mock_provider_registry, + ) + reingest_db.session.commit() + assert ok is False + + # No versioned results directory should remain + # The original fragment dir may or may not exist, but no _v2 dir should exist + versioned_dir = config.paths.results / (reingest_execution_obj.output_fragment + "_v2") + assert not versioned_dir.exists(), "Failed reingest should clean up results directory" + + +# --- ingest_execution_result standalone tests --- + + +class TestIngestExecutionResultStandalone: + """Test ingest_execution_result with the simplified signature.""" + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_ingest_with_all_outputs( + self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data, mock_result_factory + ): + """Should ingest scalars, series, and register outputs in one call.""" + mock_result = mock_result_factory(scratch_dir_with_data) + cv = CV.load_from_file(config.paths.dimensions_cv) + + ingest_execution_result( + reingest_db, + reingest_execution_obj, + mock_result, + cv, + output_base_path=scratch_dir_with_data, + ) + reingest_db.session.commit() + + execution_id = reingest_execution_obj.id + + scalars = reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution_id).all() + assert len(scalars) >= 1, "Should have ingested scalar values" + assert scalars[0].value == 42.0 + + series = reingest_db.session.query(SeriesMetricValue).filter_by(execution_id=execution_id).all() + assert len(series) >= 1, "Should have ingested series values" + + outputs = reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution_id).all() + assert len(outputs) >= 1, "Should have registered outputs" + assert any(o.short_name == "test_plot" for o in outputs) + + @pytest.mark.filterwarnings("ignore:Unknown dimension values.*:UserWarning") + def test_ingest_without_optional_outputs( + self, config, reingest_db, reingest_execution_obj, scratch_dir_with_data, mock_result_factory + ): + """Should work with no output_bundle and no series.""" + mock_result = mock_result_factory( + scratch_dir_with_data, output_bundle_filename=None, series_filename=None + ) + cv = CV.load_from_file(config.paths.dimensions_cv) + + ingest_execution_result( + reingest_db, + reingest_execution_obj, + mock_result, + cv, + output_base_path=scratch_dir_with_data, + ) + reingest_db.session.commit() + + execution_id = reingest_execution_obj.id + + scalars = reingest_db.session.query(ScalarMetricValue).filter_by(execution_id=execution_id).all() + assert len(scalars) >= 1, "Should still ingest scalar values" + + series = reingest_db.session.query(SeriesMetricValue).filter_by(execution_id=execution_id).all() + assert len(series) == 0, "Should have no series values" + + outputs = reingest_db.session.query(ExecutionOutput).filter_by(execution_id=execution_id).all() + assert len(outputs) == 0, "Should have no registered outputs" diff --git a/packages/climate-ref/tests/unit/test_solver.py b/packages/climate-ref/tests/unit/test_solver.py index c56356593..48c5686b7 100644 --- a/packages/climate-ref/tests/unit/test_solver.py +++ b/packages/climate-ref/tests/unit/test_solver.py @@ -1283,6 +1283,72 @@ def test_diagnostic_execution_build_definition(mock_diagnostic, provider, tmp_pa assert str(tmp_path.resolve()) in str(definition.output_directory) +def test_build_definition_with_fragment_collision(mock_diagnostic, provider, tmp_path): + data_catalog = { + SourceDatasetType.CMIP6: pd.DataFrame( + { + "variable_id": ["tas"], + "experiment_id": ["historical"], + "variant_label": ["r1i1p1f1"], + "instance_id": ["CMIP6.test.tas"], + } + ), + } + mock_diagnostic.data_requirements = ( + DataRequirement( + source_type=SourceDatasetType.CMIP6, + filters=(FacetFilter(facets={"variable_id": "tas"}),), + group_by=("variable_id",), + ), + ) + executions = list(solve_executions(data_catalog, mock_diagnostic, provider)) + assert len(executions) == 1 + + # First definition: no collision + definition = executions[0].build_execution_definition(output_root=tmp_path) + base_fragment = str(definition.output_fragment()) + + # Second definition: simulate collision by passing existing_fragments + definition_v2 = executions[0].build_execution_definition( + output_root=tmp_path, + existing_fragments={base_fragment}, + results_dir=tmp_path, + ) + v2_fragment = str(definition_v2.output_fragment()) + + assert v2_fragment != base_fragment, "Should allocate a different fragment on collision" + assert v2_fragment == f"{base_fragment}_v2", f"Expected _v2 suffix, got {v2_fragment}" + assert str(tmp_path.resolve()) in str(definition_v2.output_directory) + + +def test_build_definition_no_collision_params_skips_allocation(mock_diagnostic, provider, tmp_path): + data_catalog = { + SourceDatasetType.CMIP6: pd.DataFrame( + { + "variable_id": ["tas"], + "experiment_id": ["historical"], + "variant_label": ["r1i1p1f1"], + "instance_id": ["CMIP6.test.tas"], + } + ), + } + mock_diagnostic.data_requirements = ( + DataRequirement( + source_type=SourceDatasetType.CMIP6, + filters=(FacetFilter(facets={"variable_id": "tas"}),), + group_by=("variable_id",), + ), + ) + executions = list(solve_executions(data_catalog, mock_diagnostic, provider)) + + # Without collision params, should return natural fragment + def1 = executions[0].build_execution_definition(output_root=tmp_path) + def2 = executions[0].build_execution_definition( + output_root=tmp_path, existing_fragments=None, results_dir=None + ) + assert str(def1.output_fragment()) == str(def2.output_fragment()) + + def test_diagnostic_execution_selectors(mock_diagnostic, provider): """Test DiagnosticExecution.selectors property.""" data_catalog = { From 0480279ab0040e1e67c40401d8af38d54fab5265 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sun, 5 Apr 2026 07:53:50 +0900 Subject: [PATCH 17/19] fix(solver): build definition early to avoid MagicMock key in tests The definition must be built before get_or_create to provide definition.key and definition.datasets.hash. Fragment collision info is applied by rebuilding the definition only when an execution is actually created. --- packages/climate-ref/src/climate_ref/solver.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/solver.py b/packages/climate-ref/src/climate_ref/solver.py index 6822529ce..909866e7e 100644 --- a/packages/climate-ref/src/climate_ref/solver.py +++ b/packages/climate-ref/src/climate_ref/solver.py @@ -578,8 +578,11 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 total_count = 0 for potential_execution in solver.solve(filters): + # The diagnostic output is first written to the scratch directory + definition = potential_execution.build_execution_definition(output_root=config.paths.scratch) + logger.debug( - f"Identified candidate execution {potential_execution.dataset_key} " + f"Identified candidate execution {definition.key} " f"for {potential_execution.diagnostic.full_slug()}" ) @@ -602,7 +605,7 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 ) execution_group, created = db.get_or_create( ExecutionGroup, - key=potential_execution.dataset_key, + key=definition.key, diagnostic_id=diagnostic.id, defaults={ "selectors": potential_execution.selectors, @@ -626,7 +629,7 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 f"provider_count={provider_count}" ) - if execution_group.should_run(potential_execution.datasets.hash, rerun_failed=rerun_failed): + if execution_group.should_run(definition.datasets.hash, rerun_failed=rerun_failed): if (one_per_provider or one_per_diagnostic) and one_of_check_failed: logger.info( f"Skipping execution due to one-of check: {potential_execution.execution_slug()!r}" @@ -642,7 +645,7 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 break continue - # Build the definition now that we know fragment collision info + # Rebuild the definition with fragment collision info existing_fragments = {e.output_fragment for e in execution_group.executions} definition = potential_execution.build_execution_definition( output_root=config.paths.scratch, From b1eaba3d21e26794b6abd8b19fa34fc9cd53df4a Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sun, 5 Apr 2026 20:51:44 +0900 Subject: [PATCH 18/19] refactor(fragment): replace collision-checking with timestamp-based output fragments Use a UTC timestamp suffix instead of scanning the database and filesystem for collisions. This simplifies allocate_output_fragment, removes the collision parameters from build_execution_definition, and drops the redundant definition rebuild in solve_required_executions. --- .../src/climate_ref/executor/fragment.py | 34 +++------- .../src/climate_ref/executor/reingest.py | 7 +- .../climate-ref/src/climate_ref/solver.py | 38 +---------- .../tests/unit/executor/test_reingest.py | 66 +++++++++---------- .../climate-ref/tests/unit/test_solver.py | 66 ------------------- 5 files changed, 43 insertions(+), 168 deletions(-) diff --git a/packages/climate-ref/src/climate_ref/executor/fragment.py b/packages/climate-ref/src/climate_ref/executor/fragment.py index 5251ff804..0af808a72 100644 --- a/packages/climate-ref/src/climate_ref/executor/fragment.py +++ b/packages/climate-ref/src/climate_ref/executor/fragment.py @@ -2,43 +2,25 @@ Helpers for allocating non-colliding output fragment paths. """ -import pathlib +import datetime -def allocate_output_fragment( - base_fragment: str, - existing_fragments: set[str], - results_dir: pathlib.Path, -) -> str: +def allocate_output_fragment(base_fragment: str) -> str: """ - Return a non-colliding output fragment path. + Return a unique output fragment by appending a UTC timestamp. - If *base_fragment* is not already used (neither in *existing_fragments* nor - on disk under *results_dir*), it is returned unchanged. Otherwise ``_v2``, - ``_v3``, ... suffixes are tried until a free slot is found. + The returned fragment is ``{base_fragment}_{YYYYMMDDTHHMMSS}``, which is + practically unique without needing any database or filesystem lookups. Parameters ---------- base_fragment The natural fragment, e.g. ``provider/diagnostic/dataset_hash`` - existing_fragments - Set of ``output_fragment`` values already recorded in the database - for the relevant execution group - results_dir - Root results directory; used to check for orphaned directories on disk Returns ------- : - A fragment string guaranteed not to collide with *existing_fragments* - or any directory under *results_dir* + A new fragment with a timestamp suffix """ - if base_fragment not in existing_fragments and not (results_dir / base_fragment).exists(): - return base_fragment - - version = 2 - while True: - candidate = f"{base_fragment}_v{version}" - if candidate not in existing_fragments and not (results_dir / candidate).exists(): - return candidate - version += 1 + now = datetime.datetime.now(tz=datetime.timezone.utc) + return f"{base_fragment}_{now.strftime('%Y%m%dT%H%M%S')}" diff --git a/packages/climate-ref/src/climate_ref/executor/reingest.py b/packages/climate-ref/src/climate_ref/executor/reingest.py index d9b1588d3..3455fa52d 100644 --- a/packages/climate-ref/src/climate_ref/executor/reingest.py +++ b/packages/climate-ref/src/climate_ref/executor/reingest.py @@ -217,11 +217,8 @@ def reingest_execution( ) return False - # Allocate a new, non-colliding output fragment - existing_fragments = {e.output_fragment for e in execution_group.executions} - new_fragment = allocate_output_fragment( - execution.output_fragment, existing_fragments, config.paths.results - ) + # Allocate a new output fragment with a timestamp suffix + new_fragment = allocate_output_fragment(execution.output_fragment) # Copy scratch tree to the new results location dst_dir = config.paths.results / new_fragment diff --git a/packages/climate-ref/src/climate_ref/solver.py b/packages/climate-ref/src/climate_ref/solver.py index 909866e7e..f8052843e 100644 --- a/packages/climate-ref/src/climate_ref/solver.py +++ b/packages/climate-ref/src/climate_ref/solver.py @@ -23,7 +23,6 @@ PMPClimatologyDatasetAdapter, get_slug_column, ) -from climate_ref.executor.fragment import allocate_output_fragment from climate_ref.models import Diagnostic as DiagnosticModel from climate_ref.models import ExecutionGroup from climate_ref.models import Provider as ProviderModel @@ -96,39 +95,15 @@ def selectors(self) -> dict[str, Selector]: """ return self.datasets.selectors - def build_execution_definition( - self, - output_root: pathlib.Path, - *, - existing_fragments: set[str] | None = None, - results_dir: pathlib.Path | None = None, - ) -> ExecutionDefinition: + def build_execution_definition(self, output_root: pathlib.Path) -> ExecutionDefinition: """ Build the execution definition for the current diagnostic execution - - Parameters - ---------- - output_root - Root directory for storing execution output (typically scratch) - existing_fragments - Output fragments already recorded in the database for this - execution group. When provided together with *results_dir*, - the fragment is passed through - :func:`~climate_ref.executor.fragment.allocate_output_fragment` - to avoid collisions. - results_dir - Root results directory, used for on-disk collision checks """ # Ensure that the output root is always an absolute path output_root = output_root.resolve() # This is the desired path relative to the output directory - base_fragment = str(pathlib.Path() / self.provider.slug / self.diagnostic.slug / self.datasets.hash) - - if existing_fragments is not None and results_dir is not None: - fragment = allocate_output_fragment(base_fragment, existing_fragments, results_dir) - else: - fragment = base_fragment + fragment = pathlib.Path() / self.provider.slug / self.diagnostic.slug / self.datasets.hash return ExecutionDefinition( diagnostic=self.diagnostic, @@ -578,7 +553,6 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 total_count = 0 for potential_execution in solver.solve(filters): - # The diagnostic output is first written to the scratch directory definition = potential_execution.build_execution_definition(output_root=config.paths.scratch) logger.debug( @@ -645,14 +619,6 @@ def solve_required_executions( # noqa: PLR0912, PLR0913, PLR0915 break continue - # Rebuild the definition with fragment collision info - existing_fragments = {e.output_fragment for e in execution_group.executions} - definition = potential_execution.build_execution_definition( - output_root=config.paths.scratch, - existing_fragments=existing_fragments, - results_dir=config.paths.results, - ) - logger.info( f"Running new execution for execution group: {potential_execution.execution_slug()!r}" ) diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index ff7ac1eff..6ec65945c 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -1,7 +1,9 @@ """Tests for the reingest module and allocate_output_fragment helper.""" +import datetime import json import pathlib +import time import pytest from climate_ref_esmvaltool import provider as esmvaltool_provider @@ -195,41 +197,27 @@ def _patch_build_result(mocker, registry, mock_result): class TestAllocateOutputFragment: - def test_returns_base_when_no_collision(self, tmp_path): - """Should return base_fragment unchanged when no collision exists.""" - result = allocate_output_fragment("provider/diag/abc123", set(), tmp_path) - assert result == "provider/diag/abc123" - - def test_appends_v2_on_db_collision(self, tmp_path): - """Should append _v2 when base_fragment exists in DB.""" - existing = {"provider/diag/abc123"} - result = allocate_output_fragment("provider/diag/abc123", existing, tmp_path) - assert result == "provider/diag/abc123_v2" - - def test_appends_v2_on_disk_collision(self, tmp_path): - """Should append _v2 when base_fragment directory exists on disk.""" - (tmp_path / "provider/diag/abc123").mkdir(parents=True) - result = allocate_output_fragment("provider/diag/abc123", set(), tmp_path) - assert result == "provider/diag/abc123_v2" - - def test_increments_version_on_multiple_collisions(self, tmp_path): - """Should increment to _v3 when both base and _v2 are taken.""" - existing = {"provider/diag/abc123", "provider/diag/abc123_v2"} - result = allocate_output_fragment("provider/diag/abc123", existing, tmp_path) - assert result == "provider/diag/abc123_v3" - - def test_mixed_db_and_disk_collisions(self, tmp_path): - """Should handle collisions from both DB and disk.""" - existing = {"provider/diag/abc123"} - (tmp_path / "provider/diag/abc123_v2").mkdir(parents=True) - result = allocate_output_fragment("provider/diag/abc123", existing, tmp_path) - assert result == "provider/diag/abc123_v3" - - def test_no_collision_returns_base_with_existing_other_fragments(self, tmp_path): - """Other fragments in DB should not cause collision.""" - existing = {"provider/diag/xyz789"} - result = allocate_output_fragment("provider/diag/abc123", existing, tmp_path) - assert result == "provider/diag/abc123" + def test_appends_timestamp_suffix(self): + """Should append a UTC timestamp suffix to the base fragment.""" + result = allocate_output_fragment("provider/diag/abc123") + assert result.startswith("provider/diag/abc123_") + # Suffix should be a valid timestamp: YYYYMMDDTHHMMSS + suffix = result.split("_", 1)[1] + assert len(suffix) == 15 # 8 date + T + 6 time + assert "T" in suffix + + def test_preserves_base_fragment(self): + """The original fragment should be a prefix of the result.""" + base = "my_provider/my_diag/hash123" + result = allocate_output_fragment(base) + assert result.startswith(base + "_") + + def test_different_calls_produce_different_fragments(self): + """Two calls at least a second apart should produce different fragments.""" + result1 = allocate_output_fragment("provider/diag/abc123") + time.sleep(1.1) + result2 = allocate_output_fragment("provider/diag/abc123") + assert result1 != result2 # --- extract dataset attributes tests --- @@ -376,6 +364,14 @@ def test_reingest_twice_creates_distinct_fragments( mock_result = mock_result_factory(scratch_dir_with_results) _patch_build_result(mocker, mock_provider_registry, mock_result) + # Mock datetime.datetime.now to return different timestamps for each call + t1 = datetime.datetime(2026, 1, 1, 12, 0, 0, tzinfo=datetime.timezone.utc) + t2 = datetime.datetime(2026, 1, 1, 12, 0, 1, tzinfo=datetime.timezone.utc) + mocker.patch( + "climate_ref.executor.fragment.datetime.datetime", + **{"now.side_effect": [t1, t2]}, + ) + ok1 = reingest_execution( config=config, database=reingest_db, diff --git a/packages/climate-ref/tests/unit/test_solver.py b/packages/climate-ref/tests/unit/test_solver.py index 48c5686b7..c56356593 100644 --- a/packages/climate-ref/tests/unit/test_solver.py +++ b/packages/climate-ref/tests/unit/test_solver.py @@ -1283,72 +1283,6 @@ def test_diagnostic_execution_build_definition(mock_diagnostic, provider, tmp_pa assert str(tmp_path.resolve()) in str(definition.output_directory) -def test_build_definition_with_fragment_collision(mock_diagnostic, provider, tmp_path): - data_catalog = { - SourceDatasetType.CMIP6: pd.DataFrame( - { - "variable_id": ["tas"], - "experiment_id": ["historical"], - "variant_label": ["r1i1p1f1"], - "instance_id": ["CMIP6.test.tas"], - } - ), - } - mock_diagnostic.data_requirements = ( - DataRequirement( - source_type=SourceDatasetType.CMIP6, - filters=(FacetFilter(facets={"variable_id": "tas"}),), - group_by=("variable_id",), - ), - ) - executions = list(solve_executions(data_catalog, mock_diagnostic, provider)) - assert len(executions) == 1 - - # First definition: no collision - definition = executions[0].build_execution_definition(output_root=tmp_path) - base_fragment = str(definition.output_fragment()) - - # Second definition: simulate collision by passing existing_fragments - definition_v2 = executions[0].build_execution_definition( - output_root=tmp_path, - existing_fragments={base_fragment}, - results_dir=tmp_path, - ) - v2_fragment = str(definition_v2.output_fragment()) - - assert v2_fragment != base_fragment, "Should allocate a different fragment on collision" - assert v2_fragment == f"{base_fragment}_v2", f"Expected _v2 suffix, got {v2_fragment}" - assert str(tmp_path.resolve()) in str(definition_v2.output_directory) - - -def test_build_definition_no_collision_params_skips_allocation(mock_diagnostic, provider, tmp_path): - data_catalog = { - SourceDatasetType.CMIP6: pd.DataFrame( - { - "variable_id": ["tas"], - "experiment_id": ["historical"], - "variant_label": ["r1i1p1f1"], - "instance_id": ["CMIP6.test.tas"], - } - ), - } - mock_diagnostic.data_requirements = ( - DataRequirement( - source_type=SourceDatasetType.CMIP6, - filters=(FacetFilter(facets={"variable_id": "tas"}),), - group_by=("variable_id",), - ), - ) - executions = list(solve_executions(data_catalog, mock_diagnostic, provider)) - - # Without collision params, should return natural fragment - def1 = executions[0].build_execution_definition(output_root=tmp_path) - def2 = executions[0].build_execution_definition( - output_root=tmp_path, existing_fragments=None, results_dir=None - ) - assert str(def1.output_fragment()) == str(def2.output_fragment()) - - def test_diagnostic_execution_selectors(mock_diagnostic, provider): """Test DiagnosticExecution.selectors property.""" data_catalog = { From fed124cab07f10ee28db15e2dc1d7606da816fe3 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sun, 5 Apr 2026 21:29:37 +0900 Subject: [PATCH 19/19] fix: address PR review comments for reingest - Use .resolve().is_relative_to() for path traversal guards (symlink-safe) - Add microsecond resolution to allocate_output_fragment timestamps - allocate_output_fragment now raises FileExistsError on collision - Use scratch dir as output_base_path for ingest_execution_result - Update changelog to match implemented behavior (versioned-only) - Extract _build_execution_result helper to reduce return count --- changelog/610.feature.md | 2 +- .../src/climate_ref/executor/fragment.py | 24 +++- .../src/climate_ref/executor/reingest.py | 125 ++++++++++++------ .../tests/unit/executor/test_reingest.py | 36 +++-- 4 files changed, 128 insertions(+), 59 deletions(-) diff --git a/changelog/610.feature.md b/changelog/610.feature.md index b2c759346..9b3b14e27 100644 --- a/changelog/610.feature.md +++ b/changelog/610.feature.md @@ -1 +1 @@ -Added `ref executions reingest` command to re-ingest existing execution results without re-running diagnostics. Supports three modes: `additive` (keep existing values, add new), `replace` (delete and re-ingest), and `versioned` (create new execution record). +Added `ref executions reingest` command to re-ingest existing execution results without re-running diagnostics. Creates a new immutable execution record with a timestamped output fragment, leaving the original execution untouched. diff --git a/packages/climate-ref/src/climate_ref/executor/fragment.py b/packages/climate-ref/src/climate_ref/executor/fragment.py index 0af808a72..86a31965c 100644 --- a/packages/climate-ref/src/climate_ref/executor/fragment.py +++ b/packages/climate-ref/src/climate_ref/executor/fragment.py @@ -3,24 +3,42 @@ """ import datetime +from pathlib import Path -def allocate_output_fragment(base_fragment: str) -> str: +def allocate_output_fragment(base_fragment: str, results_dir: Path) -> str: """ Return a unique output fragment by appending a UTC timestamp. - The returned fragment is ``{base_fragment}_{YYYYMMDDTHHMMSS}``, which is + The returned fragment is ``{base_fragment}_{YYYYMMDDTHHMMSSffffff}``, which is practically unique without needing any database or filesystem lookups. + Microsecond resolution avoids collisions from rapid successive calls. Parameters ---------- base_fragment The natural fragment, e.g. ``provider/diagnostic/dataset_hash`` + results_dir + The results root directory. Used to verify the allocated fragment + does not already exist on disk. Returns ------- : A new fragment with a timestamp suffix + + Raises + ------ + FileExistsError + If the computed output directory already exists under *results_dir* """ now = datetime.datetime.now(tz=datetime.timezone.utc) - return f"{base_fragment}_{now.strftime('%Y%m%dT%H%M%S')}" + fragment = f"{base_fragment}_{now.strftime('%Y%m%dT%H%M%S%f')}" + + target = results_dir / fragment + if target.exists(): + raise FileExistsError( + f"Output directory already exists: {target}. Cannot allocate fragment '{fragment}'." + ) + + return fragment diff --git a/packages/climate-ref/src/climate_ref/executor/reingest.py b/packages/climate-ref/src/climate_ref/executor/reingest.py index 3455fa52d..dc49fd118 100644 --- a/packages/climate-ref/src/climate_ref/executor/reingest.py +++ b/packages/climate-ref/src/climate_ref/executor/reingest.py @@ -37,11 +37,13 @@ from climate_ref_core.pycmec.controlled_vocabulary import CV if TYPE_CHECKING: + from pathlib import Path + from climate_ref.config import Config from climate_ref.database import Database from climate_ref.models.dataset import Dataset from climate_ref.provider_registry import ProviderRegistry - from climate_ref_core.diagnostics import Diagnostic + from climate_ref_core.diagnostics import Diagnostic, ExecutionResult def reconstruct_execution_definition( @@ -149,37 +151,29 @@ def _extract_dataset_attributes(dataset: "Dataset") -> dict[str, object]: return attrs -def reingest_execution( - config: "Config", - database: "Database", - execution: Execution, - provider_registry: "ProviderRegistry", -) -> bool: +def _validate_path_containment(path: "Path", base: "Path", label: str) -> None: """ - Reingest an existing execution. + Check that *path* stays within *base* after resolving symlinks and ``..`` segments. - Re-runs ``build_execution_result()`` against the scratch directory - (which contains the raw outputs from the original diagnostic run), - creates a new ``Execution`` record with a unique output fragment, - copies results to the new location, and ingests metrics into the database. + Raises + ------ + ValueError + If the resolved *path* escapes *base* + """ + if not path.resolve().is_relative_to(base.resolve()): + msg = f"Computed {label} path {path} escapes {base}." + raise ValueError(msg) - The original execution is left untouched. - Parameters - ---------- - config - Application configuration - database - Database instance - execution - The ``Execution`` record to reingest - provider_registry - Registry of active providers (used to resolve the live diagnostic) +def _build_execution_result( + config: "Config", + execution: Execution, + provider_registry: "ProviderRegistry", +) -> "tuple[ExecutionResult, Path] | None": + """ + Resolve the diagnostic, validate paths, and build the execution result. - Returns - ------- - : - True if reingest was successful, False if it was skipped due to an error + Returns the result and scratch directory on success, or None on failure. """ execution_group = execution.execution_group diagnostic_model = execution_group.diagnostic @@ -193,12 +187,17 @@ def reingest_execution( f"Could not resolve diagnostic {provider_slug}/{diagnostic_slug} " f"from provider registry. Skipping execution {execution.id}." ) - return False + return None scratch_dir = config.paths.scratch / execution.output_fragment + try: + _validate_path_containment(scratch_dir, config.paths.scratch, "scratch") + except ValueError: + logger.error(f"Skipping execution {execution.id}: scratch path escapes base.") + return None if not scratch_dir.exists(): logger.error(f"Scratch directory does not exist: {scratch_dir}. Skipping execution {execution.id}.") - return False + return None definition = reconstruct_execution_definition(config, execution, diagnostic) @@ -209,22 +208,67 @@ def reingest_execution( f"build_execution_result failed for execution {execution.id} " f"({provider_slug}/{diagnostic_slug}). Skipping." ) - return False + return None if not result.successful or result.metric_bundle_filename is None: logger.warning( f"build_execution_result returned unsuccessful result for execution {execution.id}. Skipping." ) + return None + + return result, scratch_dir + + +def reingest_execution( + config: "Config", + database: "Database", + execution: Execution, + provider_registry: "ProviderRegistry", +) -> bool: + """ + Reingest an existing execution. + + Re-runs ``build_execution_result()`` against the scratch directory + (which contains the raw outputs from the original diagnostic run), + creates a new ``Execution`` record with a unique output fragment, + copies results to the new location, and ingests metrics into the database. + + The original execution is left untouched. + + Parameters + ---------- + config + Application configuration + database + Database instance + execution + The ``Execution`` record to reingest + provider_registry + Registry of active providers (used to resolve the live diagnostic) + + Returns + ------- + : + True if reingest was successful, False if it was skipped due to an error + """ + built = _build_execution_result(config, execution, provider_registry) + if built is None: return False + result, scratch_dir = built + + execution_group = execution.execution_group # Allocate a new output fragment with a timestamp suffix - new_fragment = allocate_output_fragment(execution.output_fragment) + new_fragment = allocate_output_fragment(execution.output_fragment, config.paths.results) # Copy scratch tree to the new results location dst_dir = config.paths.results / new_fragment + try: + _validate_path_containment(dst_dir, config.paths.results, "results") + except ValueError: + logger.error(f"Skipping execution {execution.id}: results path escapes base.") + return False dst_dir.parent.mkdir(parents=True, exist_ok=True) - if dst_dir.exists(): - shutil.rmtree(dst_dir) shutil.copytree(scratch_dir, dst_dir) cv = CV.load_from_file(config.paths.dimensions_cv) @@ -249,31 +293,26 @@ def reingest_execution( ) ) - # Standard ingestion (same path as fresh executions) + # Use scratch dir as the output base path since build_execution_result + # may produce absolute paths under scratch in output bundles ingest_execution_result( database, new_execution, result, cv, - output_base_path=config.paths.results / new_fragment, + output_base_path=scratch_dir, ) assert result.metric_bundle_filename is not None new_execution.mark_successful(result.as_relative_path(result.metric_bundle_filename)) except Exception: - logger.exception( - f"Ingestion failed for execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}). Rolling back changes." - ) + logger.exception(f"Ingestion failed for execution {execution.id}. Rolling back changes.") # Clean up the copied directory on failure if dst_dir.exists(): shutil.rmtree(dst_dir) return False - logger.info( - f"Successfully reingested execution {execution.id} " - f"({provider_slug}/{diagnostic_slug}) -> new execution {new_execution.id}" - ) + logger.info(f"Successfully reingested execution {execution.id} -> new execution {new_execution.id}") return True diff --git a/packages/climate-ref/tests/unit/executor/test_reingest.py b/packages/climate-ref/tests/unit/executor/test_reingest.py index 6ec65945c..af6d4615d 100644 --- a/packages/climate-ref/tests/unit/executor/test_reingest.py +++ b/packages/climate-ref/tests/unit/executor/test_reingest.py @@ -3,7 +3,7 @@ import datetime import json import pathlib -import time +from unittest.mock import patch import pytest from climate_ref_esmvaltool import provider as esmvaltool_provider @@ -197,28 +197,40 @@ def _patch_build_result(mocker, registry, mock_result): class TestAllocateOutputFragment: - def test_appends_timestamp_suffix(self): + def test_appends_timestamp_suffix(self, tmp_path): """Should append a UTC timestamp suffix to the base fragment.""" - result = allocate_output_fragment("provider/diag/abc123") + result = allocate_output_fragment("provider/diag/abc123", tmp_path) assert result.startswith("provider/diag/abc123_") - # Suffix should be a valid timestamp: YYYYMMDDTHHMMSS + # Suffix should be a valid timestamp: YYYYMMDDTHHMMSS followed by 6 microsecond digits suffix = result.split("_", 1)[1] - assert len(suffix) == 15 # 8 date + T + 6 time + assert len(suffix) == 21 # 8 date + T + 6 time + 6 microseconds assert "T" in suffix - def test_preserves_base_fragment(self): + def test_preserves_base_fragment(self, tmp_path): """The original fragment should be a prefix of the result.""" base = "my_provider/my_diag/hash123" - result = allocate_output_fragment(base) + result = allocate_output_fragment(base, tmp_path) assert result.startswith(base + "_") - def test_different_calls_produce_different_fragments(self): - """Two calls at least a second apart should produce different fragments.""" - result1 = allocate_output_fragment("provider/diag/abc123") - time.sleep(1.1) - result2 = allocate_output_fragment("provider/diag/abc123") + def test_different_calls_produce_different_fragments(self, tmp_path): + """Two rapid calls should produce different fragments (microsecond resolution).""" + result1 = allocate_output_fragment("provider/diag/abc123", tmp_path) + result2 = allocate_output_fragment("provider/diag/abc123", tmp_path) assert result1 != result2 + def test_raises_if_directory_already_exists(self, tmp_path): + """Should raise FileExistsError when the target directory already exists.""" + fixed_time = datetime.datetime(2026, 1, 1, 12, 0, 0, 0, tzinfo=datetime.timezone.utc) + with patch("climate_ref.executor.fragment.datetime") as mock_dt: + mock_dt.datetime.now.return_value = fixed_time + mock_dt.timezone = datetime.timezone + # First call succeeds + fragment = allocate_output_fragment("provider/diag/abc123", tmp_path) + # Create the directory so a second call with the same timestamp collides + (tmp_path / fragment).mkdir(parents=True) + with pytest.raises(FileExistsError, match="Output directory already exists"): + allocate_output_fragment("provider/diag/abc123", tmp_path) + # --- extract dataset attributes tests ---