Skip to content
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

spec: add initial model config definition #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bergwolf
Copy link
Contributor

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.

When we have agreed on the contents, I'll add a config.md to describe details of each field especially whether it is REQUIRED or OPTIONAL 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

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]>
Descriptor ModelDescriptor `json:"descriptor"`

// The model describes a layer content addresses
ModelFS ModelFS `json:"modelfs"`
Copy link
Contributor

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.

Copy link
Contributor Author

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 ;)

@caozhuozi
Copy link
Contributor

caozhuozi commented Jan 29, 2025

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 diff_ids array?

(I'm not sure whether container runtime will parse the image config when pulling a normal OCI image)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defining the diffID Generation Order in the Model Spec [RFC] Model annotation and config guidelines
2 participants