restrict captures to prefix when mapping *FromOwned (#295)

* converter: simplify MapName Signed-off-by: Isabella do Amaral <[email protected]> * converter: restrict captures to prefix when mapping *FromOwned Fixes: #255 Signed-off-by: Isabella do Amaral <[email protected]> * py: make: fix build-mr tagging Signed-off-by: Isabella do Amaral <[email protected]> * py: tests: introduce regression testing Signed-off-by: Isabella do Amaral <[email protected]> * py: make: expose IMG_* env vars Signed-off-by: Isabella do Amaral <[email protected]> --------- Signed-off-by: Isabella do Amaral <[email protected]>
kubeflow · Sep 2, 2024 · 969056f · 969056f
1 parent 332c085
commit 969056f
Show file tree

Hide file tree

Showing 6 changed files with 54 additions and 20 deletions.
diff --git a/clients/python/Makefile b/clients/python/Makefile
@@ -1,5 +1,8 @@
 all: install tidy
 
+IMG_REGISTRY ?= docker.io
+IMG_VERSION ?= latest 
+
 .PHONY: install
 install:
 	../../bin/openapi-generator-cli generate -i ../../api/openapi/model-registry.yaml -g python -o src/ --package-name mr_openapi --additional-properties=library=asyncio,generateSourceCodeOnly=true,useOneOfDiscriminatorLookup=true
@@ -13,7 +16,7 @@ clean:
 
 .PHONY: build-mr
 build-mr:
-	cd ../../ && make image/build
+	cd ../../ && IMG_REGISTRY=${IMG_REGISTRY} IMG_VERSION=${IMG_VERSION} make image/build
 
 .PHONY: test-e2e
 test-e2e: build-mr

diff --git a/clients/python/tests/conftest.py b/clients/python/tests/conftest.py
@@ -10,6 +10,8 @@
 import pytest
 import requests
 
+from model_registry import ModelRegistry
+
 
 def pytest_addoption(parser):
     parser.addoption("--e2e", action="store_true", help="run end-to-end tests")
@@ -71,7 +73,7 @@ def _compose_mr(root):
         raise FileExistsError(msg)
     print(f" Starting Docker Compose in folder {root}")
     p = subprocess.Popen(  # noqa: S602
-        f"{DOCKER} compose -f {COMPOSE_FILE} up --build",
+        f"{DOCKER} compose -f {COMPOSE_FILE} up",
         shell=True,
         cwd=root,
     )
@@ -125,3 +127,9 @@ def event_loop():
     loop = asyncio.get_event_loop_policy().get_event_loop()
     yield loop
     loop.close()
+
+
+@pytest.fixture
+@cleanup
+def client() -> ModelRegistry:
+    return ModelRegistry(REGISTRY_HOST, REGISTRY_PORT, author="author", is_secure=False)
diff --git a/clients/python/tests/regression_test.py b/clients/python/tests/regression_test.py
@@ -0,0 +1,24 @@
+import pytest
+
+from model_registry import ModelRegistry
+
+
+@pytest.mark.e2e
+def test_create_tagged_version(client: ModelRegistry):
+    """Test regression for creating tagged versions.
+
+    Reported on: https://github.com/kubeflow/model-registry/issues/255
+    """
+    name = "test_model"
+    version = "model:latest"
+    rm = client.register_model(
+        name,
+        "s3",
+        model_format_name="test_format",
+        model_format_version="test_version",
+        version=version,
+    )
+    assert rm.id
+    mv = client.get_model_version(name, version)
+    assert mv
+    assert mv.id
diff --git a/clients/python/tests/test_client.py b/clients/python/tests/test_client.py
@@ -6,14 +6,6 @@
 from model_registry import ModelRegistry, utils
 from model_registry.exceptions import StoreError
 
-from .conftest import REGISTRY_HOST, REGISTRY_PORT, cleanup
-
-
-@pytest.fixture
-@cleanup
-def client() -> ModelRegistry:
-    return ModelRegistry(REGISTRY_HOST, REGISTRY_PORT, author="author", is_secure=False)
-
 
 def test_secure_client():
     os.environ["CERT"] = ""

diff --git a/internal/converter/mlmd_converter_util_test.go b/internal/converter/mlmd_converter_util_test.go
@@ -185,6 +185,14 @@ func TestPrefixWhenOwnedWithoutOwner(t *testing.T) {
 	assertion.Equal("name", strings.Split(prefixed, ":")[1])
 }
 
+func TestPrefixNameWithColonWhenOwned(t *testing.T) {
+	assertion := setup(t)
+
+	owner := "owner"
+	entity := "name:with:colon"
+	assertion.Equal("owner:name:with:colon", PrefixWhenOwned(&owner, entity))
+}
+
 func TestMapRegisteredModelProperties(t *testing.T) {
 	assertion := setup(t)
 
@@ -578,7 +586,7 @@ func TestMapNameFromOwned(t *testing.T) {
 	assertion.Equal("name", *name)
 
 	name = MapNameFromOwned(of("prefix:name:postfix"))
-	assertion.Equal("name", *name)
+	assertion.Equal("name:postfix", *name)
 
 	name = MapNameFromOwned(nil)
 	assertion.Nil(name)
@@ -594,8 +602,9 @@ func TestMapRegisteredModelIdFromOwned(t *testing.T) {
 	_, err = MapRegisteredModelIdFromOwned(of("name"))
 	assertion.NotNil(err)
 
-	_, err = MapRegisteredModelIdFromOwned(of("prefix:name:postfix"))
-	assertion.NotNil(err)
+	result, err = MapRegisteredModelIdFromOwned(of("prefix:name:postfix"))
+	assertion.Nil(err)
+	assertion.Equal("prefix", result)
 
 	result, err = MapRegisteredModelIdFromOwned(nil)
 	assertion.Nil(err)

diff --git a/internal/converter/mlmd_openapi_converter_util.go b/internal/converter/mlmd_openapi_converter_util.go
@@ -97,7 +97,9 @@ func MapNameFromOwned(source *string) *string {
 	if len(exploded) == 1 {
 		return source
 	}
-	return &exploded[1]
+	// cat remaining parts of the exploded string
+	joined := strings.Join(exploded[1:], ":")
+	return &joined
 }
 
 // MapName derive the entity name from the mlmd fullname
@@ -107,11 +109,7 @@ func MapName(source *string) string {
 		return ""
 	}
 
-	exploded := strings.Split(*source, ":")
-	if len(exploded) == 1 {
-		return *source
-	}
-	return exploded[1]
+	return *MapNameFromOwned(source)
 }
 
 // REGISTERED MODEL
@@ -128,7 +126,7 @@ func MapRegisteredModelIdFromOwned(source *string) (string, error) {
 	}
 
 	exploded := strings.Split(*source, ":")
-	if len(exploded) != 2 {
+	if len(exploded) < 2 {
 		return "", fmt.Errorf("wrong owned format")
 	}
 	return exploded[0], nil