[#5600] feat(API): Add Java API definition for ML model in Gravitino #5612
Conversation
| * @throws NoSuchModelException If the model does not exist. | ||
| * @throws ModelVersionAliasesAlreadyExistsException If the aliases already exist in the model. | ||
| */ | ||
| ModelVersion link(String[] aliases, String comment, String url, Map<String, String> properties) |
There was a problem hiding this comment.
Do we have the interface unlink to change the version alias?
There was a problem hiding this comment.
For the change/alter/update support, I will do this in the next version. In this version, we only support register/link, get, list and delete operations, so I didn't add that interface.
| * | ||
| * @return the properties of the model version. | ||
| */ | ||
| Map<String, String> properties(); |
There was a problem hiding this comment.
Do we need to add the author and date for ModelVersion if this is a version management. I just want to refer to some design about git.
There was a problem hiding this comment.
ModelVersion has audit info, so it will have the creator and date information for each version. We don't need to have a new property to specify this.
|
@xloya @coolderli @zhoukangcn can you please also review this API design, to see if it is OK for you. |
|
LGTM. |
|
Sorry late for reply, I will take time to review it today. |
| * | ||
| * @return The versions of the model object. | ||
| */ | ||
| ModelVersion[] versions(); |
There was a problem hiding this comment.
Do we need return all versions here? If there are too many versions, I think this will lead to a significant decrease in query efficiency.
There was a problem hiding this comment.
I was thinking of returning a version list, and then using the version number to get the specific model version, but I changed to this because I assumed that there may not be too many versions in one model. Do you know about the real scenario, will there be hundreds of versions in one model?
There was a problem hiding this comment.
There are many types of models within Xiaomi. Checkpoints generated during training in common frameworks such as Tensorflow/Pytorch will also be registered as models. In the long run, there may be a large amount of version information.
There was a problem hiding this comment.
I see. So how about returning a version list, then user can use the returned version list to get the specified model version detail?
There was a problem hiding this comment.
@xloya is it neccesary to record every checkpoint when training a model or just record the best model version?
There was a problem hiding this comment.
@jerryshao I think we could only return the latest version when getting the model metadata, and provide a api to getting all versions by the model, WDYT?
There was a problem hiding this comment.
@FANNG1 I don’t know much about this, but based on our current internal business, the model is updated almost every day.
| * @return The model version object. | ||
| * @throws NoSuchModelVersionException if the version does not exist. | ||
| */ | ||
| ModelVersion version(int version) throws NoSuchModelVersionException; |
There was a problem hiding this comment.
Is it more scalable to make Model Version related operations independent of interfaces?
There was a problem hiding this comment.
I cannot get your meaning, can you explain more about the meaning of "scalable"?
There was a problem hiding this comment.
I personally think that the frequency of model version metadata operations may be more frequent than some metadata of the model itself, so perhaps the model version operations can be made into a separate interface to avoid frequent modifications to the model interface implementation.
There was a problem hiding this comment.
How about also moving these interfaces to ModelCatalog?
In my design doc, I put these interfaces in ModelCatalog, so users can directly operate the ModelVersion, no need to get the Model first, and then get the ModelVersion. But I changed to this to align with table/partition design and concept.
There was a problem hiding this comment.
Yeah, I think these methods are probably more suitable for the definition of metadata operations rather than metadata definitions.
|
LGTM |
What changes were proposed in this pull request?
This PR adds the API definition for ML model and model catalog.
Why are the changes needed?
This is the first step to support ML model management in Gravitino.
Fix: #5600
Does this PR introduce any user-facing change?
Yes.
How was this patch tested?
Manual verification.