diff --git a/python/lib/sift_client/_internal/low_level_wrappers/runs.py b/python/lib/sift_client/_internal/low_level_wrappers/runs.py index 12d5b8a75..d8691d00d 100644 --- a/python/lib/sift_client/_internal/low_level_wrappers/runs.py +++ b/python/lib/sift_client/_internal/low_level_wrappers/runs.py @@ -186,7 +186,7 @@ async def create_adhoc_run( start_time: datetime | None = None, stop_time: datetime | None = None, tag_names: list[str] | None = None, - metadata: dict[str, str | float | bool] | None = None, + metadata: dict[str, str | float | bool | list[str]] | None = None, client_key: str | None = None, ) -> Run: """Create an adhoc run. diff --git a/python/lib/sift_client/_tests/resources/test_runs.py b/python/lib/sift_client/_tests/resources/test_runs.py index 70ebe25ca..cc0eb7bd7 100644 --- a/python/lib/sift_client/_tests/resources/test_runs.py +++ b/python/lib/sift_client/_tests/resources/test_runs.py @@ -8,6 +8,7 @@ """ from datetime import datetime, timedelta, timezone +from typing import NoReturn import pytest from google.protobuf.field_mask_pb2 import FieldMask @@ -401,16 +402,24 @@ async def test_update_with_run_id_string(self, runs_api_async, new_run): 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. + """Multi-value metadata tests: raw-proto seeding, public list writes, + and read-side getall. + + 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 tests skip instead + of failing. """ + @staticmethod + def _skip_if_flag_disabled(exc: AioRpcError) -> NoReturn: + if exc.code() == StatusCode.INVALID_ARGUMENT: + pytest.skip( + "backend rejected duplicate metadata keys; " + "multi-value-metadata flag not enabled" + ) + raise exc + @pytest.mark.asyncio async def test_get_run_exposes_all_values_via_getall( self, sift_client, runs_api_async, new_run @@ -436,12 +445,7 @@ async def test_get_run_exposes_all_values_via_getall( 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 + self._skip_if_flag_disabled(exc) stored = [ entry.string_value for entry in response.run.metadata @@ -463,6 +467,47 @@ async def test_get_run_exposes_all_values_via_getall( finally: await runs_api_async.archive(new_run.id_) + @pytest.mark.asyncio + async def test_update_run_with_list_metadata(self, runs_api_async, new_run): + """A list[str] value in RunUpdate writes every element under the key.""" + values = ["flux_capacitor", "lightsaber"] + try: + update = RunUpdate(metadata={"part_number": values, "env": "prod"}) + try: + updated_run = await runs_api_async.update(new_run, update) + except AioRpcError as exc: + self._skip_if_flag_disabled(exc) + + assert updated_run.metadata["part_number"] == values[0] + assert updated_run.metadata.getall("part_number") == values + assert updated_run.metadata["env"] == "prod" + + fetched = await runs_api_async.get(run_id=new_run.id_) + assert fetched.metadata.getall("part_number") == values + finally: + await runs_api_async.archive(new_run.id_) + + @pytest.mark.asyncio + async def test_metadata_round_trip_preserves_all_values(self, runs_api_async, new_run): + """update(metadata=run.metadata) must not drop multi-value entries.""" + values = ["flux_capacitor", "lightsaber"] + try: + try: + await runs_api_async.update( + new_run, RunUpdate(metadata={"part_number": values}) + ) + except AioRpcError as exc: + self._skip_if_flag_disabled(exc) + + fetched = await runs_api_async.get(run_id=new_run.id_) + # Write the fetched Metadata mapping back unchanged. + round_tripped = await runs_api_async.update( + new_run, RunUpdate(metadata=fetched.metadata) + ) + assert round_tripped.metadata.getall("part_number") == values + finally: + await runs_api_async.archive(new_run.id_) + class TestArchive: """Tests for the async archive method.""" diff --git a/python/lib/sift_client/_tests/sift_types/test_asset.py b/python/lib/sift_client/_tests/sift_types/test_asset.py index 23d49970d..d3b6f138c 100644 --- a/python/lib/sift_client/_tests/sift_types/test_asset.py +++ b/python/lib/sift_client/_tests/sift_types/test_asset.py @@ -7,6 +7,7 @@ from sift_client.sift_types import Asset from sift_client.sift_types.asset import AssetUpdate +from sift_client.util.metadata import Metadata class TestAssetUpdate: @@ -31,6 +32,33 @@ def test_metadata_converter(self): assert metadata_dict["key3"].boolean_value is True assert "metadata" in mask.paths + def test_metadata_list_value_writes_every_element(self): + """A list[str] value produces one MetadataValue per element, in order.""" + update = AssetUpdate(metadata={"parts": ["ABC", "XYZ"], "env": "prod"}) + update.resource_id = "test_asset_id" + + proto, mask = update.to_proto_with_mask() + + assert [md.string_value for md in proto.metadata if md.key.name == "parts"] == [ + "ABC", + "XYZ", + ] + assert [md.string_value for md in proto.metadata if md.key.name == "env"] == ["prod"] + assert "metadata" in mask.paths + + def test_metadata_mapping_round_trips_all_values(self): + """Passing an entity's Metadata mapping must not drop multi-value entries.""" + metadata = Metadata({"parts": "ABC"}, {"parts": ["ABC", "XYZ"]}) + update = AssetUpdate(metadata=metadata) + update.resource_id = "test_asset_id" + + proto, _ = update.to_proto_with_mask() + + assert [md.string_value for md in proto.metadata if md.key.name == "parts"] == [ + "ABC", + "XYZ", + ] + @pytest.fixture def mock_asset(mock_client): diff --git a/python/lib/sift_client/_tests/sift_types/test_run.py b/python/lib/sift_client/_tests/sift_types/test_run.py index 64c6b8cad..e05f5fb08 100644 --- a/python/lib/sift_client/_tests/sift_types/test_run.py +++ b/python/lib/sift_client/_tests/sift_types/test_run.py @@ -7,6 +7,7 @@ from sift_client.sift_types import Run from sift_client.sift_types.run import RunCreate, RunUpdate +from sift_client.util.metadata import Metadata class TestRunCreate: @@ -26,6 +27,24 @@ def test_metadata_converter(self): assert metadata_dict["number_key"].number_value == 3.14 assert metadata_dict["bool_key"].boolean_value is True + def test_metadata_list_value_writes_every_element(self): + """A list[str] value produces one MetadataValue per element, in order.""" + create = RunCreate(name="test_run", metadata={"parts": ["ABC", "XYZ"], "env": "prod"}) + proto = create.to_proto() + + assert [md.string_value for md in proto.metadata if md.key.name == "parts"] == [ + "ABC", + "XYZ", + ] + assert [md.string_value for md in proto.metadata if md.key.name == "env"] == ["prod"] + + def test_rust_form_rejects_list_metadata(self): + """The streaming ingestion path does not support multi-value metadata yet.""" + pytest.importorskip("sift_stream_bindings") + create = RunCreate(name="test_run", metadata={"parts": ["ABC", "XYZ"]}) + with pytest.raises(ValueError, match="streaming ingestion path"): + create._to_rust_form() + def test_time_validator_start_before_stop(self): """Test time validator accepts start_time before stop_time.""" start_time = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc) @@ -74,6 +93,34 @@ def test_metadata_converter(self): assert metadata_dict["key3"].boolean_value is False assert "metadata" in mask.paths + def test_metadata_list_value_writes_every_element(self): + """A list[str] value produces one MetadataValue per element, in order.""" + update = RunUpdate(metadata={"parts": ["ABC", "XYZ"], "env": "prod"}) + update.resource_id = "test_run_id" + + proto, mask = update.to_proto_with_mask() + + assert [md.string_value for md in proto.metadata if md.key.name == "parts"] == [ + "ABC", + "XYZ", + ] + assert [md.string_value for md in proto.metadata if md.key.name == "env"] == ["prod"] + assert "metadata" in mask.paths + + def test_metadata_mapping_round_trips_all_values(self): + """Passing an entity's Metadata mapping must not drop multi-value entries.""" + metadata = Metadata({"parts": "ABC", "env": "prod"}, {"parts": ["ABC", "XYZ"]}) + update = RunUpdate(metadata=metadata) + update.resource_id = "test_run_id" + + proto, _ = update.to_proto_with_mask() + + assert [md.string_value for md in proto.metadata if md.key.name == "parts"] == [ + "ABC", + "XYZ", + ] + assert [md.string_value for md in proto.metadata if md.key.name == "env"] == ["prod"] + def test_time_validator_start_before_stop(self): """Test time validator accepts start_time before stop_time.""" start_time = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc) diff --git a/python/lib/sift_client/_tests/util/test_metadata.py b/python/lib/sift_client/_tests/util/test_metadata.py index 2e0ce6e3e..bb8f63477 100644 --- a/python/lib/sift_client/_tests/util/test_metadata.py +++ b/python/lib/sift_client/_tests/util/test_metadata.py @@ -8,7 +8,12 @@ MetadataValue, ) -from sift_client.util.metadata import Metadata, metadata_dict_to_proto, metadata_proto_to_dict +from sift_client.util.metadata import ( + Metadata, + expand_metadata_for_write, + metadata_dict_to_proto, + metadata_proto_to_dict, +) def _string_value(name: str, value: str) -> MetadataValue: @@ -70,6 +75,60 @@ def test_round_trip_from_dict(self): assert metadata_proto_to_dict(metadata_dict_to_proto(original)) == original +class TestMetadataDictToProto: + def test_list_value_writes_one_proto_per_element_in_order(self): + protos = metadata_dict_to_proto({"parts": ["ABC", "XYZ"], "env": "prod"}) + parts = [p.string_value for p in protos if p.key.name == "parts"] + assert parts == ["ABC", "XYZ"] + assert all( + p.key.type == MetadataKeyType.METADATA_KEY_TYPE_STRING + for p in protos + if p.key.name == "parts" + ) + assert [p.string_value for p in protos if p.key.name == "env"] == ["prod"] + + def test_list_round_trips_through_proto(self): + protos = metadata_dict_to_proto({"parts": ["ABC", "XYZ"], "build": 1.5}) + result = metadata_proto_to_dict(protos) + assert result == {"parts": "ABC", "build": 1.5} + assert result.getall("parts") == ["ABC", "XYZ"] + + def test_metadata_instance_keeps_all_values(self): + md = Metadata({"parts": "ABC", "env": "prod"}, {"parts": ["ABC", "XYZ"]}) + protos = metadata_dict_to_proto(md) + assert [p.string_value for p in protos if p.key.name == "parts"] == ["ABC", "XYZ"] + assert [p.string_value for p in protos if p.key.name == "env"] == ["prod"] + + def test_empty_list_rejected(self): + with pytest.raises(ValueError, match="empty value list"): + metadata_dict_to_proto({"parts": []}) + + def test_non_string_list_element_rejected(self): + with pytest.raises(ValueError, match="only string values may be multi-value"): + metadata_dict_to_proto({"parts": ["ABC", 2]}) # type: ignore[list-item] + + def test_unsupported_scalar_rejected(self): + with pytest.raises(ValueError, match="Unsupported metadata value type"): + metadata_dict_to_proto({"parts": None}) # type: ignore[dict-item] + + +class TestExpandMetadataForWrite: + def test_multi_value_keys_become_lists_single_stay_scalar(self): + md = Metadata( + {"parts": "ABC", "env": "prod", "build": 1.5}, + {"parts": ["ABC", "XYZ"], "env": ["prod"]}, + ) + assert expand_metadata_for_write(md) == { + "parts": ["ABC", "XYZ"], + "env": "prod", + "build": 1.5, + } + + def test_plain_dict_returned_unchanged(self): + plain = {"parts": ["ABC", "XYZ"], "env": "prod"} + assert expand_metadata_for_write(plain) is plain + + class TestMetadataMapping: def test_dict_view_and_getall_invariant(self): md = Metadata( diff --git a/python/lib/sift_client/sift_types/asset.py b/python/lib/sift_client/sift_types/asset.py index 1a3b82c32..27572456d 100644 --- a/python/lib/sift_client/sift_types/asset.py +++ b/python/lib/sift_client/sift_types/asset.py @@ -3,12 +3,18 @@ from datetime import datetime, timezone from typing import TYPE_CHECKING, ClassVar +from pydantic import field_validator from sift.assets.v1.assets_pb2 import Asset as AssetProto 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, metadata_dict_to_proto, metadata_proto_to_dict +from sift_client.util.metadata import ( + Metadata, + expand_metadata_for_write, + metadata_dict_to_proto, + metadata_proto_to_dict, +) if TYPE_CHECKING: from sift_client.client import SiftClient @@ -114,9 +120,21 @@ class AssetUpdate(ModelUpdate[AssetProto]): """Model of the Asset Fields that can be updated.""" tags: list[str | Tag] | None = None - metadata: dict[str, str | float | bool] | None = None + # A list[str] value writes every element under the same key (multi-value + # metadata, string keys only); writes REPLACE the full metadata map. + metadata: dict[str, str | float | bool | list[str]] | None = None is_archived: bool | None = None + @field_validator("metadata", mode="before") + @classmethod + def _expand_metadata(cls, value): + # A Metadata mapping hides values beyond the first behind getall(); + # expand it before validation so pydantic's dict rebuild (and the + # model_dump that feeds proto conversion) can't drop them. + if isinstance(value, Metadata): + return expand_metadata_for_write(value) + return value + _to_proto_helpers: ClassVar[dict[str, MappingHelper]] = { "metadata": MappingHelper( proto_attr_path="metadata", diff --git a/python/lib/sift_client/sift_types/run.py b/python/lib/sift_client/sift_types/run.py index 689341e6b..a44ea8831 100644 --- a/python/lib/sift_client/sift_types/run.py +++ b/python/lib/sift_client/sift_types/run.py @@ -3,7 +3,7 @@ from datetime import datetime, timedelta, timezone from typing import TYPE_CHECKING, ClassVar -from pydantic import model_validator +from pydantic import field_validator, model_validator from sift.runs.v2.runs_pb2 import CreateRunRequest as CreateRunRequestProto from sift.runs.v2.runs_pb2 import Run as RunProto @@ -16,7 +16,12 @@ ) 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, metadata_dict_to_proto, metadata_proto_to_dict +from sift_client.util.metadata import ( + Metadata, + expand_metadata_for_write, + metadata_dict_to_proto, + metadata_proto_to_dict, +) if TYPE_CHECKING: from pathlib import Path @@ -178,7 +183,19 @@ class RunBase(ModelCreateUpdateBase): start_time: datetime | None = None stop_time: datetime | None = None tags: list[str] | list[Tag] | None = None - metadata: dict[str, str | float | bool] | None = None + # A list[str] value writes every element under the same key (multi-value + # metadata, string keys only); writes REPLACE the full metadata map. + metadata: dict[str, str | float | bool | list[str]] | None = None + + @field_validator("metadata", mode="before") + @classmethod + def _expand_metadata(cls, value): + # A Metadata mapping hides values beyond the first behind getall(); + # expand it before validation so pydantic's dict rebuild (and the + # model_dump that feeds proto conversion) can't drop them. + if isinstance(value, Metadata): + return expand_metadata_for_write(value) + return value _to_proto_helpers: ClassVar[dict[str, MappingHelper]] = { "metadata": MappingHelper( @@ -233,6 +250,11 @@ def _to_rust_form(self) -> RunFormPy: if self.metadata: metadata = [] for key, value in self.metadata.items(): + if isinstance(value, list): + raise ValueError( + f"Metadata key '{key}': multi-value metadata is not supported " + "on the streaming ingestion path yet; use a scalar value" + ) metadata.append(MetadataPy(key=key, value=MetadataValuePy(value))) else: metadata = None diff --git a/python/lib/sift_client/util/metadata.py b/python/lib/sift_client/util/metadata.py index 33b61113f..75ea474f2 100644 --- a/python/lib/sift_client/util/metadata.py +++ b/python/lib/sift_client/util/metadata.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Any, Dict, Union +from typing import TYPE_CHECKING, Any, Dict, Mapping, Union from pydantic_core import core_schema from sift.metadata.v1.metadata_pb2 import ( @@ -70,44 +70,98 @@ def _validate(value: Any) -> Metadata: ) -def metadata_dict_to_proto(_metadata: dict[str, str | float | bool]) -> list[MetadataProto]: +def expand_metadata_for_write( + metadata: Mapping[str, str | float | bool | list[str]], +) -> Mapping[str, str | float | bool | list[str]]: + """Expand a ``Metadata`` mapping into a plain write dict, keeping every value. + + ``Metadata`` hides values beyond the first behind ``getall()``; pydantic + write models serialize fields with ``model_dump``, which would flatten the + mapping to its first-value dict view and silently drop the rest. Calling + this in a before-validator keeps round-trips (``update.metadata = + entity.metadata``) lossless: single-value keys stay scalars, multi-value + keys become lists. + + Any other mapping is returned unchanged. + """ + if not isinstance(metadata, Metadata): + return metadata + expanded: dict[str, str | float | bool | list[str]] = {} + for key in metadata: + values = metadata.getall(key) + if len(values) == 1: + expanded[key] = values[0] + else: + # Multi-value keys are string-typed (backend enforces this), so + # the cast to list[str] holds for any server-produced mapping. + expanded[key] = [str(value) for value in values] + return expanded + + +def _scalar_metadata_proto(key: str, value: str | float | bool) -> MetadataProto: + """Wrap one scalar metadata value in a MetadataValue proto.""" + metadata_key_type = MetadataKeyType.METADATA_KEY_TYPE_UNSPECIFIED + string_value = None + boolean_value = None + number_value = None + + if isinstance(value, str): + string_value = value + metadata_key_type = MetadataKeyType.METADATA_KEY_TYPE_STRING + elif isinstance(value, bool): + # Need to check bool before int since python thinks "True" is an int + boolean_value = value + metadata_key_type = MetadataKeyType.METADATA_KEY_TYPE_BOOLEAN + elif isinstance(value, (int, float)): + number_value = value + metadata_key_type = MetadataKeyType.METADATA_KEY_TYPE_NUMBER + else: + raise ValueError(f"Unsupported metadata value type for key '{key}': {value}") + + wrapped_key = MetadataKey(name=key, type=metadata_key_type) + return MetadataProto( + key=wrapped_key, + string_value=string_value, # type: ignore + boolean_value=boolean_value, # type: ignore + number_value=number_value, # type: ignore + ) + + +def metadata_dict_to_proto( + _metadata: Mapping[str, str | float | bool | list[str]], +) -> list[MetadataProto]: """Converts metadata dictionary into a list of MetadataValue objects. + A value may be a scalar (``str | float | bool``) or a ``list[str]``. A + list produces one MetadataValue per element under the same key, in list + order -- the canonical order for multi-value metadata. Only string values + may repeat (mirroring the backend rule), so list elements must be + strings, and a list must not be empty: replace semantics mean a key is + removed by omitting it, not by writing an empty list. + Args: _metadata: Dictionary of metadata key-value pairs. Returns: List of MetadataValue objects. """ + _metadata = expand_metadata_for_write(_metadata) metadata = [] for key, value in _metadata.items(): - metadata_key_type = MetadataKeyType.METADATA_KEY_TYPE_UNSPECIFIED - string_value = None - boolean_value = None - number_value = None - - if isinstance(value, str): - string_value = value - metadata_key_type = MetadataKeyType.METADATA_KEY_TYPE_STRING - elif isinstance(value, bool): - # Need to check bool before int since python thinks "True" is an int - boolean_value = value - metadata_key_type = MetadataKeyType.METADATA_KEY_TYPE_BOOLEAN - elif isinstance(value, (int, float)): - number_value = value - metadata_key_type = MetadataKeyType.METADATA_KEY_TYPE_NUMBER + if isinstance(value, list): + if not value: + raise ValueError( + f"Metadata key '{key}' has an empty value list; omit the key to remove it" + ) + for item in value: + if not isinstance(item, str): + raise ValueError( + f"Metadata key '{key}': only string values may be multi-value, got {item!r}" + ) + metadata.append(_scalar_metadata_proto(key, item)) else: - raise ValueError(f"Unsupported metadata value type for key '{key}': {value}") - - wrapped_key = MetadataKey(name=key, type=metadata_key_type) - wrapped_value = MetadataProto( - key=wrapped_key, - string_value=string_value, # type: ignore - boolean_value=boolean_value, # type: ignore - number_value=number_value, # type: ignore - ) - metadata.append(wrapped_value) + metadata.append(_scalar_metadata_proto(key, value)) return metadata