-
Notifications
You must be signed in to change notification settings - Fork 11
spec: add initial model config definition #25
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
Conversation
Right now we define a lot of model general information as annotations. They should be defined as a field of model config. We use the guidelines below to decide if a model artifact description should be an annotation or in a structured config file: * If a description belongs to a specific layer, it SHOULD be an annotation * If a description describes a general property of the model, it SHOULD be in a config Also a `ModelFS` structure is introduced to describe the ordering of multiple layers. Fixes: CloudNativeAI#19 Fixes: CloudNativeAI#22 Signed-off-by: Peng Tao <[email protected]>
7955ca7
to
5714784
Compare
Descriptor ModelDescriptor `json:"descriptor"` | ||
|
||
// The model describes a layer content addresses | ||
ModelFS ModelFS `json:"modelfs"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to import the ModelFS
type from github.com/opencontainers/image-spec/specs-go/v1.RootFS
instead of defining it ourselves?
The benefit is that it just demonstrates a clear alignment with the image-spec standard.
However, the downside is that it may cause confusion, as people might wonder why there is a RootFS in an OCI Artifacts Config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the RootFS
name is confusing. Let's define it here ;)
Currently, we define ModelFS in a manner similar to that of RootFS within an image config. Does this mean that the container runtime must be able to parse the "application/vnd.cnai.model.config.v1+json" format to handle layers correctly? like reading the (I'm not sure whether container runtime will parse the image config when pulling a normal OCI image) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Right now we define a lot of model general information as annotations. They should be defined as a field of model config.
We use the guidelines below to decide if a model artifact description should be an annotation or in a structured config file:
Also, a
ModelFS
structure is introduced to describe the ordering of multiple layers.When we have agreed on the contents, I'll add a
config.md
to describe details of each field especially whether it isREQUIRED
orOPTIONAL
in a separate PR.What's more, there is a slight conflict with #24. Will solve it if #24 gets merged first.
Fixes: #19
Fixes: #22