From 8b1fe1f49214c4441a367b7b3e7f12c3227799f8 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Fri, 13 Nov 2026 07:18:03 +0000 Subject: [PATCH 1/2] feat: a digest of a spec's interface, and a check that a changed interface gets a new version An outputs or params model can change under the same spec name and version, and a record made with the old models is then read into the new classes without an error (ADR 0009). WorkflowSpec.digest is a short hash of the name, type, and requiredness of every field of both models, at any depth, so that such a change can be detected. Titles, descriptions, defaults, constraints, validators, field order, and class names and modules do not change it. ess.spec.testing.check_digests compares each spec's digest with a JSON file kept in the package, so that a model changed without a new version fails in the author's tests; write_digests writes the file. LoKI keeps its digests next to its spec tests. Co-Authored-By: Claude Fable 5.2 --- .../tests/loki/spec_digests.json | 7 + .../essinstruments/tests/loki/specs_test.py | 13 ++ packages/essspec/docs/params.md | 28 +++ .../essspec/src/ess/spec/_workflow_spec.py | 91 ++++++++- packages/essspec/src/ess/spec/testing.py | 52 +++++- packages/essspec/tests/digest_test.py | 176 ++++++++++++++++++ packages/essspec/tests/testing_test.py | 48 ++++- 7 files changed, 411 insertions(+), 4 deletions(-) create mode 100644 packages/essinstruments/tests/loki/spec_digests.json create mode 100644 packages/essspec/tests/digest_test.py diff --git a/packages/essinstruments/tests/loki/spec_digests.json b/packages/essinstruments/tests/loki/spec_digests.json new file mode 100644 index 00000000..caa81f98 --- /dev/null +++ b/packages/essinstruments/tests/loki/spec_digests.json @@ -0,0 +1,7 @@ +{ + "loki-beam-centre/v1": "8ecc60fbdaca2a91", + "loki-calibrated-detector/v1": "31b35013cb34f95c", + "loki-create-mask/v1": "f5fc5f6545f3b74d", + "loki-iofq/v1": "104c554cb62c72da", + "loki-masks/v1": "81f547b6fb3bcaea" +} diff --git a/packages/essinstruments/tests/loki/specs_test.py b/packages/essinstruments/tests/loki/specs_test.py index 34866836..14fcf72a 100644 --- a/packages/essinstruments/tests/loki/specs_test.py +++ b/packages/essinstruments/tests/loki/specs_test.py @@ -2,11 +2,18 @@ # Copyright (c) 2026 Scipp contributors (https://github.com/scipp) import subprocess import sys +from pathlib import Path import pytest +from ess.instruments.loki import specs from ess.instruments.loki.specs import AngleRange, BeamCentre from pydantic import ValidationError +from ess.spec import WorkflowSpec +from ess.spec.testing import check_digests + +DIGESTS = Path(__file__).with_name('spec_digests.json') + def test_specs_load_without_the_workflow_package() -> None: code = ( @@ -18,6 +25,12 @@ def test_specs_load_without_the_workflow_package() -> None: assert result.returncode == 0 +def test_each_spec_has_the_digest_kept_for_its_version() -> None: + check_digests( + [s for s in vars(specs).values() if isinstance(s, WorkflowSpec)], DIGESTS + ) + + @pytest.mark.parametrize('z', [0.0, -1.0]) def test_a_beam_centre_without_a_distance_from_the_sample_is_refused(z: float) -> None: with pytest.raises(ValidationError, match='greater than 0'): diff --git a/packages/essspec/docs/params.md b/packages/essspec/docs/params.md index a456e2f5..d0fd8f83 100644 --- a/packages/essspec/docs/params.md +++ b/packages/essspec/docs/params.md @@ -338,6 +338,34 @@ Values that a check compares, such as the start and stop of a range, are fields A check across parameters, such as a Q range the wavelength band can reach, belongs to the workflow and fails when the request runs. Since each parameter is validated alone, and a stage and a driver never get a whole params instance, the params model has no decorators, neither validators of any mode nor serializers nor computed fields, and no `__init__` of its own. +## The digest of a spec + +`spec.digest` is a short hex string computed from the name, type, and requiredness of each field of the params and outputs models, and of each group inside them, at any depth. +It tells a record made with other models under the same name and version from one of the bound spec ([ADR 0009](../../../docs/developer/adr/0009-a-record-of-another-spec-stays-readable.md)). + +| Changes the digest | Does not | +|---|---| +| a field renamed, added, or removed | a title, a description, a docstring | +| a field's type, such as `float` to `int`, `X` to `X \| None`, `X` to `Referenced[X]` | a default | +| a field made required or optional | a constraint, such as `Field(gt=0)`, or a validator | +| the values of a `Literal` or an enum | the order of fields, or of the members of a union | +| the metadata of a `File(...)` marker | the name or module of a group's class | + +A group is described by its fields, and any other class, such as `sc.DataArray`, by its name. +The digest does not see a change of what the workflow computes, so the version stays the author's statement of any change a consumer can observe. + +A package keeps the digests of its specs in a file and checks them in its tests, so that a model changed without a new version fails there: + +```python +from ess.spec.testing import check_digests, write_digests + +check_digests([IOFQ, BEAM_CENTRE], Path(__file__).with_name('spec_digests.json')) +write_digests([IOFQ, BEAM_CENTRE], path) # once, to make the file: {"sans-iofq/v1": "9f2c...", ...} +``` + +The check names each spec whose digest differs from the file's, and each spec the file lacks, with the entry the file needs. +A changed interface gets a new version; the file is updated under the same version only for a change that is meant to keep it, knowing that records of the old models are then of another spec. + ## What `WorkflowSpec` refuses Making a spec raises `pydantic.ValidationError`, which lists each problem with the model and field it concerns. diff --git a/packages/essspec/src/ess/spec/_workflow_spec.py b/packages/essspec/src/ess/spec/_workflow_spec.py index 8483d4a2..849f2912 100644 --- a/packages/essspec/src/ess/spec/_workflow_spec.py +++ b/packages/essspec/src/ess/spec/_workflow_spec.py @@ -48,12 +48,16 @@ from __future__ import annotations +import hashlib +import json from collections.abc import Callable, Sequence +from enum import Enum from pathlib import Path, PurePath from types import NoneType, UnionType from typing import ( Annotated, Any, + Literal, Self, TypeAliasType, Union, @@ -159,7 +163,8 @@ class WorkflowSpec[P: BaseModel, O: BaseModel](_SpecFields, frozen=True): version: Version of the workflow interface, at least 1. Increase it on any change that a consumer can observe: the params, the outputs, or what - the workflow computes. + the workflow computes. :attr:`digest` detects a change of the models + under one version. title: Display title of the workflow, not empty. description: @@ -218,6 +223,42 @@ def _check_models(self) -> Self: raise ValueError(f'{self.name}: {"; ".join(problems)}') return self + @property + def digest(self) -> str: + """ + A short hex string that changes when the stored form of the params or + the outputs changes, so that a record of the spec can tell whether the + spec it was made with had the models of this one. + + It covers the name, type, and requiredness of each field of the params + and outputs models, and of each group inside them, at any depth: + + - a group, a pydantic model, by its fields, not by its class name or + module, so that a class may be renamed or moved; + - the order of fields, and of the members of a union, does not count; + - a ``Literal`` by its values, and an enum by the values of its + members, which the stored form holds; + - ``Referenced[T]`` as a reference to ``T``, and a file field by the + metadata of its ``File(...)`` marker, which decide which files fit; + - a list, a dict, or a tuple by its type arguments, in order; + - any other type by its ``__name__``, such as ``float`` or + ``DataArray``, not by its module. + + Titles, descriptions, defaults, constraints such as ``Field(gt=0)``, + validators, discriminators, and the spec's name and version are not + part of it: they do not change the stored form of a value, and a + record stores its params with the defaults filled in. The digest does + not see a change of what the workflow computes, so the version stays + the author's statement of any change a consumer can observe. + ``ess.spec.testing.check_digests`` checks that a changed digest comes + with a new version. + """ + interface = { + 'params': _interface(self.params, ()), + 'outputs': _interface(self.outputs, ()), + } + return hashlib.sha256(_canonical(interface).encode()).hexdigest()[:16] + def serialize(self) -> SerializedWorkflowSpec: """ Project to the plain-data form. @@ -804,3 +845,51 @@ def _output_type_schema(annotation: Any) -> dict[str, Any]: else: schema = {'type': 'object', 'additionalProperties': _output_type_schema(item)} return {'anyOf': [schema, {'type': 'null'}]} if is_optional else schema + + +def _interface(model: type[BaseModel], stack: tuple[type[BaseModel], ...]) -> Any: + """ + The name, type, and requiredness of each field of ``model``, a group + inside the groups ``stack``, as plain data for :attr:`WorkflowSpec.digest`. + """ + if model in stack: + # A group that contains itself, which only an output may hold: how far up. + return {'recursive': len(stack) - stack.index(model)} + return { + name: { + 'type': _type_interface(field_types.field_type(info), (*stack, model)), + 'required': info.is_required(), + } + for name, info in model.model_fields.items() + } + + +def _type_interface(tp: Any, stack: tuple[type[BaseModel], ...]) -> Any: + """The type ``tp`` as plain data, by the rules of :attr:`WorkflowSpec.digest`.""" + origin, args = get_origin(tp), get_args(tp) + if origin is Annotated: + if REFERENCED in tp.__metadata__: + return {'referenced': _type_interface(args[0], stack)} + if (marker := field_types.file_marker(args[0], *tp.__metadata__)) is not None: + return {'file': marker.metadata} + return _type_interface(args[0], stack) + if origin in (Union, UnionType): + members = (_type_interface(arg, stack) for arg in args) + return {'union': sorted(members, key=_canonical)} + if origin is Literal: + values = (v.value if isinstance(v, Enum) else v for v in args) + return {'literal': sorted(map(_canonical, values))} + if isinstance(tp, type) and issubclass(tp, Enum): + return {'enum': sorted(_canonical(member.value) for member in tp)} + if field_types.is_group(tp): + return {'group': _interface(tp, stack)} + if origin is not None: + return { + field_types.type_name(origin): [_type_interface(a, stack) for a in args] + } + return field_types.type_name(tp) + + +def _canonical(data: Any) -> str: + """The JSON form of ``data``, the same for equal data in any process.""" + return json.dumps(data, sort_keys=True, separators=(',', ':')) diff --git a/packages/essspec/src/ess/spec/testing.py b/packages/essspec/src/ess/spec/testing.py index f83e1502..9bdb76fd 100644 --- a/packages/essspec/src/ess/spec/testing.py +++ b/packages/essspec/src/ess/spec/testing.py @@ -1,8 +1,11 @@ # SPDX-License-Identifier: BSD-3-Clause # Copyright (c) 2026 Scipp contributors (https://github.com/scipp) """ -Checks of the symmetries that a workflow author promises, for a package's tests. +Checks of what a workflow author promises, for a package's tests. +:func:`check_digests` checks that a spec whose interface changed has a new +version; ``packages/essspec/docs/params.md`` (The digest of a spec) shows how +a package uses it. The other checks are of the symmetries of a binding; ``packages/essspec/docs/bindings.md`` (Testing the symmetries) shows how a package uses them. @@ -17,8 +20,10 @@ from __future__ import annotations import copy +import json import math -from collections.abc import Callable, Iterator, Mapping, Sequence +from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence +from pathlib import Path from typing import Any from pydantic import BaseModel @@ -31,6 +36,49 @@ """A table's rows, as a push gives them.""" +def check_digests(specs: Iterable[WorkflowSpec[Any, Any]], path: Path) -> None: + """ + Raise ``AssertionError`` unless the digest of each spec is the one that + the file at ``path`` keeps under the spec's name and version. + + A digest changes when the stored form of the params or the outputs + changes (see :attr:`~ess.spec.WorkflowSpec.digest`). A record made with + the old models is then of another spec than the one bound under the same + name and version, so such a change comes with a new version. The file is + JSON, ``{"loki-iofq/v1": "9f2c..."}``, kept in the package and written by + :func:`write_digests`. The message names each spec whose digest differs + and each spec the file lacks, with the entry that the file would need. + """ + kept = json.loads(path.read_text()) if path.exists() else {} + problems = [] + for spec in specs: + key, digest = _digest_entry(spec) + entry = f'"{key}": "{digest}"' + if key not in kept: + problems.append( + f'{key} is not in {path}: add {entry}, or write the file with ' + 'ess.spec.testing.write_digests' + ) + elif kept[key] != digest: + problems.append( + f'the interface of {key} changed: give the spec a new version, or ' + f'update {path} to {entry} if the change is meant to keep the version' + ) + if problems: + raise AssertionError('; '.join(problems)) + + +def write_digests(specs: Iterable[WorkflowSpec[Any, Any]], path: Path) -> None: + """Write the digest of each spec to ``path``, as :func:`check_digests` reads it.""" + digests = dict(sorted(map(_digest_entry, specs))) + path.write_text(json.dumps(digests, indent=2) + '\n') + + +def _digest_entry(spec: WorkflowSpec[Any, Any]) -> tuple[str, str]: + """The key of ``spec`` in a file of digests, ``name/vN``, and its digest.""" + return f'{spec.name}/v{spec.version}', spec.digest + + def check_one_row( single: Binding[Any, Any], multi: Binding[Any, Any], diff --git a/packages/essspec/tests/digest_test.py b/packages/essspec/tests/digest_test.py new file mode 100644 index 00000000..ef00663b --- /dev/null +++ b/packages/essspec/tests/digest_test.py @@ -0,0 +1,176 @@ +# SPDX-License-Identifier: BSD-3-Clause +# Copyright (c) 2026 Scipp contributors (https://github.com/scipp) +""" +The digest of a spec changes with the name, type, or requiredness of a field +of its params or outputs, at any depth, and with nothing else. + +Each test places a group ``Centre``, or a variant of it, in a spec at one of +``PLACES``: a parameter, inside a parameter, and the same in the outputs. +""" + +from collections.abc import Callable +from enum import Enum +from typing import Annotated, Any, Literal + +import pytest +from pydantic import AfterValidator, BaseModel, Field, create_model, field_validator + +from ess.spec import MaskFile, Referenced, RunFile, WorkflowSpec + + +class Centre(BaseModel): + x: float + y: float = 0.0 + + +class Outputs(BaseModel): + n: int + + +def _spec( + params: dict[str, Any] | None = None, outputs: dict[str, Any] | None = None +) -> WorkflowSpec[Any, Any]: + return WorkflowSpec( + name='centre', + version=1, + title='Centre', + description='Finds the centre.', + params=create_model('Params', **(params or {})), + outputs=create_model('Outputs', **outputs) if outputs else Outputs, + ) + + +def _outer(group: type[BaseModel]) -> type[BaseModel]: + return create_model('Outer', centre=(group, ...)) + + +PLACES: dict[str, Callable[[type[BaseModel]], WorkflowSpec[Any, Any]]] = { + 'parameter': lambda g: _spec(params={'centre': (g, ...)}), + 'group in a parameter': lambda g: _spec(params={'outer': (_outer(g), ...)}), + 'list parameter': lambda g: _spec(params={'centres': (list[g], ...)}), # type: ignore[valid-type] + 'reference': lambda g: _spec(params={'centre': (Referenced[g], ...)}), + 'output': lambda g: _spec(outputs={'centre': (g, ...)}), + 'group in an output': lambda g: _spec(outputs={'outer': (_outer(g), ...)}), + 'dict output': lambda g: _spec(outputs={'centres': (dict[str, g] | None, ...)}), # type: ignore[valid-type] +} + + +class Described(BaseModel): + """Another class, with titles, descriptions, defaults, a constraint, a + validator, and the fields in another order.""" + + y: float = Field(default=1.0, title='Y', description='Vertical, in mm.') + x: Annotated[float, AfterValidator(abs)] = Field(gt=0, title='X') + + @field_validator('y') + @classmethod + def _finite(cls, value: float) -> float: + return value + + +class Renamed(BaseModel): + x_mm: float + y: float = 0.0 + + +class Retyped(BaseModel): + x: int + y: float = 0.0 + + +class XOptional(BaseModel): + x: float = 0.0 + y: float = 0.0 + + +class YRequired(BaseModel): + x: float + y: float + + +class Added(BaseModel): + x: float + y: float = 0.0 + z: float = 1.0 + + +class Removed(BaseModel): + x: float + + +@pytest.mark.parametrize('place', PLACES) +def test_titles_descriptions_defaults_and_order_do_not_change_the_digest( + place: str, +) -> None: + assert PLACES[place](Described).digest == PLACES[place](Centre).digest + + +@pytest.mark.parametrize('place', PLACES) +@pytest.mark.parametrize( + 'variant', [Renamed, Retyped, XOptional, YRequired, Added, Removed] +) +def test_a_changed_field_changes_the_digest( + place: str, variant: type[BaseModel] +) -> None: + assert PLACES[place](variant).digest != PLACES[place](Centre).digest + + +class Unit(Enum): + MM = 'mm' + M = 'm' + + +class Metre(Enum): + M = 'm' + + +class Given(BaseModel): + kind: Literal['given'] = 'given' + x: float = 0.0 + + +class Find(BaseModel): + kind: Literal['find'] = 'find' + + +@pytest.mark.parametrize( + ('a', 'b'), + [ + (float, float | None), + (Literal['a', 'b'], Literal['a']), + (Unit, Metre), + (Centre, Referenced[Centre]), + (RunFile, MaskFile), + (list[Centre], Centre), + ], +) +def test_a_changed_type_changes_the_digest(a: Any, b: Any) -> None: + assert _spec(params={'p': (a, ...)}).digest != _spec(params={'p': (b, ...)}).digest + + +def test_the_order_of_the_members_of_a_union_does_not_change_the_digest() -> None: + a = Annotated[Given | Find, Field(discriminator='kind')] + b = Annotated[Find | Given, Field(discriminator='kind')] + assert _spec(params={'p': (a, ...)}).digest == _spec(params={'p': (b, ...)}).digest + + +def test_a_field_moved_from_the_params_to_the_outputs_changes_the_digest() -> None: + assert ( + _spec(params={'centre': (Centre, ...)}).digest + != _spec(outputs={'centre': (Centre, ...), 'n': (int, ...)}).digest + ) + + +class Node(BaseModel): + value: float + children: list['Node'] + + +def test_an_output_group_that_contains_itself_has_a_digest() -> None: + assert _spec(outputs={'tree': (Node, ...)}).digest != _spec().digest + + +def test_the_digest_is_the_same_in_every_process() -> None: + # A literal, so that a change of the digest's rules, or of what pydantic or + # Python report of a type, fails here rather than in a workspace's log. + assert PLACES['parameter'](Centre).digest == '9c81351d3a6df500' diff --git a/packages/essspec/tests/testing_test.py b/packages/essspec/tests/testing_test.py index 91574d81..ead8b75c 100644 --- a/packages/essspec/tests/testing_test.py +++ b/packages/essspec/tests/testing_test.py @@ -2,15 +2,18 @@ # Copyright (c) 2026 Scipp contributors (https://github.com/scipp) """ Each check passes for a binding or driver that keeps its symmetry and fails -for one with a bug that breaks it. +for one with a bug that breaks it. The check of digests passes for the specs +whose digests a file keeps, and fails for a spec changed under its version. The toy spec ``SUB`` takes a sample and a can run and gives ``diff = factor * (sample.counts - can.counts)``; ``SUB_MULTI`` sums the counts of the rows of each table. """ +import re from collections.abc import Callable, Collection, Mapping from dataclasses import dataclass +from pathlib import Path from typing import Any import numpy as np @@ -25,8 +28,10 @@ assert_close, check_arrival_and_order, check_caching, + check_digests, check_one_row, one_row, + write_digests, ) @@ -387,3 +392,44 @@ def masked(data: Measured, scale: Scale) -> DataOutputs: check_caching( Calls(DATA, masked), {'data': Measured(array=data)}, [{'scale': Scale()}] ) + + +class ScaleInPercent(BaseModel): + percent: float = 100.0 + + +def test_digests_pass_for_the_specs_they_were_written_for(tmp_path: Path) -> None: + write_digests([SUB, SUB_MULTI], tmp_path / 'digests.json') + check_digests([SUB, SUB_MULTI], tmp_path / 'digests.json') + + +def test_digests_fail_for_a_spec_whose_interface_changed_under_its_version( + tmp_path: Path, +) -> None: + path = tmp_path / 'digests.json' + write_digests([SUB], path) + + class Changed(SubParams): + scale: ScaleInPercent = Field(default_factory=ScaleInPercent) # type: ignore[assignment] + + changed = _spec('sub', params=Changed, outputs=SubOutputs) + message = ( + 'the interface of sub/v1 changed: give the spec a new version, or update ' + f'{path} to "sub/v1": "{changed.digest}" if the change is meant to keep ' + 'the version' + ) + with pytest.raises(AssertionError, match=f'^{re.escape(message)}$'): + check_digests([changed], path) + + +def test_digests_fail_for_a_spec_the_file_lacks_and_say_how_to_add_it( + tmp_path: Path, +) -> None: + path = tmp_path / 'digests.json' + write_digests([SUB], path) + with pytest.raises( + AssertionError, + match=f'sub-multi/v1 is not in .*: add "sub-multi/v1": "{SUB_MULTI.digest}", ' + 'or write the file with ess.spec.testing.write_digests', + ): + check_digests([SUB, SUB_MULTI], path) From 4d5e5a6cbf18796e27c6e745214d5bef91abf1b6 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Fri, 13 Nov 2026 07:31:50 +0000 Subject: [PATCH 2/2] fix: the digest tells apart what stores differently, and the check covers every entry A RootModel is described by its root type, and a class that is not a group by its module and qualified name, so two classes of one name no longer share a digest. A Literal and an enum of the same values have one description. A group's extra setting counts. An unresolved type raises, naming the field, instead of hashing its repr. A params type with a serializer written with Annotated is refused, as a decorator serializer is. check_digests fails for a file entry of no given spec, and both functions refuse two specs of one name and version. LoKI's iofq digest changes with the Literal form. Co-Authored-By: Claude Fable 5.2 --- .../tests/loki/spec_digests.json | 2 +- packages/essspec/docs/params.md | 27 ++- .../essspec/src/ess/spec/_workflow_spec.py | 155 ++++++++++++++---- packages/essspec/src/ess/spec/testing.py | 47 ++++-- packages/essspec/tests/digest_test.py | 123 +++++++++++++- packages/essspec/tests/testing_test.py | 32 ++++ packages/essspec/tests/workflow_spec_test.py | 22 +++ 7 files changed, 353 insertions(+), 55 deletions(-) diff --git a/packages/essinstruments/tests/loki/spec_digests.json b/packages/essinstruments/tests/loki/spec_digests.json index caa81f98..438da84c 100644 --- a/packages/essinstruments/tests/loki/spec_digests.json +++ b/packages/essinstruments/tests/loki/spec_digests.json @@ -2,6 +2,6 @@ "loki-beam-centre/v1": "8ecc60fbdaca2a91", "loki-calibrated-detector/v1": "31b35013cb34f95c", "loki-create-mask/v1": "f5fc5f6545f3b74d", - "loki-iofq/v1": "104c554cb62c72da", + "loki-iofq/v1": "49a8fc1845f68d66", "loki-masks/v1": "81f547b6fb3bcaea" } diff --git a/packages/essspec/docs/params.md b/packages/essspec/docs/params.md index d0fd8f83..63dc73c2 100644 --- a/packages/essspec/docs/params.md +++ b/packages/essspec/docs/params.md @@ -330,7 +330,7 @@ A validator raises `ValueError` for a value it does not take; any other exceptio pydantic tries the members of a union in order, so a validator of a member written inline, such as a `model_validator(mode='before')` of `GivenCentre`, also sees a reference to an output, and refuses it with `ValueError`. The record stores a value written inline in JSON form, and the workflow gets it validated from that form. -So the JSON form of a group is the object of its fields: a field has no alias and no `exclude`, and a group has no serializers and no computed fields. +So the JSON form of a group is the object of its fields: a field has no alias, no `exclude`, and no serializer in its type, such as `Annotated[float, PlainSerializer(...)]`, and a group has no serializers and no computed fields. A group may have field validators and model validators of any mode. A value written inline must round-trip through JSON: a validator that changes an already valid value, such as converting millimetres to metres, is refused. @@ -350,8 +350,27 @@ It tells a record made with other models under the same name and version from on | a field made required or optional | a constraint, such as `Field(gt=0)`, or a validator | | the values of a `Literal` or an enum | the order of fields, or of the members of a union | | the metadata of a `File(...)` marker | the name or module of a group's class | +| the `extra` setting of a group | the names of an enum's members, or an enum replaced by a `Literal` of its values | +| the root type of a `RootModel` | | +| the module or name of a class that is not a group | | -A group is described by its fields, and any other class, such as `sc.DataArray`, by its name. +A type is described so: + +```python +class Centre(BaseModel): # a group: by its fields, and by `extra` unless default + x: float # a builtin, a time, Path, a scipp type: by its name + unit: Literal['mm', 'm'] # a Literal or an enum: by its values, {'values': ['m', 'mm']} + +class Masks(RootModel[dict[str, sc.Variable]]): ... # by its root type, dict[str, Variable] + +class Shapes(RootModel[list[Polygon]]): ... # any other class, such as Polygon: by module and name, 'mypackage.shapes.Polygon' +``` + +Changing a file field's metadata, including the role of a raw file, changes the digest. +`Referenced[T]` is described by the fields of `T`, since a reference is checked by class when it is used: `T` replaced by another class with the same fields keeps the digest, and a reference made with the old class is refused when it is checked. +Requiredness counts, although a record stores its params with the defaults filled in, since a parameter that becomes required breaks callers. +Serialization settings of a group's `model_config` other than `extra`, such as `ser_json_inf_nan`, are not covered. +A type that is not resolved, such as a forward reference before `model_rebuild()`, has no digest: `spec.digest` raises, naming the field. The digest does not see a change of what the workflow computes, so the version stays the author's statement of any change a consumer can observe. A package keeps the digests of its specs in a file and checks them in its tests, so that a model changed without a new version fails there: @@ -363,7 +382,8 @@ check_digests([IOFQ, BEAM_CENTRE], Path(__file__).with_name('spec_digests.json') write_digests([IOFQ, BEAM_CENTRE], path) # once, to make the file: {"sans-iofq/v1": "9f2c...", ...} ``` -The check names each spec whose digest differs from the file's, and each spec the file lacks, with the entry the file needs. +The check names each spec whose digest differs from the file's, each spec the file lacks, with the entry the file needs, and each entry of the file that no spec given has. +The file holds the given specs and nothing else, so a test that finds no specs fails. A changed interface gets a new version; the file is updated under the same version only for a change that is meant to keep it, knowing that records of the old models are then of another spec. ## What `WorkflowSpec` refuses @@ -422,4 +442,5 @@ Params model and groups ([When values are validated](#when-values-are-validated) - A field has no alias (`alias`, `validation_alias`, `serialization_alias`) and no `exclude` or `exclude_if`. - A group has no serializers and no computed fields. +- A field's type has no serializer written with `Annotated`, such as `PlainSerializer` or `WrapSerializer`. - The params model has no decorators (validators of any mode, serializers, computed fields) and no `__init__` of its own. diff --git a/packages/essspec/src/ess/spec/_workflow_spec.py b/packages/essspec/src/ess/spec/_workflow_spec.py index 849f2912..ce99f07d 100644 --- a/packages/essspec/src/ess/spec/_workflow_spec.py +++ b/packages/essspec/src/ess/spec/_workflow_spec.py @@ -57,6 +57,7 @@ from typing import ( Annotated, Any, + ForwardRef, Literal, Self, TypeAliasType, @@ -70,6 +71,9 @@ BaseModel, ConfigDict, Field, + PlainSerializer, + RootModel, + WrapSerializer, model_validator, ) from pydantic.fields import FieldInfo @@ -231,27 +235,49 @@ def digest(self) -> str: spec it was made with had the models of this one. It covers the name, type, and requiredness of each field of the params - and outputs models, and of each group inside them, at any depth: - - - a group, a pydantic model, by its fields, not by its class name or - module, so that a class may be renamed or moved; - - the order of fields, and of the members of a union, does not count; - - a ``Literal`` by its values, and an enum by the values of its - members, which the stored form holds; - - ``Referenced[T]`` as a reference to ``T``, and a file field by the - metadata of its ``File(...)`` marker, which decide which files fit; - - a list, a dict, or a tuple by its type arguments, in order; - - any other type by its ``__name__``, such as ``float`` or - ``DataArray``, not by its module. + and outputs models, and of each group inside them, at any depth. + Requiredness counts although a record stores its params with the + defaults filled in, since a parameter that becomes required breaks + callers. A type is described so: + + - a group, a pydantic model that is not a ``RootModel``, by its fields + and by its ``extra`` setting unless that is the default, not by its + class name or module, so that a group may be renamed or moved; + - a ``RootModel`` by the type of its root; + - a ``Literal`` and an enum by their values, which the stored form + holds, so that the two are the same and an enum member may be + renamed; + - ``Referenced[T]`` as a reference to ``T`` and its fields, and a file + field by the metadata of its ``File(...)`` marker, the role of a raw + file included, which decide which files fit; + - a union by its members, a list, a dict, or a tuple by its type + arguments, in order; + - a builtin, such as ``float`` or ``list``, a time, ``Path``, and a + scipp type, such as ``DataArray``, by its name; + - any other class by its module and qualified name, such as + ``mypackage.masks.MaskSet``, so that moving or renaming it changes + the digest. + + The order of fields, and of the members of a union, does not count. + A reference is checked by class when it is used, so ``T`` replaced by + another class of the same fields keeps the digest, and a reference + made with the old class is refused when it is checked. Titles, descriptions, defaults, constraints such as ``Field(gt=0)``, validators, discriminators, and the spec's name and version are not - part of it: they do not change the stored form of a value, and a - record stores its params with the defaults filled in. The digest does - not see a change of what the workflow computes, so the version stays - the author's statement of any change a consumer can observe. + part of it: they do not change the stored form of a value. Settings + of a group's ``model_config`` that change its JSON form, other than + ``extra``, such as ``ser_json_inf_nan``, are not covered. The digest + does not see a change of what the workflow computes, so the version + stays the author's statement of any change a consumer can observe. ``ess.spec.testing.check_digests`` checks that a changed digest comes with a new version. + + Raises + ------ + ValueError + If the type of a field is not resolved, such as a forward + reference before ``model_rebuild()``, or is not made of classes. """ interface = { 'params': _interface(self.params, ()), @@ -385,7 +411,8 @@ def _fields_problems( The problems of each field of ``model``, the params model or a group, inside the groups ``stack``, the params model first. Each field is typed as :func:`_type_problems` checks, has no alias, is not excluded from the - JSON form, and has a default that is a value (see + JSON form, has no serializer in its type (see + :func:`_annotated_serializers`), and has a default that is a value (see :func:`_default_problem`). """ kind = 'a parameter' if len(stack) == 1 else 'a field of a group' @@ -402,6 +429,9 @@ def _fields_problems( ) continue metadata = field_types.field_metadata(field) + if found := _annotated_serializers(field.annotation, metadata): + problems.append(f'{where}: no serializers, {found}') + continue if found := _type_problems(field.annotation, metadata, stack): problems.extend(f'{where}: {problem}' for problem in found) elif field.default_factory_takes_validated_data: @@ -411,6 +441,25 @@ def _fields_problems( return problems +def _annotated_serializers(annotation: Any, metadata: tuple[Any, ...]) -> list[str]: + """ + The serializers written with ``Annotated``, such as ``PlainSerializer``, + in ``metadata`` or in ``annotation``, not inside a class in it: each makes + the JSON form of a value differ from that of its type, as a decorator + serializer of a group does. + """ + found = [type(m).__name__ for m in metadata if isinstance(m, _SERIALIZER_MARKERS)] + if get_origin(annotation) is Annotated: + inner = get_args(annotation)[0] + return [*found, *_annotated_serializers(inner, annotation.__metadata__)] + for arg in get_args(annotation): + found.extend(_annotated_serializers(arg, ())) + return found + + +_SERIALIZER_MARKERS = (PlainSerializer, WrapSerializer) + + def _type_problems( annotation: Any, metadata: tuple[Any, ...], stack: tuple[type[BaseModel], ...] ) -> list[str]: @@ -851,17 +900,37 @@ def _interface(model: type[BaseModel], stack: tuple[type[BaseModel], ...]) -> An """ The name, type, and requiredness of each field of ``model``, a group inside the groups ``stack``, as plain data for :attr:`WorkflowSpec.digest`. + + Raises + ------ + ValueError + If the type of a field cannot be described, naming the field. """ if model in stack: # A group that contains itself, which only an output may hold: how far up. return {'recursive': len(stack) - stack.index(model)} - return { - name: { - 'type': _type_interface(field_types.field_type(info), (*stack, model)), - 'required': info.is_required(), - } - for name, info in model.model_fields.items() - } + described = {} + for name, info in model.model_fields.items(): + try: + tp = _type_interface(field_types.field_type(info), (*stack, model)) + except _Undescribed as error: + raise ValueError( + f'{model.__name__}.{name}: {error}; the digest describes a field ' + 'by its type, so each type in it is a class, resolved' + ) from None + described[name] = {'type': tp, 'required': info.is_required()} + return described + + +class _Undescribed(Exception): + """A type that :func:`_type_interface` cannot describe.""" + + +_SHORT_NAMED = (Path, *field_types.TIMES) +""" +Classes outside ``builtins`` and scipp described by their name alone, which no +other class of a spec takes. +""" def _type_interface(tp: Any, stack: tuple[type[BaseModel], ...]) -> Any: @@ -876,18 +945,40 @@ def _type_interface(tp: Any, stack: tuple[type[BaseModel], ...]) -> Any: if origin in (Union, UnionType): members = (_type_interface(arg, stack) for arg in args) return {'union': sorted(members, key=_canonical)} + # A Literal and an enum of the same values store the same JSON. if origin is Literal: - values = (v.value if isinstance(v, Enum) else v for v in args) - return {'literal': sorted(map(_canonical, values))} + values = [v.value if isinstance(v, Enum) else v for v in args] + return {'values': sorted(values, key=_canonical)} if isinstance(tp, type) and issubclass(tp, Enum): - return {'enum': sorted(_canonical(member.value) for member in tp)} + return {'values': sorted((m.value for m in tp), key=_canonical)} if field_types.is_group(tp): - return {'group': _interface(tp, stack)} + group = {'group': _interface(tp, stack)} + if (extra := tp.model_config.get('extra')) not in (None, 'ignore'): + group['extra'] = extra + return group + if isinstance(tp, type) and issubclass(tp, RootModel): + root = tp.model_fields['root'] + return {'root': _type_interface(field_types.field_type(root), stack)} if origin is not None: - return { - field_types.type_name(origin): [_type_interface(a, stack) for a in args] - } - return field_types.type_name(tp) + return {_class_name(origin): [_type_interface(a, stack) for a in args]} + if tp is Ellipsis: + return '...' + if isinstance(tp, ForwardRef | str): + raise _Undescribed(f'{tp!r} is not resolved; call model_rebuild() first') + if not isinstance(tp, type): + raise _Undescribed(f'{tp!r} is not a class') + return _class_name(tp) + + +def _class_name(cls: type) -> str: + """ + The name of ``cls`` in a digest: its name alone for a builtin, a time, + ``Path``, or a scipp type, else its module and qualified name. + """ + module = cls.__module__.partition('.')[0] + if cls in _SHORT_NAMED or module in ('builtins', 'scipp'): + return field_types.type_name(cls) + return f'{cls.__module__}.{cls.__qualname__}' def _canonical(data: Any) -> str: diff --git a/packages/essspec/src/ess/spec/testing.py b/packages/essspec/src/ess/spec/testing.py index 9bdb76fd..dd8ca0b2 100644 --- a/packages/essspec/src/ess/spec/testing.py +++ b/packages/essspec/src/ess/spec/testing.py @@ -38,21 +38,28 @@ def check_digests(specs: Iterable[WorkflowSpec[Any, Any]], path: Path) -> None: """ - Raise ``AssertionError`` unless the digest of each spec is the one that - the file at ``path`` keeps under the spec's name and version. + Raise ``AssertionError`` unless the file at ``path`` keeps the digest of + each spec under the spec's name and version, and nothing else. A digest changes when the stored form of the params or the outputs changes (see :attr:`~ess.spec.WorkflowSpec.digest`). A record made with the old models is then of another spec than the one bound under the same name and version, so such a change comes with a new version. The file is JSON, ``{"loki-iofq/v1": "9f2c..."}``, kept in the package and written by - :func:`write_digests`. The message names each spec whose digest differs - and each spec the file lacks, with the entry that the file would need. + :func:`write_digests`. The message names each spec whose digest differs, + each spec the file lacks, with the entry that the file would need, and + each entry of the file that no spec has, so that a test that finds no + specs fails. + + Raises + ------ + ValueError + If two specs have the same name and version. """ + digests = _digests(specs) kept = json.loads(path.read_text()) if path.exists() else {} problems = [] - for spec in specs: - key, digest = _digest_entry(spec) + for key, digest in digests.items(): entry = f'"{key}": "{digest}"' if key not in kept: problems.append( @@ -64,19 +71,35 @@ def check_digests(specs: Iterable[WorkflowSpec[Any, Any]], path: Path) -> None: f'the interface of {key} changed: give the spec a new version, or ' f'update {path} to {entry} if the change is meant to keep the version' ) + problems.extend( + f'{key} in {path} is of no spec given: remove it if the spec is gone' + for key in sorted(kept.keys() - digests.keys()) + ) if problems: raise AssertionError('; '.join(problems)) def write_digests(specs: Iterable[WorkflowSpec[Any, Any]], path: Path) -> None: - """Write the digest of each spec to ``path``, as :func:`check_digests` reads it.""" - digests = dict(sorted(map(_digest_entry, specs))) - path.write_text(json.dumps(digests, indent=2) + '\n') + """ + Write the digest of each spec to ``path``, as :func:`check_digests` reads it. + + Raises + ------ + ValueError + If two specs have the same name and version. + """ + path.write_text(json.dumps(dict(sorted(_digests(specs).items())), indent=2) + '\n') -def _digest_entry(spec: WorkflowSpec[Any, Any]) -> tuple[str, str]: - """The key of ``spec`` in a file of digests, ``name/vN``, and its digest.""" - return f'{spec.name}/v{spec.version}', spec.digest +def _digests(specs: Iterable[WorkflowSpec[Any, Any]]) -> dict[str, str]: + """The digest of each spec by its key in a file of digests, ``name/vN``.""" + digests: dict[str, str] = {} + for spec in specs: + key = f'{spec.name}/v{spec.version}' + if key in digests: + raise ValueError(f'{key}: two specs of the same name and version') + digests[key] = spec.digest + return digests def check_one_row( diff --git a/packages/essspec/tests/digest_test.py b/packages/essspec/tests/digest_test.py index ef00663b..bcc47913 100644 --- a/packages/essspec/tests/digest_test.py +++ b/packages/essspec/tests/digest_test.py @@ -2,18 +2,29 @@ # Copyright (c) 2026 Scipp contributors (https://github.com/scipp) """ The digest of a spec changes with the name, type, or requiredness of a field -of its params or outputs, at any depth, and with nothing else. +of its params or outputs, at any depth, and with the ``extra`` setting of a +group, and with nothing else. Each test places a group ``Centre``, or a variant of it, in a spec at one of ``PLACES``: a parameter, inside a parameter, and the same in the outputs. """ +import uuid from collections.abc import Callable from enum import Enum from typing import Annotated, Any, Literal import pytest -from pydantic import AfterValidator, BaseModel, Field, create_model, field_validator +import scipp as sc +from pydantic import ( + AfterValidator, + BaseModel, + ConfigDict, + Field, + RootModel, + create_model, + field_validator, +) from ess.spec import MaskFile, Referenced, RunFile, WorkflowSpec @@ -27,6 +38,9 @@ class Outputs(BaseModel): n: int +ANY_TYPE = ConfigDict(arbitrary_types_allowed=True) + + def _spec( params: dict[str, Any] | None = None, outputs: dict[str, Any] | None = None ) -> WorkflowSpec[Any, Any]: @@ -36,7 +50,9 @@ def _spec( title='Centre', description='Finds the centre.', params=create_model('Params', **(params or {})), - outputs=create_model('Outputs', **outputs) if outputs else Outputs, + outputs=create_model('Outputs', __config__=ANY_TYPE, **outputs) + if outputs + else Outputs, ) @@ -124,6 +140,11 @@ class Metre(Enum): M = 'm' +class UnitRenamed(Enum): + MILLIMETRE = 'mm' + M = 'm' + + class Given(BaseModel): kind: Literal['given'] = 'given' x: float = 0.0 @@ -148,6 +169,59 @@ def test_a_changed_type_changes_the_digest(a: Any, b: Any) -> None: assert _spec(params={'p': (a, ...)}).digest != _spec(params={'p': (b, ...)}).digest +@pytest.mark.parametrize( + ('a', 'b'), + [ + (Unit, UnitRenamed), + (Unit, Literal['m', 'mm']), + (Literal['mm', 'm'], Literal['m', 'mm']), + ], +) +def test_an_enum_or_literal_of_the_same_values_keeps_the_digest(a: Any, b: Any) -> None: + assert _spec(params={'p': (a, ...)}).digest == _spec(params={'p': (b, ...)}).digest + + +class Forbidding(Centre): + model_config = ConfigDict(extra='forbid') + + +def test_the_extra_setting_of_a_group_changes_the_digest() -> None: + assert PLACES['parameter'](Forbidding).digest != PLACES['parameter'](Centre).digest + + +class IntMasks(RootModel[dict[str, int]]): + pass + + +class StrMasks(RootModel[dict[str, str]]): + pass + + +def test_a_root_model_is_described_by_its_root_type() -> None: + assert ( + _spec(outputs={'masks': (IntMasks, ...)}).digest + != _spec(outputs={'masks': (StrMasks, ...)}).digest + ) + + +def test_a_class_that_is_not_a_group_is_described_by_its_module() -> None: + here = type('Mask', (), {'__module__': 'here'}) + there = type('Mask', (), {'__module__': 'there'}) + assert ( + _spec(outputs={'mask': (here, ...)}).digest + != _spec(outputs={'mask': (there, ...)}).digest + ) + + +def test_an_unresolved_type_has_no_digest() -> None: + class Late(BaseModel): + later: 'Later' # type: ignore[name-defined] # noqa: F821 + + spec = _spec(outputs={'late': (Late, ...)}) + with pytest.raises(ValueError, match=r"^Late\.later: ForwardRef\('Later'\)"): + _ = spec.digest + + def test_the_order_of_the_members_of_a_union_does_not_change_the_digest() -> None: a = Annotated[Given | Find, Field(discriminator='kind')] b = Annotated[Find | Given, Field(discriminator='kind')] @@ -170,7 +244,42 @@ def test_an_output_group_that_contains_itself_has_a_digest() -> None: assert _spec(outputs={'tree': (Node, ...)}).digest != _spec().digest -def test_the_digest_is_the_same_in_every_process() -> None: - # A literal, so that a change of the digest's rules, or of what pydantic or - # Python report of a type, fails here rather than in a workspace's log. - assert PLACES['parameter'](Centre).digest == '9c81351d3a6df500' +# Literals, so that a change of the digest's rules, or of what pydantic or Python +# report of a type, fails here rather than in a workspace's log. One per kind of +# description. +STABLE = { + 'scalar': (lambda: _spec(params={'p': (float, ...)}), 'dae9f48e5eab709f'), + 'group': (lambda: PLACES['parameter'](Centre), '9c81351d3a6df500'), + 'nested group': ( + lambda: PLACES['group in a parameter'](Centre), + '5e679ffa780f085e', + ), + 'union with None': ( + lambda: _spec(params={'p': (float | None, None)}), + 'a0fe99b0f1b0f016', + ), + 'Literal': ( + lambda: _spec(params={'p': (Literal['a', 'b'], ...)}), + 'def1a06088db55d0', + ), + 'enum': (lambda: _spec(params={'p': (Unit, ...)}), '32e94d99d33720a4'), + 'file field': (lambda: _spec(params={'p': (MaskFile, ...)}), 'bf81c990bc493a95'), + 'Referenced[T]': (lambda: PLACES['reference'](Centre), '8fd19f5a95e8a310'), + 'list': (lambda: PLACES['list parameter'](Centre), '1d5ed60276784d20'), + 'dict': (lambda: _spec(outputs={'o': (dict[str, float], ...)}), '016544ebacea2bae'), + 'scipp type': ( + lambda: _spec(outputs={'o': (sc.DataArray, ...)}), + '8c37efc2323ffa18', + ), + 'RootModel': (lambda: _spec(outputs={'o': (IntMasks, ...)}), '1cc5f4c89f907304'), + 'class by module': ( + lambda: _spec(outputs={'o': (uuid.UUID, ...)}), + 'b73ba80b6447254a', + ), +} + + +@pytest.mark.parametrize('kind', STABLE) +def test_the_digest_is_the_same_in_every_process(kind: str) -> None: + make, expected = STABLE[kind] + assert make().digest == expected diff --git a/packages/essspec/tests/testing_test.py b/packages/essspec/tests/testing_test.py index ead8b75c..f86354fd 100644 --- a/packages/essspec/tests/testing_test.py +++ b/packages/essspec/tests/testing_test.py @@ -433,3 +433,35 @@ def test_digests_fail_for_a_spec_the_file_lacks_and_say_how_to_add_it( 'or write the file with ess.spec.testing.write_digests', ): check_digests([SUB, SUB_MULTI], path) + + +def test_digests_fail_for_an_entry_of_no_spec_and_say_to_remove_it( + tmp_path: Path, +) -> None: + path = tmp_path / 'digests.json' + write_digests([SUB, SUB_MULTI], path) + message = ( + f'sub-multi/v1 in {path} is of no spec given: remove it if the spec is gone' + ) + with pytest.raises(AssertionError, match=f'^{re.escape(message)}$'): + check_digests([SUB], path) + + +def test_digests_fail_when_no_spec_is_given(tmp_path: Path) -> None: + path = tmp_path / 'digests.json' + write_digests([SUB], path) + with pytest.raises(AssertionError, match=r'sub/v1 .* is of no spec given'): + check_digests([], path) + + +@pytest.mark.parametrize('function', [check_digests, write_digests]) +def test_digests_refuse_two_specs_of_the_same_name_and_version( + tmp_path: Path, function: Callable[..., None] +) -> None: + other = _spec( + 'sub', params=SubMultiParams, outputs=SubOutputs, tables=('sample', 'can') + ) + with pytest.raises( + ValueError, match=r'^sub/v1: two specs of the same name and version$' + ): + function([SUB, other], tmp_path / 'digests.json') diff --git a/packages/essspec/tests/workflow_spec_test.py b/packages/essspec/tests/workflow_spec_test.py index c2d76dbf..e2f6d442 100644 --- a/packages/essspec/tests/workflow_spec_test.py +++ b/packages/essspec/tests/workflow_spec_test.py @@ -16,7 +16,9 @@ BaseModel, Field, NaiveDatetime, + PlainSerializer, RootModel, + WrapSerializer, computed_field, field_serializer, field_validator, @@ -153,6 +155,10 @@ def _bins(self, bins: int) -> str: return str(bins) +InMilli = Annotated[float, PlainSerializer(lambda v: v * 1000, return_type=float)] +Wrapped = Annotated[int, WrapSerializer(lambda v, handler: handler(v))] + + class SerializedParams(BaseModel): reduction: Reduction = Field(default_factory=Reduction) @@ -1288,6 +1294,22 @@ def test_accepted(self, fields: dict[str, Any]) -> None: r"P.reduction: SerializedReduction: no serializers or computed " r"fields, {'field_serializers': \['_bins'\]}", ), + *( + ({'params': model('P', **fields)}, f"{where}: no serializers, {found}") + for fields, where, found in ( + ({'bins': (InMilli, ...)}, 'P.bins', r"\['PlainSerializer'\]"), + ( + {'bins': (list[InMilli] | None, None)}, + 'P.bins', + r"\['PlainSerializer'\]", + ), + ( + {'run': (model('Run', bins=(Wrapped, ...)), ...)}, + 'P.run: Run.bins', + r"\['WrapSerializer'\]", + ), + ) + ), ( { 'params': model(