From 507e93f36013387b4c0886ee9a15e833c37edba0 Mon Sep 17 00:00:00 2001 From: Andrea Lamparelli Date: Sun, 19 Nov 2023 10:13:49 +0100 Subject: [PATCH] Use canAddFields option in mlmd type setup (#160) * Use canAddFields option in mlmd type setup * Manage proxy errors * Update pkg/core/core_test.go Co-authored-by: Matteo Mortari --------- Co-authored-by: Matteo Mortari --- cmd/proxy.go | 15 +-- pkg/core/core.go | 19 ++- pkg/core/core_test.go | 289 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 307 insertions(+), 16 deletions(-) diff --git a/cmd/proxy.go b/cmd/proxy.go index ecdfa64e..aafd87b9 100644 --- a/cmd/proxy.go +++ b/cmd/proxy.go @@ -3,7 +3,6 @@ package cmd import ( "context" "fmt" - "log" "net/http" "time" @@ -29,13 +28,13 @@ hostname and port where it listens.'`, ) func runProxyServer(cmd *cobra.Command, args []string) error { - glog.Infof("Proxy server started at %s:%v", cfg.Hostname, cfg.Port) + glog.Infof("proxy server started at %s:%v", cfg.Hostname, cfg.Port) ctxTimeout, cancel := context.WithTimeout(context.Background(), time.Second*30) defer cancel() mlmdAddr := fmt.Sprintf("%s:%d", proxyCfg.MLMDHostname, proxyCfg.MLMDPort) - glog.Infof("Connecting to MLMD server %s..", mlmdAddr) + glog.Infof("connecting to MLMD server %s..", mlmdAddr) conn, err := grpc.DialContext( ctxTimeout, mlmdAddr, @@ -44,16 +43,14 @@ func runProxyServer(cmd *cobra.Command, args []string) error { grpc.WithTransportCredentials(insecure.NewCredentials()), ) if err != nil { - log.Fatalf("Error dialing connection to mlmd server %s: %v", mlmdAddr, err) - return err + return fmt.Errorf("error dialing connection to mlmd server %s: %v", mlmdAddr, err) } defer conn.Close() - glog.Infof("Connected to MLMD server") + glog.Infof("connected to MLMD server") service, err := core.NewModelRegistryService(conn) if err != nil { - log.Fatalf("Error creating core service: %v", err) - return err + return fmt.Errorf("error creating core service: %v", err) } ModelRegistryServiceAPIService := openapi.NewModelRegistryServiceAPIService(service) @@ -61,7 +58,7 @@ func runProxyServer(cmd *cobra.Command, args []string) error { router := openapi.NewRouter(ModelRegistryServiceAPIController) - log.Fatal(http.ListenAndServe(fmt.Sprintf("%s:%d", cfg.Hostname, cfg.Port), router)) + glog.Fatal(http.ListenAndServe(fmt.Sprintf("%s:%d", cfg.Hostname, cfg.Port), router)) return nil } diff --git a/pkg/core/core.go b/pkg/core/core.go index 7c675abe..bd4350db 100644 --- a/pkg/core/core.go +++ b/pkg/core/core.go @@ -21,6 +21,7 @@ var ( servingEnvironmentTypeName = of(converter.ServingEnvironmentTypeName) inferenceServiceTypeName = of(converter.InferenceServiceTypeName) serveModelTypeName = of(converter.ServeModelTypeName) + canAddFields = of(true) ) // modelRegistryService is the core library of the model registry @@ -37,6 +38,7 @@ func NewModelRegistryService(cc grpc.ClientConnInterface) (api.ModelRegistryApi, // Setup the needed Type instances if not existing already registeredModelReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, ContextType: &proto.ContextType{ Name: registeredModelTypeName, Properties: map[string]proto.PropertyType{ @@ -46,6 +48,7 @@ func NewModelRegistryService(cc grpc.ClientConnInterface) (api.ModelRegistryApi, } modelVersionReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, ContextType: &proto.ContextType{ Name: modelVersionTypeName, Properties: map[string]proto.PropertyType{ @@ -58,6 +61,7 @@ func NewModelRegistryService(cc grpc.ClientConnInterface) (api.ModelRegistryApi, } modelArtifactReq := proto.PutArtifactTypeRequest{ + CanAddFields: canAddFields, ArtifactType: &proto.ArtifactType{ Name: modelArtifactTypeName, Properties: map[string]proto.PropertyType{ @@ -72,6 +76,7 @@ func NewModelRegistryService(cc grpc.ClientConnInterface) (api.ModelRegistryApi, } servingEnvironmentReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, ContextType: &proto.ContextType{ Name: servingEnvironmentTypeName, Properties: map[string]proto.PropertyType{ @@ -81,6 +86,7 @@ func NewModelRegistryService(cc grpc.ClientConnInterface) (api.ModelRegistryApi, } inferenceServiceReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, ContextType: &proto.ContextType{ Name: inferenceServiceTypeName, Properties: map[string]proto.PropertyType{ @@ -95,6 +101,7 @@ func NewModelRegistryService(cc grpc.ClientConnInterface) (api.ModelRegistryApi, } serveModelReq := proto.PutExecutionTypeRequest{ + CanAddFields: canAddFields, ExecutionType: &proto.ExecutionType{ Name: serveModelTypeName, Properties: map[string]proto.PropertyType{ @@ -106,32 +113,32 @@ func NewModelRegistryService(cc grpc.ClientConnInterface) (api.ModelRegistryApi, registeredModelResp, err := client.PutContextType(context.Background(), ®isteredModelReq) if err != nil { - glog.Fatalf("Error setting up context type %s: %v", *registeredModelTypeName, err) + return nil, fmt.Errorf("error setting up context type %s: %v", *registeredModelTypeName, err) } modelVersionResp, err := client.PutContextType(context.Background(), &modelVersionReq) if err != nil { - glog.Fatalf("Error setting up context type %s: %v", *modelVersionTypeName, err) + return nil, fmt.Errorf("error setting up context type %s: %v", *modelVersionTypeName, err) } modelArtifactResp, err := client.PutArtifactType(context.Background(), &modelArtifactReq) if err != nil { - glog.Fatalf("Error setting up artifact type %s: %v", *modelArtifactTypeName, err) + return nil, fmt.Errorf("error setting up artifact type %s: %v", *modelArtifactTypeName, err) } servingEnvironmentResp, err := client.PutContextType(context.Background(), &servingEnvironmentReq) if err != nil { - glog.Fatalf("Error setting up context type %s: %v", *servingEnvironmentTypeName, err) + return nil, fmt.Errorf("error setting up context type %s: %v", *servingEnvironmentTypeName, err) } inferenceServiceResp, err := client.PutContextType(context.Background(), &inferenceServiceReq) if err != nil { - glog.Fatalf("Error setting up context type %s: %v", *inferenceServiceTypeName, err) + return nil, fmt.Errorf("error setting up context type %s: %v", *inferenceServiceTypeName, err) } serveModelResp, err := client.PutExecutionType(context.Background(), &serveModelReq) if err != nil { - glog.Fatalf("Error setting up execution type %s: %v", *serveModelTypeName, err) + return nil, fmt.Errorf("error setting up execution type %s: %v", *serveModelTypeName, err) } return &modelRegistryService{ diff --git a/pkg/core/core_test.go b/pkg/core/core_test.go index 51892818..c5b9eecb 100644 --- a/pkg/core/core_test.go +++ b/pkg/core/core_test.go @@ -217,11 +217,143 @@ func registerInferenceService(assertion *assert.Assertions, service api.ModelReg return *createdEntity.Id } +func TestModelRegistryStartupWithExistingEmptyTypes(t *testing.T) { + ctx := context.Background() + assertion, conn, client, teardown := setup(t) + defer teardown(t) + + // create all types without props + registeredModelReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, + ContextType: &proto.ContextType{ + Name: registeredModelTypeName, + }, + } + modelVersionReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, + ContextType: &proto.ContextType{ + Name: modelVersionTypeName, + }, + } + modelArtifactReq := proto.PutArtifactTypeRequest{ + CanAddFields: canAddFields, + ArtifactType: &proto.ArtifactType{ + Name: modelArtifactTypeName, + }, + } + servingEnvironmentReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, + ContextType: &proto.ContextType{ + Name: servingEnvironmentTypeName, + }, + } + inferenceServiceReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, + ContextType: &proto.ContextType{ + Name: inferenceServiceTypeName, + }, + } + serveModelReq := proto.PutExecutionTypeRequest{ + CanAddFields: canAddFields, + ExecutionType: &proto.ExecutionType{ + Name: serveModelTypeName, + }, + } + + _, err := client.PutContextType(context.Background(), ®isteredModelReq) + assertion.Nil(err) + _, err = client.PutContextType(context.Background(), &modelVersionReq) + assertion.Nil(err) + _, err = client.PutArtifactType(context.Background(), &modelArtifactReq) + assertion.Nil(err) + _, err = client.PutContextType(context.Background(), &servingEnvironmentReq) + assertion.Nil(err) + _, err = client.PutContextType(context.Background(), &inferenceServiceReq) + assertion.Nil(err) + _, err = client.PutExecutionType(context.Background(), &serveModelReq) + assertion.Nil(err) + + // check empty props + regModelResp, _ := client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: registeredModelTypeName, + }) + modelVersionResp, _ := client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: modelVersionTypeName, + }) + modelArtifactResp, _ := client.GetArtifactType(ctx, &proto.GetArtifactTypeRequest{ + TypeName: modelArtifactTypeName, + }) + servingEnvResp, _ := client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: servingEnvironmentTypeName, + }) + inferenceServiceResp, _ := client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: inferenceServiceTypeName, + }) + serveModelResp, _ := client.GetExecutionType(ctx, &proto.GetExecutionTypeRequest{ + TypeName: serveModelTypeName, + }) + + assertion.Equal(0, len(regModelResp.ContextType.Properties)) + assertion.Equal(0, len(modelVersionResp.ContextType.Properties)) + assertion.Equal(0, len(modelArtifactResp.ArtifactType.Properties)) + assertion.Equal(0, len(servingEnvResp.ContextType.Properties)) + assertion.Equal(0, len(inferenceServiceResp.ContextType.Properties)) + assertion.Equal(0, len(serveModelResp.ExecutionType.Properties)) + + // create model registry service + _, err = NewModelRegistryService(conn) + assertion.Nil(err) + + // assure the types have been correctly setup at startup + // check NOT empty props + regModelResp, _ = client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: registeredModelTypeName, + }) + assertion.NotNilf(regModelResp.ContextType, "registered model type %s should exists", *registeredModelTypeName) + assertion.Equal(*registeredModelTypeName, *regModelResp.ContextType.Name) + assertion.Equal(1, len(regModelResp.ContextType.Properties)) + + modelVersionResp, _ = client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: modelVersionTypeName, + }) + assertion.NotNilf(modelVersionResp.ContextType, "model version type %s should exists", *modelVersionTypeName) + assertion.Equal(*modelVersionTypeName, *modelVersionResp.ContextType.Name) + assertion.Equal(4, len(modelVersionResp.ContextType.Properties)) + + modelArtifactResp, _ = client.GetArtifactType(ctx, &proto.GetArtifactTypeRequest{ + TypeName: modelArtifactTypeName, + }) + assertion.NotNilf(modelArtifactResp.ArtifactType, "model artifact type %s should exists", *modelArtifactTypeName) + assertion.Equal(*modelArtifactTypeName, *modelArtifactResp.ArtifactType.Name) + assertion.Equal(6, len(modelArtifactResp.ArtifactType.Properties)) + + servingEnvResp, _ = client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: servingEnvironmentTypeName, + }) + assertion.NotNilf(servingEnvResp.ContextType, "serving environment type %s should exists", *servingEnvironmentTypeName) + assertion.Equal(*servingEnvironmentTypeName, *servingEnvResp.ContextType.Name) + assertion.Equal(1, len(servingEnvResp.ContextType.Properties)) + + inferenceServiceResp, _ = client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: inferenceServiceTypeName, + }) + assertion.NotNilf(inferenceServiceResp.ContextType, "inference service type %s should exists", *inferenceServiceTypeName) + assertion.Equal(*inferenceServiceTypeName, *inferenceServiceResp.ContextType.Name) + assertion.Equal(5, len(inferenceServiceResp.ContextType.Properties)) + + serveModelResp, _ = client.GetExecutionType(ctx, &proto.GetExecutionTypeRequest{ + TypeName: serveModelTypeName, + }) + assertion.NotNilf(serveModelResp.ExecutionType, "serve model type %s should exists", *serveModelTypeName) + assertion.Equal(*serveModelTypeName, *serveModelResp.ExecutionType.Name) + assertion.Equal(2, len(serveModelResp.ExecutionType.Properties)) +} + func TestModelRegistryTypes(t *testing.T) { assertion, conn, client, teardown := setup(t) defer teardown(t) - // create mode registry service + // create model registry service _ = initModelRegistryService(assertion, conn) // assure the types have been correctly setup at startup @@ -243,6 +375,161 @@ func TestModelRegistryTypes(t *testing.T) { }) assertion.NotNilf(modelArtifactResp.ArtifactType, "model version type %s should exists", *modelArtifactTypeName) assertion.Equal(*modelArtifactTypeName, *modelArtifactResp.ArtifactType.Name) + + servingEnvResp, _ := client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: servingEnvironmentTypeName, + }) + assertion.NotNilf(servingEnvResp.ContextType, "serving environment type %s should exists", *servingEnvironmentTypeName) + assertion.Equal(*servingEnvironmentTypeName, *servingEnvResp.ContextType.Name) + + inferenceServiceResp, _ := client.GetContextType(ctx, &proto.GetContextTypeRequest{ + TypeName: inferenceServiceTypeName, + }) + assertion.NotNilf(inferenceServiceResp.ContextType, "inference service type %s should exists", *inferenceServiceTypeName) + assertion.Equal(*inferenceServiceTypeName, *inferenceServiceResp.ContextType.Name) + + serveModelResp, _ := client.GetExecutionType(ctx, &proto.GetExecutionTypeRequest{ + TypeName: serveModelTypeName, + }) + assertion.NotNilf(serveModelResp.ExecutionType, "serve model type %s should exists", *serveModelTypeName) + assertion.Equal(*serveModelTypeName, *serveModelResp.ExecutionType.Name) +} + +func TestModelRegistryFailureForOmittedFieldInRegisteredModel(t *testing.T) { + assertion, conn, client, teardown := setup(t) + defer teardown(t) + + registeredModelReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, + ContextType: &proto.ContextType{ + Name: registeredModelTypeName, + Properties: map[string]proto.PropertyType{ + "deprecated": proto.PropertyType_STRING, + }, + }, + } + + _, err := client.PutContextType(context.Background(), ®isteredModelReq) + assertion.Nil(err) + + // create model registry service + _, err = NewModelRegistryService(conn) + assertion.NotNil(err) + assertion.Regexp("error setting up context type odh.RegisteredModel: rpc error: code = AlreadyExists.*", err.Error()) +} + +func TestModelRegistryFailureForOmittedFieldInModelVersion(t *testing.T) { + assertion, conn, client, teardown := setup(t) + defer teardown(t) + + modelVersionReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, + ContextType: &proto.ContextType{ + Name: modelVersionTypeName, + Properties: map[string]proto.PropertyType{ + "deprecated": proto.PropertyType_STRING, + }, + }, + } + + _, err := client.PutContextType(context.Background(), &modelVersionReq) + assertion.Nil(err) + + // create model registry service + _, err = NewModelRegistryService(conn) + assertion.NotNil(err) + assertion.Regexp("error setting up context type odh.ModelVersion: rpc error: code = AlreadyExists.*", err.Error()) +} + +func TestModelRegistryFailureForOmittedFieldInModelArtifact(t *testing.T) { + assertion, conn, client, teardown := setup(t) + defer teardown(t) + + modelArtifactReq := proto.PutArtifactTypeRequest{ + CanAddFields: canAddFields, + ArtifactType: &proto.ArtifactType{ + Name: modelArtifactTypeName, + Properties: map[string]proto.PropertyType{ + "deprecated": proto.PropertyType_STRING, + }, + }, + } + + _, err := client.PutArtifactType(context.Background(), &modelArtifactReq) + assertion.Nil(err) + + // create model registry service + _, err = NewModelRegistryService(conn) + assertion.NotNil(err) + assertion.Regexp("error setting up artifact type odh.ModelArtifact: rpc error: code = AlreadyExists.*", err.Error()) +} + +func TestModelRegistryFailureForOmittedFieldInServingEnvironment(t *testing.T) { + assertion, conn, client, teardown := setup(t) + defer teardown(t) + + servingEnvironmentReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, + ContextType: &proto.ContextType{ + Name: servingEnvironmentTypeName, + Properties: map[string]proto.PropertyType{ + "deprecated": proto.PropertyType_STRING, + }, + }, + } + _, err := client.PutContextType(context.Background(), &servingEnvironmentReq) + assertion.Nil(err) + + // create model registry service + _, err = NewModelRegistryService(conn) + assertion.NotNil(err) + assertion.Regexp("error setting up context type odh.ServingEnvironment: rpc error: code = AlreadyExists.*", err.Error()) +} + +func TestModelRegistryFailureForOmittedFieldInInferenceService(t *testing.T) { + assertion, conn, client, teardown := setup(t) + defer teardown(t) + + inferenceServiceReq := proto.PutContextTypeRequest{ + CanAddFields: canAddFields, + ContextType: &proto.ContextType{ + Name: inferenceServiceTypeName, + Properties: map[string]proto.PropertyType{ + "deprecated": proto.PropertyType_STRING, + }, + }, + } + + _, err := client.PutContextType(context.Background(), &inferenceServiceReq) + assertion.Nil(err) + + // create model registry service + _, err = NewModelRegistryService(conn) + assertion.NotNil(err) + assertion.Regexp("error setting up context type odh.InferenceService: rpc error: code = AlreadyExists.*", err.Error()) +} + +func TestModelRegistryFailureForOmittedFieldInServeModel(t *testing.T) { + assertion, conn, client, teardown := setup(t) + defer teardown(t) + + serveModelReq := proto.PutExecutionTypeRequest{ + CanAddFields: canAddFields, + ExecutionType: &proto.ExecutionType{ + Name: serveModelTypeName, + Properties: map[string]proto.PropertyType{ + "deprecated": proto.PropertyType_STRING, + }, + }, + } + + _, err := client.PutExecutionType(context.Background(), &serveModelReq) + assertion.Nil(err) + + // create model registry service + _, err = NewModelRegistryService(conn) + assertion.NotNil(err) + assertion.Regexp("error setting up execution type odh.ServeModel: rpc error: code = AlreadyExists.*", err.Error()) } // REGISTERED MODELS