Skip to content

Commit

Permalink
py: ctx: remove tags prop
Browse files Browse the repository at this point in the history
As discussed on [1], there's no need for a tags property just yet, and
there's also no agreed format for list metadata, so let's just get rid
of it.

[1]: https://github.com/opendatahub-io/model-registry/pull/260#discussion_r1441786749

Signed-off-by: Isabella Basso do Amaral <[email protected]>
  • Loading branch information
isinyaaa committed Jan 17, 2024
1 parent 7adacc2 commit 622ce61
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 26 deletions.
15 changes: 5 additions & 10 deletions clients/python/src/model_registry/types/contexts.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,13 @@ class ModelVersion(BaseContext, Prefixable):
description: Description of the object.
external_id: Customizable ID. Has to be unique among instances of the same type.
artifacts: Artifacts associated with this version.
tags: Tags associated with this version.
metadata: Metadata associated with this version.
"""

model_name: str
version: str
author: str
artifacts: list[BaseArtifact] = field(init=False, factory=list)
tags: list[str] = field(init=False, factory=list)
metadata: dict[str, ScalarType] = field(init=False, factory=dict)

_registered_model_id: str | None = field(init=False, default=None)
Expand All @@ -103,10 +101,11 @@ def mlmd_name_prefix(self) -> str:
def map(self, type_id: int) -> Context:
mlmd_obj = super().map(type_id)
# this should match the name of the registered model
mlmd_obj.properties["model_name"].string_value = self.model_name
mlmd_obj.properties["author"].string_value = self.author
if self.tags:
mlmd_obj.properties["tags"].string_value = ",".join(self.tags)
props = {
"model_name": self.model_name,
"author": self.author,
}
self._map_props(props, mlmd_obj.properties)
self._map_props(self.metadata, mlmd_obj.custom_properties)
return mlmd_obj

Expand All @@ -120,10 +119,6 @@ def unmap(cls, mlmd_obj: Context) -> ModelVersion:
py_obj.version = py_obj.name
py_obj.model_name = mlmd_obj.properties["model_name"].string_value
py_obj.author = mlmd_obj.properties["author"].string_value
tags = mlmd_obj.properties["tags"].string_value
if tags:
tags = tags.split(",")
py_obj.tags = tags or []
py_obj.metadata = cls._unmap_props(mlmd_obj.custom_properties)
return py_obj

Expand Down
41 changes: 29 additions & 12 deletions clients/python/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,36 @@

ProtoTypeType = Union[ArtifactType, ContextType]


# ruff: noqa: PT021 supported
@pytest.fixture(scope="session")
def mlmd_conn(request) -> MetadataStoreClientConfig:
model_registry_root_dir = model_registry_root(request)
print("Assuming this is the Model Registry root directory:", model_registry_root_dir)
print(
"Assuming this is the Model Registry root directory:", model_registry_root_dir
)
shared_volume = model_registry_root_dir / "test/config/ml-metadata"
sqlite_db_file = shared_volume / "metadata.sqlite.db"
if sqlite_db_file.exists():
msg = f"The file {sqlite_db_file} already exists; make sure to cancel it before running these tests."
raise FileExistsError(msg)
container = DockerContainer("gcr.io/tfx-oss-public/ml_metadata_store_server:1.14.0")
container.with_exposed_ports(8080)
container.with_volume_mapping(shared_volume, "/tmp/shared", "rw") # noqa this is file target in container
container.with_env("METADATA_STORE_SERVER_CONFIG_FILE", "/tmp/shared/conn_config.pb") # noqa this is target in container
container.with_volume_mapping(
shared_volume,
"/tmp/shared", # noqa: S108
"rw",
)
container.with_env(
"METADATA_STORE_SERVER_CONFIG_FILE",
"/tmp/shared/conn_config.pb", # noqa: S108
)
container.start()
wait_for_logs(container, "Server listening on")
os.system('docker container ls --format "table {{.ID}}\t{{.Names}}\t{{.Ports}}" -a') # noqa governed test
os.system('docker container ls --format "table {{.ID}}\t{{.Names}}\t{{.Ports}}" -a') # noqa governed test
print("waited for logs and port")
cfg = MetadataStoreClientConfig(
host = "localhost",
port = int(container.get_exposed_port(8080))
host="localhost", port=int(container.get_exposed_port(8080))
)
print(cfg)

Expand All @@ -51,7 +60,9 @@ def teardown():

request.addfinalizer(teardown)

time.sleep(3) # allowing some time for mlmd grpc to fully stabilize (is "spent" once per pytest session anyway)
time.sleep(
3
) # allowing some time for mlmd grpc to fully stabilize (is "spent" once per pytest session anyway)
_throwaway_store = metadata_store.MetadataStore(cfg)
wait_for_grpc(container, _throwaway_store)

Expand All @@ -64,7 +75,10 @@ def model_registry_root(request):

@pytest.fixture()
def plain_wrapper(request, mlmd_conn: MetadataStoreClientConfig) -> MLMDStore:
sqlite_db_file = model_registry_root(request) / "test/config/ml-metadata/metadata.sqlite.db"
sqlite_db_file = (
model_registry_root(request) / "test/config/ml-metadata/metadata.sqlite.db"
)

def teardown():
try:
os.remove(sqlite_db_file)
Expand Down Expand Up @@ -110,7 +124,6 @@ def store_wrapper(plain_wrapper: MLMDStore) -> MLMDStore:
"description",
"model_name",
"state",
"tags",
],
)

Expand All @@ -137,7 +150,12 @@ def mr_api(store_wrapper: MLMDStore) -> ModelRegistryAPIClient:
return mr


def wait_for_grpc(container: DockerContainer, store: metadata_store.MetadataStore, timeout=6, interval=2):
def wait_for_grpc(
container: DockerContainer,
store: metadata_store.MetadataStore,
timeout=6,
interval=2,
):
start = time.time()
while True:
duration = time.time() - start
Expand All @@ -151,6 +169,5 @@ def wait_for_grpc(container: DockerContainer, store: metadata_store.MetadataStor
if results is not None:
return duration
if timeout and duration > timeout:
raise TimeoutError("wait_for_grpc not ready %.3f seconds"
% timeout)
raise TimeoutError("wait_for_grpc not ready %.3f seconds" % timeout)
time.sleep(interval)
4 changes: 0 additions & 4 deletions clients/python/tests/types/test_context_mapping.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ def full_model_version() -> Mapped:
proto_version.properties["description"].string_value = "test description"
proto_version.properties["model_name"].string_value = "test_model"
proto_version.properties["author"].string_value = "test_author"
proto_version.properties["tags"].string_value = "test_tag1,test_tag2"
proto_version.properties["state"].string_value = "ARCHIVED"
proto_version.custom_properties["int_key"].int_value = 1
proto_version.custom_properties["float_key"].double_value = 1.0
Expand All @@ -35,7 +34,6 @@ def full_model_version() -> Mapped:
description="test description",
)
py_version._registered_model_id = "1"
py_version.tags = ["test_tag1", "test_tag2"]
py_version.metadata = {
"int_key": 1,
"float_key": 1.0,
Expand Down Expand Up @@ -75,7 +73,6 @@ def test_partial_model_version_unmapping(minimal_model_version: Mapped):
assert unmapped_version.model_name == py_version.model_name
assert unmapped_version.author == py_version.author
assert unmapped_version.state == py_version.state
assert unmapped_version.tags == py_version.tags
assert unmapped_version.metadata == py_version.metadata


Expand All @@ -98,5 +95,4 @@ def test_full_model_version_unmapping(full_model_version: Mapped):
assert unmapped_version.model_name == py_version.model_name
assert unmapped_version.author == py_version.author
assert unmapped_version.state == py_version.state
assert unmapped_version.tags == py_version.tags
assert unmapped_version.metadata == py_version.metadata

0 comments on commit 622ce61

Please sign in to comment.