From 060c05a34db6c8378ab5274a5a4ebcfeeaff6302 Mon Sep 17 00:00:00 2001 From: Jon Deng Date: Wed, 29 Jul 2026 16:09:42 -0700 Subject: [PATCH 1/3] Python(feat): expose multi-value metadata via a Metadata mapping with getall metadata_proto_to_dict now returns Metadata, a dict subclass whose scalar view keeps the first value per key (unchanged for existing callers) and whose getall(key) returns the full ordered value list. Run/Asset/Report/Channel .metadata fields adopt the type, with a pydantic core-schema hook so validation passes instances through instead of rebuilding them as plain dicts (which would drop the extra values). Co-Authored-By: Claude Fable 5 --- .../sift_client/_tests/util/test_metadata.py | 72 +++++++++++++- python/lib/sift_client/sift_types/asset.py | 4 +- python/lib/sift_client/sift_types/channel.py | 4 +- python/lib/sift_client/sift_types/report.py | 4 +- python/lib/sift_client/sift_types/run.py | 4 +- python/lib/sift_client/util/metadata.py | 93 ++++++++++++++++--- 6 files changed, 159 insertions(+), 22 deletions(-) diff --git a/python/lib/sift_client/_tests/util/test_metadata.py b/python/lib/sift_client/_tests/util/test_metadata.py index 3bc14a5a0..2e0ce6e3e 100644 --- a/python/lib/sift_client/_tests/util/test_metadata.py +++ b/python/lib/sift_client/_tests/util/test_metadata.py @@ -1,10 +1,14 @@ +import json + +import pytest +from pydantic import BaseModel from sift.metadata.v1.metadata_pb2 import ( MetadataKey, MetadataKeyType, MetadataValue, ) -from sift_client.util.metadata import metadata_dict_to_proto, metadata_proto_to_dict +from sift_client.util.metadata import Metadata, metadata_dict_to_proto, metadata_proto_to_dict def _string_value(name: str, value: str) -> MetadataValue: @@ -46,9 +50,75 @@ def test_multi_value_key_keeps_first_value(self): "env": "prod", } + def test_multi_value_key_exposes_all_values_via_getall(self): + metadata = [ + _string_value("associated_parts", "ABC"), + _string_value("associated_parts", "XYZ"), + _string_value("env", "prod"), + ] + result = metadata_proto_to_dict(metadata) + assert isinstance(result, Metadata) + assert result.getall("associated_parts") == ["ABC", "XYZ"] + assert result.getall("env") == ["prod"] + assert result.getall("missing") == [] + def test_empty_metadata(self): assert metadata_proto_to_dict([]) == {} def test_round_trip_from_dict(self): original = {"env": "prod", "build": 1.5, "armed": True} assert metadata_proto_to_dict(metadata_dict_to_proto(original)) == original + + +class TestMetadataMapping: + def test_dict_view_and_getall_invariant(self): + md = Metadata( + {"associated_parts": "ABC", "env": "prod"}, + {"associated_parts": ["ABC", "XYZ"], "env": ["prod"]}, + ) + assert md["associated_parts"] == "ABC" + assert md == {"associated_parts": "ABC", "env": "prod"} + for key in md: + assert md[key] == md.getall(key)[0] + + def test_getall_falls_back_to_scalar_without_all_values(self): + md = Metadata({"env": "prod"}) + assert md.getall("env") == ["prod"] + assert md.getall("missing") == [] + + def test_getall_returns_a_copy(self): + md = Metadata({"k": "a"}, {"k": ["a", "b"]}) + md.getall("k").append("mutated") + assert md.getall("k") == ["a", "b"] + + def test_json_serializable(self): + md = Metadata({"env": "prod"}, {"env": ["prod"]}) + assert json.loads(json.dumps(md)) == {"env": "prod"} + + +class TestMetadataPydanticField: + class _Model(BaseModel): + metadata: Metadata + + def test_instance_passes_through_validation_with_all_values(self): + # Pydantic must not rebuild the mapping as a plain dict -- that would + # silently drop the extra values behind getall(). + md = Metadata({"parts": "ABC"}, {"parts": ["ABC", "XYZ"]}) + model = self._Model(metadata=md) + assert isinstance(model.metadata, Metadata) + assert model.metadata.getall("parts") == ["ABC", "XYZ"] + + def test_plain_dict_is_wrapped(self): + model = self._Model(metadata={"env": "prod"}) + assert isinstance(model.metadata, Metadata) + assert model.metadata.getall("env") == ["prod"] + + def test_non_mapping_rejected(self): + with pytest.raises(ValueError, match="expected a metadata mapping"): + self._Model(metadata="not-a-mapping") # type: ignore[arg-type] + + def test_model_dump_serializes_as_plain_dict(self): + md = Metadata({"parts": "ABC"}, {"parts": ["ABC", "XYZ"]}) + model = self._Model(metadata=md) + assert model.model_dump() == {"metadata": {"parts": "ABC"}} + assert json.loads(model.model_dump_json()) == {"metadata": {"parts": "ABC"}} diff --git a/python/lib/sift_client/sift_types/asset.py b/python/lib/sift_client/sift_types/asset.py index ea0895929..1a3b82c32 100644 --- a/python/lib/sift_client/sift_types/asset.py +++ b/python/lib/sift_client/sift_types/asset.py @@ -8,7 +8,7 @@ from sift_client.sift_types._base import BaseType, MappingHelper, ModelUpdate from sift_client.sift_types._mixins.file_attachments import FileAttachmentsMixin from sift_client.sift_types.tag import Tag -from sift_client.util.metadata import metadata_dict_to_proto, metadata_proto_to_dict +from sift_client.util.metadata import Metadata, metadata_dict_to_proto, metadata_proto_to_dict if TYPE_CHECKING: from sift_client.client import SiftClient @@ -29,7 +29,7 @@ class Asset(BaseType[AssetProto, "Asset"], FileAttachmentsMixin): tags: list[str | Tag] # NOTE: update() replaces this map wholesale. See TODO(metadata-mixin) in # sift_types/_mixins/metadata.py before adding keys at runtime. - metadata: dict[str, str | float | bool] + metadata: Metadata is_archived: bool # Optional fields diff --git a/python/lib/sift_client/sift_types/channel.py b/python/lib/sift_client/sift_types/channel.py index 03e8129bc..38d47c5dd 100644 --- a/python/lib/sift_client/sift_types/channel.py +++ b/python/lib/sift_client/sift_types/channel.py @@ -26,7 +26,7 @@ ) from sift_client.sift_types._base import BaseType, MappingHelper, ModelUpdate -from sift_client.util.metadata import metadata_dict_to_proto, metadata_proto_to_dict +from sift_client.util.metadata import Metadata, metadata_dict_to_proto, metadata_proto_to_dict if TYPE_CHECKING: from sift_stream_bindings import ChannelBitFieldElementPy, ChannelDataTypePy @@ -254,7 +254,7 @@ class Channel(BaseType[ChannelProto, "Channel"]): bit_field_elements: list[ChannelBitFieldElement] = Field(default_factory=list) enum_types: dict[str, int] = Field(default_factory=dict) asset_id: str - metadata: dict[str, str | float | bool] = Field(default_factory=dict) + metadata: Metadata = Field(default_factory=Metadata) is_archived: bool created_date: datetime modified_date: datetime diff --git a/python/lib/sift_client/sift_types/report.py b/python/lib/sift_client/sift_types/report.py index 34f64e2f1..446211ebb 100644 --- a/python/lib/sift_client/sift_types/report.py +++ b/python/lib/sift_client/sift_types/report.py @@ -11,7 +11,7 @@ from sift_client.sift_types._base import BaseType, MappingHelper, ModelUpdate from sift_client.sift_types.tag import Tag -from sift_client.util.metadata import metadata_dict_to_proto, metadata_proto_to_dict +from sift_client.util.metadata import Metadata, metadata_dict_to_proto, metadata_proto_to_dict if TYPE_CHECKING: from sift_client.client import SiftClient @@ -110,7 +110,7 @@ class Report(BaseType[ReportProto, "Report"]): rerun_from_report_id: str | None = None # NOTE: update() replaces this map wholesale. See TODO(metadata-mixin) in # sift_types/_mixins/metadata.py before adding keys at runtime. - metadata: dict[str, str | float | bool] + metadata: Metadata job_id: str archived_date: datetime | None = None is_archived: bool diff --git a/python/lib/sift_client/sift_types/run.py b/python/lib/sift_client/sift_types/run.py index e91225342..689341e6b 100644 --- a/python/lib/sift_client/sift_types/run.py +++ b/python/lib/sift_client/sift_types/run.py @@ -16,7 +16,7 @@ ) from sift_client.sift_types._mixins.file_attachments import FileAttachmentsMixin from sift_client.sift_types.tag import Tag -from sift_client.util.metadata import metadata_dict_to_proto, metadata_proto_to_dict +from sift_client.util.metadata import Metadata, metadata_dict_to_proto, metadata_proto_to_dict if TYPE_CHECKING: from pathlib import Path @@ -42,7 +42,7 @@ class Run(BaseType[RunProto, "Run"], FileAttachmentsMixin): organization_id: str # NOTE: update() replaces this map wholesale. See TODO(metadata-mixin) in # sift_types/_mixins/metadata.py before adding keys at runtime. - metadata: dict[str, str | float | bool] + metadata: Metadata tags: list[str] asset_ids: list[str] is_adhoc: bool diff --git a/python/lib/sift_client/util/metadata.py b/python/lib/sift_client/util/metadata.py index bda406a91..33b61113f 100644 --- a/python/lib/sift_client/util/metadata.py +++ b/python/lib/sift_client/util/metadata.py @@ -1,5 +1,8 @@ from __future__ import annotations +from typing import TYPE_CHECKING, Any, Dict, Union + +from pydantic_core import core_schema from sift.metadata.v1.metadata_pb2 import ( MetadataKey, MetadataKeyType, @@ -8,6 +11,64 @@ MetadataValue as MetadataProto, ) +if TYPE_CHECKING: + from pydantic import GetCoreSchemaHandler + + +class Metadata(Dict[str, Union[str, float, bool]]): + """Entity metadata: a dict of key -> first value, plus access to every + value of a key via getall(). + + A metadata key may hold multiple values (multi-value metadata), kept in + canonical order. Plain dict access (md["k"], md.get("k"), iteration, ==) + sees the first value only -- the same value every single-value context + uses -- so existing code written against scalar metadata keeps working + unchanged. getall(key) returns the full ordered value list; its first + element is always the dict value. + """ + + def __init__( + self, + first_values: dict[str, str | float | bool] | None = None, + all_values: dict[str, list[str | float | bool]] | None = None, + ): + """Build the mapping from a first-value dict and optional full value lists.""" + super().__init__(first_values or {}) + self._all_values: dict[str, list[str | float | bool]] = { + key: list(values) for key, values in (all_values or {}).items() + } + + def getall(self, key: str) -> list[str | float | bool]: + """Return every value of ``key`` as a new list, in canonical order. + + Keys absent from the metadata yield []; keys holding a single value + yield a one-element list. + """ + if key in self._all_values: + return list(self._all_values[key]) + if key in self: + return [self[key]] + return [] + + @classmethod + def __get_pydantic_core_schema__( + cls, source_type: Any, handler: GetCoreSchemaHandler + ) -> core_schema.CoreSchema: + # Without this hook, pydantic validates ``Metadata`` fields as plain + # dicts and rebuilds them, dropping the extra values behind getall(). + # Pass instances through untouched; wrap plain mappings. + def _validate(value: Any) -> Metadata: + if isinstance(value, cls): + return value + if isinstance(value, dict): + return cls(value) + raise ValueError(f"expected a metadata mapping, got {type(value).__name__}") + + return core_schema.no_info_plain_validator_function( + _validate, + serialization=core_schema.plain_serializer_function_ser_schema(dict), + ) + def metadata_dict_to_proto(_metadata: dict[str, str | float | bool]) -> list[MetadataProto]: """Converts metadata dictionary into a list of MetadataValue objects. @@ -51,29 +112,35 @@ def metadata_dict_to_proto(_metadata: dict[str, str | float | bool]) -> list[Met return metadata -def metadata_proto_to_dict(metadata: list[MetadataProto]) -> dict[str, str | float | bool]: - """Converts a list of MetadataValue objects into a dictionary. +def metadata_proto_to_dict(metadata: list[MetadataProto]) -> Metadata: + """Converts a list of MetadataValue objects into a Metadata mapping. A key may appear multiple times when it holds multiple values - (multi-value metadata). The dictionary keeps the first value of each - key -- the API returns values in canonical order, so this matches the - value every other single-value context (e.g. backend flattening) uses. + (multi-value metadata). The mapping's dict view keeps the first value of + each key -- the API returns values in canonical order, so this matches + the value every other single-value context (e.g. backend flattening) + uses -- and ``Metadata.getall(key)`` returns the full ordered list. Args: metadata: List of MetadataValue objects. Returns: - Dictionary of metadata key-value pairs (first value per key). + Metadata mapping of key-value pairs (first value per key; all values + via getall). """ - unwrapped_metadata: dict[str, str | float | bool] = {} + first_values: dict[str, str | float | bool] = {} + all_values: dict[str, list[str | float | bool]] = {} for md in metadata: - if md.key.name in unwrapped_metadata: - continue + value: str | float | bool if md.key.type == MetadataKeyType.METADATA_KEY_TYPE_STRING: - unwrapped_metadata[md.key.name] = md.string_value + value = md.string_value elif md.key.type == MetadataKeyType.METADATA_KEY_TYPE_BOOLEAN: - unwrapped_metadata[md.key.name] = md.boolean_value + value = md.boolean_value elif md.key.type == MetadataKeyType.METADATA_KEY_TYPE_NUMBER: - unwrapped_metadata[md.key.name] = md.number_value + value = md.number_value + else: + continue + first_values.setdefault(md.key.name, value) + all_values.setdefault(md.key.name, []).append(value) - return unwrapped_metadata + return Metadata(first_values, all_values) From 540da190d3df8ff63ae34b1372562e7ea5cd9e22 Mon Sep 17 00:00:00 2001 From: Jon Deng Date: Wed, 29 Jul 2026 16:15:35 -0700 Subject: [PATCH 2/3] Python(fix): construct Metadata in channel test fixture for mypy Co-Authored-By: Claude Fable 5 --- python/lib/sift_client/_tests/sift_types/test_channel.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/lib/sift_client/_tests/sift_types/test_channel.py b/python/lib/sift_client/_tests/sift_types/test_channel.py index 226e8bcdc..c6e2b5e74 100644 --- a/python/lib/sift_client/_tests/sift_types/test_channel.py +++ b/python/lib/sift_client/_tests/sift_types/test_channel.py @@ -7,6 +7,7 @@ from sift_client.sift_types import Channel from sift_client.sift_types.channel import ChannelDataType, ChannelReference, ChannelUpdate +from sift_client.util.metadata import Metadata def _make_channel( @@ -26,7 +27,7 @@ def _make_channel( bit_field_elements=[], enum_types={}, asset_id="test_asset_id", - metadata={}, + metadata=Metadata(), is_archived=is_archived, created_date=datetime.now(timezone.utc), modified_date=datetime.now(timezone.utc), From 07eef3891669b261768da9007a3b434d0233b2d2 Mon Sep 17 00:00:00 2001 From: Jon Deng Date: Thu, 30 Jul 2026 17:32:51 -0700 Subject: [PATCH 3/3] Python(test): integration coverage for reading multi-value run metadata Seeds duplicate-key metadata through the raw UpdateRun proto (the public write path is scalar-only until ENG-13281 Phase 3) and asserts the full read-side contract: Metadata type, scalar first-value view, getall order, and the first-value invariant. Skips cleanly against backends without the multi-value-metadata flag so CI stays green until flag GA. Co-Authored-By: Claude Fable 5 --- .../sift_client/_tests/resources/test_runs.py | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/python/lib/sift_client/_tests/resources/test_runs.py b/python/lib/sift_client/_tests/resources/test_runs.py index 0de6bc04e..70ebe25ca 100644 --- a/python/lib/sift_client/_tests/resources/test_runs.py +++ b/python/lib/sift_client/_tests/resources/test_runs.py @@ -10,12 +10,19 @@ from datetime import datetime, timedelta, timezone import pytest +from google.protobuf.field_mask_pb2 import FieldMask +from grpc import StatusCode from grpc.aio import AioRpcError +from sift.metadata.v1.metadata_pb2 import METADATA_KEY_TYPE_STRING, MetadataKey, MetadataValue +from sift.runs.v2.runs_pb2 import Run as RunProto +from sift.runs.v2.runs_pb2 import UpdateRunRequest +from sift.runs.v2.runs_pb2_grpc import RunServiceStub from sift_client import SiftClient from sift_client.resources import RunsAPI, RunsAPIAsync from sift_client.sift_types import Run from sift_client.sift_types.run import RunCreate, RunUpdate +from sift_client.util.metadata import Metadata pytestmark = pytest.mark.integration @@ -393,6 +400,69 @@ async def test_update_with_run_id_string(self, runs_api_async, new_run): finally: await runs_api_async.archive(new_run.id_) + class TestMultiValueMetadata: + """Read-side tests for multi-value metadata. + + The public write path is scalar-only (list values land in ENG-13281 + Phase 3), so these tests seed duplicate-key metadata through the raw + `UpdateRun` proto. Storing multiple values per key requires the + backend's org-scoped `multi-value-metadata` flag; against a backend + without it the seed either fails or collapses to one value, and the + test skips instead of failing. + """ + + @pytest.mark.asyncio + async def test_get_run_exposes_all_values_via_getall( + self, sift_client, runs_api_async, new_run + ): + """Reading a run with duplicate-key metadata yields every value.""" + values = ["flux_capacitor", "lightsaber"] + request = UpdateRunRequest( + run=RunProto( + run_id=new_run.id_, + metadata=[ + MetadataValue( + key=MetadataKey(name="part_number", type=METADATA_KEY_TYPE_STRING), + string_value=value, + ) + for value in values + ], + ), + update_mask=FieldMask(paths=["metadata"]), + ) + try: + try: + response = await sift_client.grpc_client.get_stub(RunServiceStub).UpdateRun( + request + ) + except AioRpcError as exc: + if exc.code() == StatusCode.INVALID_ARGUMENT: + pytest.skip( + "backend rejected duplicate metadata keys; " + "multi-value-metadata flag not enabled" + ) + raise + stored = [ + entry.string_value + for entry in response.run.metadata + if entry.key.name == "part_number" + ] + if stored != values: + pytest.skip( + "backend did not store duplicate metadata keys; " + "multi-value-metadata flag not enabled" + ) + + run = await runs_api_async.get(run_id=new_run.id_) + metadata = run.metadata + assert isinstance(metadata, Metadata) + assert metadata["part_number"] == values[0] + assert metadata.getall("part_number") == values + assert metadata["part_number"] == metadata.getall("part_number")[0] + assert metadata.getall("missing") == [] + finally: + await runs_api_async.archive(new_run.id_) + class TestArchive: """Tests for the async archive method."""