InferenceModel CRD - using the .metadata.name field instead of having .spec.modelName field

[nirrozenbaum]

I'd like to discuss the Pros/Cons of having the .spec.modelName field in InferenceModel CRD vs using the .metadata.name field.

why should we have .spec.modelName field in InferenceModel instead of using metadata.name?

Model names may have characters disallowed by k8s naming.
More specifically, K8s metadata.name field must follow the DNS subdomain name format.

downsides of using .spec.modelName field:

multiple InferenceModel resources may have the same value of .spec.modeName (in the same pool) and uniqueness is not guaranteed. This opens the door for human errors and undesired issues/bugs.
Current logic in EPP in case there are multiple InferenceModel resources with the same .spec.modelName within the same pool is the following:

• Upon creation of a new InferenceModel resource, EPP checks if an older one with the same spec.modelName exists and keeps only the oldest in datastore.
• Upon deletion of an InferenceModel resource, EPP triggers a ModelResync and checks if other resources with the same .spec.modelName exist in the pool namespace and if so, keeps in datastore the oldest (instead of the one that was deleted).

The above behavior is not natural and not intuitive for any k8s user, since the following may happen:

• When I create a new InferenceModel and I expect a request to go to the targetModel I defined, it may go to a different target (since there was an older model with the same .spec.modelName).
• When I delete an existing InferenceModel, I'm expecting the resource to be deleted, meaning that sending a request to the same ModelName should return 404 (or some similar error) because model should not exist. As explain above, if a different InferenceModel with the same .spec.modelName exist, requests will not fail (it will go the oldest existing model).
• There is no indication for the user when creating an InferenceModel with a name that already exist and that it will be ignored (I cannot use it for serving). This is a very bad UX.
  As a creator of InferenceModel resources, what UX would you prefer -
  create InferenceModel that is later ignored but you don't understand why and you need to inspect?
  OR
  get an appropriate error when creating the resource that the user facing name is taken?

The Alternative - using metadata.name as model name and remove the spec.modelName field.

I must admit that personally it felt really weird to see a field .spec.modelName in a resource that is called InferenceModel which can obviously use the metadata.name.
As mentioned previously, using metadata.name comes with some restrictions on the name. Having said that, this CRD also has the targetModels field which can be used to direct the request to any desired model (using equivalent names to current .spec.modelName). so if one wants to define a model with a model name that is disallowed by k8s restrictions, he can define in metadata.name the user facing model name and in the targetModel field the "real" model.
This is aligned with GIE mental model that modelName is the user facing name and in some cases may be identical to targetModel, but if it doesn't, then targetModel field can be used.

using metadata.name ensures uniqueness of modelName within the same namespace (therefore also within the same pool). This removes the cumbersome existing logic that handles creation of InferenceModel, and more specifically the deletion of InferenceModel, which currently triggers a ModelResync.

While some people may say this is a downside (e.g., "what if I want to use the same user facing name in multiple pools on the same namespace?"), I actually think this is an advantage.

Not only this would improve the current UX (it's undesired to be able to create InferenceModel object that is later ignored), but also having uniqueness enforced on modelName field opens the door for multi-pool implementation with the same UX as a single pool. I've prepared an initial design for this which can be shared in a different issue, but at the very high level - since modelName is unique, when an incoming request arrives its pool can be identified uniquely and the request may be directed to the right pool.
This cannot be done today (having multi-pool UX similar to a single pool) due to the fact that user facing model names are not unique (e.g., multiple pools on same namespace).

• as a clarification to last point, uniqueness of modelName is guaranteed only within the same namespace (when using .metadata.name), so if one uses different pools on different namespaces, it is possible to use the same model name.

To summarize this issue, I believe that using .metadata.name is much more intuitive, much more aligned with k8s principles, has a better UX, and keeps the code that handles InferenceModel resources clean, simple and maintainable.
Is it also aligned with GIE mental model of having a user facing model name and a target model name (which can optionally be identical to user facing model name).
it does introduce some restrictions on the USER FACING name only (NOT on target model name), but this is easily solved by using the targetModel field. I believe that in the majority of the user facing names selection, the names will anyway be in DNS format (I expect user facing names to be simple, descriptive, etc).

would be more than happy to hear your thoughts on this -
cc: @kfswain @smarterclayton @ahg-g @robscott @danehans @shaneutt

[nirrozenbaum]

as a reference, I'm adding the ModelSetIfOlder and ModelResync code (that is triggered right after ModelDelete) from datastore. I'm aiming for a much simpler code (e.g., ModelSet, ModelGet, ModelDelete, no resync needed):

func (ds *datastore) ModelSetIfOlder(infModel *v1alpha2.InferenceModel) bool {
	ds.poolAndModelsMu.Lock()
	defer ds.poolAndModelsMu.Unlock()

	// Check first if the existing model is older.
	// One exception is if the incoming model object is the same, in which case, we should not
	// check for creation timestamp since that means the object was re-created, and so we should override.
	existing, exists := ds.models[infModel.Spec.ModelName]
	if exists {
		diffObj := infModel.Name != existing.Name || infModel.Namespace != existing.Namespace
		if diffObj && existing.ObjectMeta.CreationTimestamp.Before(&infModel.ObjectMeta.CreationTimestamp) {
			return false
		}
	}
	// Set the model.
	ds.models[infModel.Spec.ModelName] = infModel
	return true
}

func (ds *datastore) ModelResync(ctx context.Context, c client.Client, modelName string) (bool, error) {
	ds.poolAndModelsMu.Lock()
	defer ds.poolAndModelsMu.Unlock()

	var models v1alpha2.InferenceModelList
	if err := c.List(ctx, &models, client.MatchingFields{ModelNameIndexKey: modelName}, client.InNamespace(ds.pool.Namespace)); err != nil {
		return false, fmt.Errorf("listing models that match the modelName %s: %w", modelName, err)
	}
	if len(models.Items) == 0 {
		// No other instances of InferenceModels with this ModelName exists.
		return false, nil
	}

	var oldest *v1alpha2.InferenceModel
	for i := range models.Items {
		m := &models.Items[i]
		if m.Spec.ModelName != modelName || // The index should filter those out, but just in case!
			m.Spec.PoolRef.Name != v1alpha2.ObjectName(ds.pool.Name) || // We don't care about other pools, we could setup an index on this too!
			!m.DeletionTimestamp.IsZero() { // ignore objects marked for deletion
			continue
		}
		if oldest == nil || m.ObjectMeta.CreationTimestamp.Before(&oldest.ObjectMeta.CreationTimestamp) {
			oldest = m
		}
	}
	if oldest == nil {
		return false, nil
	}
	ds.models[modelName] = oldest
	return true, nil
}