Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

periodic sync upstream KF to midstream ODH #143

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,7 @@ internal/server/openapi/api_model_registry_service.go: bin/openapi-generator-cli
.PHONY: gen/openapi
gen/openapi: bin/openapi-generator-cli openapi/validate pkg/openapi/client.go

pkg/openapi/client.go: bin/openapi-generator-cli api/openapi/model-registry.yaml
rm -rf pkg/openapi
pkg/openapi/client.go: bin/openapi-generator-cli api/openapi/model-registry.yaml clean-pkg-openapi
${OPENAPI_GENERATOR} generate \
-i api/openapi/model-registry.yaml -g go -o pkg/openapi --package-name openapi \
--ignore-file-override ./.openapi-generator-ignore --additional-properties=isGoSubmodule=true,enumClassPrefix=true,useOneOfDiscriminatorLookup=true
Expand All @@ -111,9 +110,12 @@ pkg/openapi/client.go: bin/openapi-generator-cli api/openapi/model-registry.yaml
vet:
${GO} vet ./...

.PHONY: clean
.PHONY: clean-pkg-openapi
while IFS= read -r file; do rm -f "pkg/openapi/$file"; done < pkg/openapi/.openapi-generator/FILES

.PHONY: clean clean-pkg-openapi
clean:
rm -Rf ./model-registry internal/ml_metadata/proto/*.go internal/converter/generated/*.go pkg/openapi internal/server/openapi/api_model_registry_service.go
rm -Rf ./model-registry internal/ml_metadata/proto/*.go internal/converter/generated/*.go internal/server/openapi/api_model_registry_service.go

.PHONY: clean/odh
clean/odh:
Expand Down
60 changes: 23 additions & 37 deletions api/openapi/model-registry.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1093,50 +1093,43 @@ components:
type: string
ModelArtifact:
description: An ML model artifact.
type: object
required:
- artifactType
properties:
artifactType:
type: string
default: "model-artifact"
allOf:
- $ref: "#/components/schemas/BaseArtifact"
- $ref: "#/components/schemas/ModelArtifactCreate"
- type: object
properties:
artifactType:
type: string
default: "model-artifact"
DocArtifact:
description: A document.
type: object
required:
- artifactType
properties:
artifactType:
type: string
default: "doc-artifact"
allOf:
- $ref: "#/components/schemas/BaseArtifact"
- $ref: "#/components/schemas/DocArtifactCreate"
- type: object
properties:
artifactType:
type: string
default: "doc-artifact"
DocArtifactCreate:
description: A document artifact to be created.
type: object
required:
- artifactType
properties:
artifactType:
type: string
default: "doc-artifact"
allOf:
- $ref: "#/components/schemas/BaseArtifactCreate"
- $ref: "#/components/schemas/DocArtifactUpdate"
- type: object
properties:
artifactType:
type: string
default: "doc-artifact"
DocArtifactUpdate:
description: A document artifact to be updated.
required:
- artifactType
properties:
artifactType:
type: string
default: "doc-artifact"
allOf:
- $ref: "#/components/schemas/BaseArtifactUpdate"
- type: object
properties:
artifactType:
type: string
default: "doc-artifact"
RegisteredModel:
description: A registered model in model registry. A registered model has ModelVersion children.
allOf:
Expand Down Expand Up @@ -1247,7 +1240,6 @@ components:
- $ref: "#/components/schemas/BaseExecutionUpdate"
- $ref: "#/components/schemas/BaseResourceCreate"
BaseExecutionUpdate:
type: object
allOf:
- type: object
properties:
Expand Down Expand Up @@ -1438,16 +1430,13 @@ components:
- $ref: "#/components/schemas/BaseResourceList"
ModelArtifactUpdate:
description: An ML model artifact to be updated.
required:
- artifactType
properties:
artifactType:
type: string
default: "model-artifact"
allOf:
- $ref: "#/components/schemas/BaseArtifactUpdate"
- type: object
properties:
artifactType:
type: string
default: "model-artifact"
modelFormatName:
description: Name of the model format.
type: string
Expand All @@ -1465,8 +1454,6 @@ components:
type: string
ModelArtifactCreate:
description: An ML model artifact.
required:
- artifactType
properties:
artifactType:
type: string
Expand Down Expand Up @@ -1562,7 +1549,6 @@ components:
description: A Model Serving environment for serving `RegisteredModels`.
allOf:
- $ref: "#/components/schemas/BaseResource"
- type: object
- $ref: "#/components/schemas/ServingEnvironmentCreate"
ServingEnvironmentUpdate:
description: A Model Serving environment for serving `RegisteredModels`.
Expand Down
2 changes: 1 addition & 1 deletion clients/python/src/mr_openapi/models/doc_artifact.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
class DocArtifact(BaseModel):
"""A document.""" # noqa: E501

artifact_type: StrictStr = Field(alias="artifactType")
custom_properties: dict[str, MetadataValue] | None = Field(
default=None,
description="User provided custom properties which are not defined by its type.",
Expand Down Expand Up @@ -57,6 +56,7 @@ class DocArtifact(BaseModel):
description="Output only. Last update time of the resource since epoch in millisecond since epoch.",
alias="lastUpdateTimeSinceEpoch",
)
artifact_type: StrictStr | None = Field(default="doc-artifact", alias="artifactType")
__properties: ClassVar[list[str]] = [
"customProperties",
"description",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
class DocArtifactCreate(BaseModel):
"""A document artifact to be created.""" # noqa: E501

artifact_type: StrictStr = Field(alias="artifactType")
custom_properties: dict[str, MetadataValue] | None = Field(
default=None,
description="User provided custom properties which are not defined by its type.",
Expand All @@ -46,6 +45,7 @@ class DocArtifactCreate(BaseModel):
default=None,
description="The client provided name of the artifact. This field is optional. If set, it must be unique among all the artifacts of the same artifact type within a database instance and cannot be changed once set.",
)
artifact_type: StrictStr | None = Field(default="doc-artifact", alias="artifactType")
__properties: ClassVar[list[str]] = [
"customProperties",
"description",
Expand Down
12 changes: 10 additions & 2 deletions clients/python/src/mr_openapi/models/doc_artifact_update.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
class DocArtifactUpdate(BaseModel):
"""A document artifact to be updated.""" # noqa: E501

artifact_type: StrictStr = Field(alias="artifactType")
custom_properties: dict[str, MetadataValue] | None = Field(
default=None,
description="User provided custom properties which are not defined by its type.",
Expand All @@ -42,7 +41,15 @@ class DocArtifactUpdate(BaseModel):
description="The uniform resource identifier of the physical artifact. May be empty if there is no physical artifact.",
)
state: ArtifactState | None = None
__properties: ClassVar[list[str]] = ["customProperties", "description", "externalId", "uri", "state"]
artifact_type: StrictStr | None = Field(default="doc-artifact", alias="artifactType")
__properties: ClassVar[list[str]] = [
"customProperties",
"description",
"externalId",
"uri",
"state",
"artifactType",
]

model_config = ConfigDict(
populate_by_name=True,
Expand Down Expand Up @@ -110,5 +117,6 @@ def from_dict(cls, obj: dict[str, Any] | None) -> Self | None:
"externalId": obj.get("externalId"),
"uri": obj.get("uri"),
"state": obj.get("state"),
"artifactType": obj.get("artifactType") if obj.get("artifactType") is not None else "doc-artifact",
}
)
2 changes: 1 addition & 1 deletion clients/python/src/mr_openapi/models/model_artifact.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
class ModelArtifact(BaseModel):
"""An ML model artifact.""" # noqa: E501

artifact_type: StrictStr = Field(alias="artifactType")
custom_properties: dict[str, MetadataValue] | None = Field(
default=None,
description="User provided custom properties which are not defined by its type.",
Expand Down Expand Up @@ -57,6 +56,7 @@ class ModelArtifact(BaseModel):
description="Output only. Last update time of the resource since epoch in millisecond since epoch.",
alias="lastUpdateTimeSinceEpoch",
)
artifact_type: StrictStr | None = Field(default="model-artifact", alias="artifactType")
model_format_name: StrictStr | None = Field(
default=None, description="Name of the model format.", alias="modelFormatName"
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
class ModelArtifactCreate(BaseModel):
"""An ML model artifact.""" # noqa: E501

artifact_type: StrictStr = Field(alias="artifactType")
artifact_type: StrictStr | None = Field(default="model-artifact", alias="artifactType")
custom_properties: dict[str, MetadataValue] | None = Field(
default=None,
description="User provided custom properties which are not defined by its type.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
class ModelArtifactUpdate(BaseModel):
"""An ML model artifact to be updated.""" # noqa: E501

artifact_type: StrictStr = Field(alias="artifactType")
custom_properties: dict[str, MetadataValue] | None = Field(
default=None,
description="User provided custom properties which are not defined by its type.",
Expand All @@ -42,6 +41,7 @@ class ModelArtifactUpdate(BaseModel):
description="The uniform resource identifier of the physical artifact. May be empty if there is no physical artifact.",
)
state: ArtifactState | None = None
artifact_type: StrictStr | None = Field(default="model-artifact", alias="artifactType")
model_format_name: StrictStr | None = Field(
default=None, description="Name of the model format.", alias="modelFormatName"
)
Expand All @@ -61,6 +61,7 @@ class ModelArtifactUpdate(BaseModel):
"externalId",
"uri",
"state",
"artifactType",
"modelFormatName",
"storageKey",
"storagePath",
Expand Down Expand Up @@ -134,6 +135,7 @@ def from_dict(cls, obj: dict[str, Any] | None) -> Self | None:
"externalId": obj.get("externalId"),
"uri": obj.get("uri"),
"state": obj.get("state"),
"artifactType": obj.get("artifactType") if obj.get("artifactType") is not None else "model-artifact",
"modelFormatName": obj.get("modelFormatName"),
"storageKey": obj.get("storageKey"),
"storagePath": obj.get("storagePath"),
Expand Down
2 changes: 1 addition & 1 deletion clients/python/src/mr_openapi/models/registered_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class RegisteredModel(BaseModel):
alias="externalId",
)
name: StrictStr = Field(
description="The client provided name of the artifact. This field is optional. If set, it must be unique among all the artifacts of the same artifact type within a database instance and cannot be changed once set."
description="The client provided name of the model. It must be unique among all the RegisteredModels of the same type within a Model Registry instance and cannot be changed once set."
)
id: StrictStr | None = Field(default=None, description="The unique server generated id of the resource.")
create_time_since_epoch: StrictStr | None = Field(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class RegisteredModelCreate(BaseModel):
alias="externalId",
)
name: StrictStr = Field(
description="The client provided name of the artifact. This field is optional. If set, it must be unique among all the artifacts of the same artifact type within a database instance and cannot be changed once set."
description="The client provided name of the model. It must be unique among all the RegisteredModels of the same type within a Model Registry instance and cannot be changed once set."
)
owner: StrictStr | None = None
state: RegisteredModelState | None = None
Expand Down
34 changes: 34 additions & 0 deletions clients/python/tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from itertools import islice

import pytest
import requests

from model_registry import ModelRegistry, utils
from model_registry.exceptions import StoreError
Expand Down Expand Up @@ -162,6 +163,39 @@ async def test_update_logical_model_with_labels(client: ModelRegistry):
assert ma.custom_properties == ma_labels


@pytest.mark.e2e
async def test_patch_model_artifacts_artifact_type(client: ModelRegistry):
"""Patching ModelArtifact requires `artifactType` value which was previously not required

reported with https://issues.redhat.com/browse/RHOAIENG-15326
"""
name = "test_model"
version = "1.0.0"
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
ma = client.get_model_artifact(name, version)
assert ma
assert ma.id

payload = { "modelFormatName": "foo" }
from .conftest import REGISTRY_HOST, REGISTRY_PORT
response = requests.patch(url=f"{REGISTRY_HOST}:{REGISTRY_PORT}/api/model_registry/v1alpha3/model_artifacts/{ma.id}", json=payload, timeout=10, headers={"Content-Type": "application/json"})
assert response.status_code == 200
ma = client.get_model_artifact(name, version)
assert ma
assert ma.id
assert ma.model_format_name == "foo"


@pytest.mark.e2e
async def test_update_preserves_model_info(client: ModelRegistry):
name = "test_model"
Expand Down
20 changes: 17 additions & 3 deletions clients/ui/bff/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ IMG ?= model-registry-bff:latest
PORT ?= 4000
MOCK_K8S_CLIENT ?= false
MOCK_MR_CLIENT ?= false
# ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
ENVTEST_K8S_VERSION = 1.29.0

.PHONY: all
all: build
Expand Down Expand Up @@ -32,15 +34,17 @@ vet: .
go vet ./...

.PHONY: test
test:
test: fmt vet envtest
ENVTEST_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" \
go test ./...

.PHONY: build
build: fmt vet test
go build -o bin/bff cmd/main.go

.PHONY: run
run: fmt vet
run: fmt vet envtest
ENVTEST_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" \
go run ./cmd/main.go --port=$(PORT) --mock-k8s-client=$(MOCK_K8S_CLIENT) --mock-mr-client=$(MOCK_MR_CLIENT)

.PHONY: docker-build
Expand All @@ -54,8 +58,18 @@ LOCALBIN ?= $(shell pwd)/bin
$(LOCALBIN):
mkdir -p $(LOCALBIN)

## Tool Binaries
ENVTEST ?= $(LOCALBIN)/setup-envtest-$(ENVTEST_VERSION)
GOLANGCI_LINT ?= $(LOCALBIN)/golangci-lint-$(GOLANGCI_LINT_VERSION)

## Tool Versions
GOLANGCI_LINT_VERSION ?= v1.57.2
ENVTEST_VERSION ?= release-0.17

.PHONY: envtest
envtest: $(ENVTEST) ## Download setup-envtest locally if necessary.
$(ENVTEST): $(LOCALBIN)
$(call go-install-tool,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest,$(ENVTEST_VERSION))

.PHONY: golangci-lint
golangci-lint: $(GOLANGCI_LINT) ## Download golangci-lint locally if necessary.
Expand All @@ -75,4 +89,4 @@ echo "Downloading $${package}" ;\
GOBIN=$(LOCALBIN) go install $${package} ;\
mv "$$(echo "$(1)" | sed "s/-$(3)$$//")" $(1) ;\
}
endef
endef
Loading