Merge pull request #32 from tarilabs/tarilabs-20240326-sync

periodic sync upstream KF to midstream ODH

.github/workflows/build-and-push-image.yml

@@ -14,10 +14,10 @@ on:
       - '.github/dependabot.yml'
       - 'docs/**'
 env:
-  QUAY_ORG: opendatahub
-  QUAY_IMG_REPO: model-registry
-  QUAY_USERNAME: ${{ secrets.QUAY_USERNAME }}
-  QUAY_PASSWORD: ${{ secrets.QUAY_PASSWORD }}
+  IMG_ORG: opendatahub
+  IMG_REPO: model-registry
+  DOCKER_USER: ${{ secrets.QUAY_USERNAME }}
+  DOCKER_PWD: ${{ secrets.QUAY_PASSWORD }}
   PUSH_IMAGE: true
 jobs:
   build-image:
@@ -50,8 +50,8 @@ jobs:
         if: env.BUILD_CONTEXT == 'main'
         shell: bash
         env:
-          IMG: quay.io/${{ env.QUAY_ORG }}/${{ env.QUAY_IMG_REPO }}
-          BUILD_IMAGE: false
+          IMG: quay.io/${{ env.IMG_ORG }}/${{ env.IMG_REPO }}
+          BUILD_IMAGE: false # image is already built in "Build and Push Image" step
         run: |
           docker tag ${{ env.IMG }}:$VERSION ${{ env.IMG }}:latest
           # BUILD_IMAGE=false skip the build, just push the tag made above
@@ -60,8 +60,8 @@ jobs:
         if: env.BUILD_CONTEXT == 'main'
         shell: bash
         env:
-          IMG: quay.io/${{ env.QUAY_ORG }}/${{ env.QUAY_IMG_REPO }}
-          BUILD_IMAGE: false
+          IMG: quay.io/${{ env.IMG_ORG }}/${{ env.IMG_REPO }}
+          BUILD_IMAGE: false # image is already built in "Build and Push Image" step
         run: |
           docker tag ${{ env.IMG }}:$VERSION ${{ env.IMG }}:main
           # BUILD_IMAGE=false skip the build, just push the tag made above

.github/workflows/build-image-pr.yml

@@ -11,7 +11,8 @@ on:
       - 'docs/**'
       - 'clients/python/**'
 env:
-  QUAY_IMG_REPO: model-registry
+  IMG_ORG: opendatahub
+  IMG_REPO: model-registry
   PUSH_IMAGE: false
   BRANCH: ${{ github.base_ref }}
 jobs:
@@ -35,12 +36,12 @@ jobs:
         uses: helm/[email protected]
       - name: Load Local Registry Test Image
         env:
-          IMG: "quay.io/opendatahub/model-registry:${{ steps.tags.outputs.tag }}"
+          IMG: "quay.io/${{ env.IMG_ORG }}/${{ env.IMG_REPO }}:${{ steps.tags.outputs.tag }}"
         run: |
           kind load docker-image -n chart-testing ${IMG}
       - name: Deploy Operator With Test Image
         env:
-          IMG: "quay.io/opendatahub/model-registry:${{ steps.tags.outputs.tag }}"
+          IMG: "quay.io/${{ env.IMG_ORG }}/${{ env.IMG_REPO }}:${{ steps.tags.outputs.tag }}"
         run: |
           echo "Deploying operator from model-registry-operator branch ${BRANCH}"
           kubectl apply -k "https://github.com/opendatahub-io/model-registry-operator.git/config/default?ref=${BRANCH}"

Makefile

@@ -12,7 +12,7 @@ MLMD_VERSION ?= 1.14.0
 DOCKER ?= docker
 # default Dockerfile
 DOCKERFILE ?= Dockerfile
-# container registry
+# container registry, default to empty (dockerhub) if not explicitly set
 IMG_REGISTRY ?= quay.io
 # container image organization
 IMG_ORG ?= opendatahub
@@ -21,7 +21,11 @@ IMG_VERSION ?= main
 # container image repository
 IMG_REPO ?= model-registry
 # container image
-IMG ?= ${IMG_REGISTRY}/$(IMG_ORG)/$(IMG_REPO)
+ifdef IMG_REGISTRY
+    IMG := ${IMG_REGISTRY}/${IMG_ORG}/${IMG_REPO}
+else
+    IMG := ${IMG_ORG}/${IMG_REPO}
+endif
 
 model-registry: build
 
@@ -190,7 +194,12 @@ proxy: build
 # login to docker
 .PHONY: docker/login
 docker/login:
-	$(DOCKER) login -u "${DOCKER_USER}" -p "${DOCKER_PWD}" "${IMG_REGISTRY}"
+	ifdef IMG_REGISTRY
+		$(DOCKER) login -u "${DOCKER_USER}" -p "${DOCKER_PWD}" "${IMG_REGISTRY}"
+	else
+		$(DOCKER) login -u "${DOCKER_USER}" -p "${DOCKER_PWD}"
+	endif
+
 
 # build docker image
 .PHONY: image/build

clients/python/README.md

@@ -14,12 +14,12 @@ registry = ModelRegistry(server_address="server-address", port=9090, author="aut
 
 model = registry.register_model(
     "my-model",  # model name
-    "s3://path/to/model",  # model URI
+    "https://storage-place.my-company.com",  # model URI
     version="2.0.0",
     description="lorem ipsum",
     model_format_name="onnx",
     model_format_version="1",
-    storage_key="aws-connection-path",
+    storage_key="my-data-connection",
     storage_path="path/to/model",
     metadata={
         # can be one of the following types
@@ -37,10 +37,33 @@ version = registry.get_model_version("my-model", "v2.0")
 experiment = registry.get_model_artifact("my-model", "v2.0")
 ```
 
-### Default values for metadata
+### Importing from S3
 
-If not supplied, `metadata` values defaults to a predefined set of conventional values.
-Reference the technical documentation in the pydoc of the client.
+When registering models stored on S3-compatible object storage, you should use `utils.s3_uri_from` to build an
+unambiguous URI for your artifact.
+
+```py
+from model_registry import ModelRegistry, utils
+
+registry = ModelRegistry(server_address="server-address", port=9090, author="author")
+
+model = registry.register_model(
+    "my-model",  # model name
+    uri=utils.s3_uri_from("path/to/model", "my-bucket"),
+    version="2.0.0",
+    description="lorem ipsum",
+    model_format_name="onnx",
+    model_format_version="1",
+    storage_key="my-data-connection",
+    metadata={
+        # can be one of the following types
+        "int_key": 1,
+        "bool_key": False,
+        "float_key": 3.14,
+        "str_key": "str_value",
+    }
+)
+```
 
 ### Importing from Hugging Face Hub
 

clients/python/src/model_registry/_client.py

@@ -1,7 +1,7 @@
 """Standard client for the model registry."""
+
 from __future__ import annotations
 
-import os
 from typing import get_args
 from warnings import warn
 
@@ -75,16 +75,22 @@ def register_model(
         model_format_name: str,
         model_format_version: str,
         version: str,
-        author: str | None = None,
-        description: str | None = None,
         storage_key: str | None = None,
         storage_path: str | None = None,
         service_account_name: str | None = None,
+        author: str | None = None,
+        description: str | None = None,
         metadata: dict[str, ScalarType] | None = None,
     ) -> RegisteredModel:
         """Register a model.
 
-        Either `storage_key` and `storage_path`, or `service_account_name` must be provided.
+        This registers a model in the model registry. The model is not downloaded, and has to be stored prior to
+        registration.
+
+        Most models can be registered using their URI, along with optional connection-specific parameters, `storage_key`
+        and `storage_path` or, simply a `service_account_name`.
+        URI builder utilities are recommended when referring to specialized storage; for example `utils.s3_uri_from`
+        helper when using S3 object storage data connections.
 
         Args:
             name: Name of the model.
@@ -110,7 +116,7 @@ def register_model(
             version,
             author or self._author,
             description=description,
-            metadata=metadata or self.default_metadata(),
+            metadata=metadata or {},
         )
         self._register_model_artifact(
             mv,
@@ -124,19 +130,6 @@ def register_model(
 
         return rm
 
-    def default_metadata(self) -> dict[str, ScalarType]:
-        """Default metadata valorisations.
-
-        When not explicitly supplied by the end users, these valorisations will be used
-        by default.
-
-        Returns:
-            default metadata valorisations.
-        """
-        return {
-            key: os.environ[key] for key in ["AWS_S3_ENDPOINT", "AWS_S3_BUCKET", "AWS_DEFAULT_REGION"] if key in os.environ
-        }
-
     def register_hf_model(
         self,
         repo: str,
@@ -202,7 +195,6 @@ def register_hf_model(
             model_author = author
         source_uri = hf_hub_url(repo, path, revision=git_ref)
         metadata = {
-            **self.default_metadata(),
             "repo": repo,
             "source_uri": source_uri,
             "model_origin": "huggingface_hub",

clients/python/src/model_registry/_utils.py

@@ -0,0 +1,109 @@
+from __future__ import annotations
+
+import functools
+import inspect
+from collections.abc import Sequence
+from typing import Any, Callable, TypeVar
+
+CallableT = TypeVar("CallableT", bound=Callable[..., Any])
+
+
+# copied from https://github.com/Rapptz/RoboDanny
+def human_join(seq: Sequence[str], *, delim: str = ", ", final: str = "or") -> str:
+    size = len(seq)
+    if size == 0:
+        return ""
+
+    if size == 1:
+        return seq[0]
+
+    if size == 2:
+        return f"{seq[0]} {final} {seq[1]}"
+
+    return delim.join(seq[:-1]) + f" {final} {seq[-1]}"
+
+
+def quote(string: str) -> str:
+    """Add single quotation marks around the given string. Does *not* do any escaping."""
+    return f"'{string}'"
+
+
+# copied from https://github.com/openai/openai-python
+def required_args(*variants: Sequence[str]) -> Callable[[CallableT], CallableT]:  # noqa: C901
+    """Decorator to enforce a given set of arguments or variants of arguments are passed to the decorated function.
+
+    Useful for enforcing runtime validation of overloaded functions.
+
+    Example usage:
+    ```py
+    @overload
+    def foo(*, a: str) -> str:
+        ...
+
+
+    @overload
+    def foo(*, b: bool) -> str:
+        ...
+
+
+    # This enforces the same constraints that a static type checker would
+    # i.e. that either a or b must be passed to the function
+    @required_args(["a"], ["b"])
+    def foo(*, a: str | None = None, b: bool | None = None) -> str:
+        ...
+    ```
+    """
+
+    def inner(func: CallableT) -> CallableT:  # noqa: C901
+        params = inspect.signature(func).parameters
+        positional = [
+            name
+            for name, param in params.items()
+            if param.kind
+            in {
+                param.POSITIONAL_ONLY,
+                param.POSITIONAL_OR_KEYWORD,
+            }
+        ]
+
+        @functools.wraps(func)
+        def wrapper(*args: object, **kwargs: object) -> object:
+            given_params: set[str] = set()
+            for i, _ in enumerate(args):
+                try:
+                    given_params.add(positional[i])
+                except IndexError:
+                    msg = f"{func.__name__}() takes {len(positional)} argument(s) but {len(args)} were given"
+                    raise TypeError(msg) from None
+
+            for key in kwargs:
+                given_params.add(key)
+
+            for variant in variants:
+                matches = all(param in given_params for param in variant)
+                if matches:
+                    break
+            else:  # no break
+                if len(variants) > 1:
+                    variations = human_join(
+                        [
+                            "("
+                            + human_join([quote(arg) for arg in variant], final="and")
+                            + ")"
+                            for variant in variants
+                        ]
+                    )
+                    msg = f"Missing required arguments; Expected either {variations} arguments to be given"
+                else:
+                    # TODO: this error message is not deterministic
+                    missing = list(set(variants[0]) - given_params)
+                    if len(missing) > 1:
+                        msg = f"Missing required arguments: {human_join([quote(arg) for arg in missing])}"
+                    else:
+                        msg = f"Missing required argument: {quote(missing[0])}"
+                raise TypeError(msg)
+            return func(*args, **kwargs)
+
+        return wrapper  # type: ignore
+
+    return inner

clients/python/src/model_registry/exceptions.py

@@ -5,6 +5,10 @@ class StoreException(Exception):
     """Storage related error."""
 
 
+class MissingMetadata(Exception):
+    """Not enough metadata to complete operation."""
+
+
 class UnsupportedTypeException(StoreException):
     """Raised when an unsupported type is encountered."""