Context names can't be empty

[isinyaaa]

Describe the bug When creating either model versions or registered models, failing to provide a name will result in a server error. For registered models, the server will handle a 500 with the rpc error, but it will crash on model version create requests without names.

To Reproduce Steps to reproduce the behavior:

1. Do this '...'
2. Do that '....'
3. See error

Expected behavior A clear and concise description of what you expected to happen.

Additional context Add any other context about the problem here.

[tarilabs]

Thank you for reporting this, without more context information not really sure how/where the alleged "crash" happens.

I just wanted to point out maybe this is a gap from the REST layer to the Core API layer.

For ModelVersion specifically in the converter, called from:

https://github.com/kubeflow/model-registry/blob/bda3731f5f8c12ef9ee291eacd13bdfd980add07/pkg/core/core.go#L346

then:

https://github.com/kubeflow/model-registry/blob/bda3731f5f8c12ef9ee291eacd13bdfd980add07/internal/converter/openapi_mlmd_converter.go#L32-L38

(notice the explicit mapping fn for name)

then:

https://github.com/kubeflow/model-registry/blob/bda3731f5f8c12ef9ee291eacd13bdfd980add07/internal/converter/openapi_mlmd_converter_util.go#L208-L212

if "contrasted" with the analogous function for artifact:

https://github.com/kubeflow/model-registry/blob/bda3731f5f8c12ef9ee291eacd13bdfd980add07/internal/converter/openapi_mlmd_converter_util.go#L321-L333

we can notice the ModelVersion was indeed considered named.

I believe to recall that for RegisteredModel we decided not to allow "un-named" RegisteredModel, so to avoid "pollution" of a lot of UUIDs, since when you are using MR to create a RegisteredModel you likely want to give it a proper name.

So in conclusion:

• I believe for ModelVersion we deliberately decided it was due a name.
• For RegisteredModel I seem to recall we wanted to avoid un-named, but maybe I'm recalling it wrong and @lampajr might remember more here.

[lampajr]

I believe for ModelVersion we deliberately decided it was due a name.

I think this was mainly because the name of the ModelVersion that the user provides is actually the version, e.g., v1 or whatever identifies that version in the RegisteredModel, therefore it must be provided.

For RegisteredModel I seem to recall we wanted to avoid un-named

If I recall this was because, by default Context's names cannot be modified in MLMD (I might be wrong but that's what I recall) and therefore we decided to put the RegisteredModel name as mandatory as it cannot be changed in the future.

[tarilabs]

Thank you for chiming in, @lampajr

[isinyaaa]

Thank you for reporting this, without more context information not really sure how/where the alleged "crash" happens. ... https://github.com/kubeflow/model-registry/blob/bda3731f5f8c12ef9ee291eacd13bdfd980add07/internal/converter/openapi_mlmd_converter_util.go#L208-L212

The crash happens when calling PrefixWhenOwned with a nil string, as you can't indirect it.

To reproduce:

1. Create a registered model
2. Create a model version with a missing name (but providing the registered model id) e.g. using httpie I just need to

   http POST localhost:8080/api/model_registry/v1alpha3/registered_models 'Content-Type:application/json' 'accept:application/json' name="test"
   http POST localhost:8080/api/model_registry/v1alpha3/model_versions 'Content-Type:application/json' 'accept:application/json' registeredModelId="1"