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

feat: add the model processor proposal #257

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

Conversation

chlins
Copy link
Member

@chlins chlins commented Jan 15, 2025

Copy link

@vivek vivek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chlins for authoring this proposal. Overall it looks good. The key aspects of the proposal make it appealing. My takeaways:

  • This proposal brings AI models as first class citizen in Harbor. The user stories to use this CNAI model spec which follows OCI spec to surface AI model
  • Uses existing OCI spec (artifactType and mediaType ) along with following CNAI model format https://github.com/CloudNativeAI/model-spec/blob/main/docs/spec.md
  • No additional APIs needed as it leverages additions API - This is great

I left a comment regarding future model format changes and how new additions can be handled in generic way without requiring code changes. PTAL, I might be off as new to Harbor at code level, as long as we can handle such use case, its all looking good.


## To Be Discussed

Should we think about offering the capability to open files? like `config.json` and `tokenizer.json`, but how should we handle model files? One approach could be to not display them or only show certain parameter information in the header, similar to what HuggingFace does.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, this about displaying model file content in U, API will return the full mode file anyways, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part needs to more detailed investigation and design, the model always be large, so I am not sure whether it should return the full model content.


## API

There is no need to introduce new APIs for this proposal, as the existing "addition" API for the artifact can be utilized to extend capabilities for parsing AI model metadata.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In example below under Endpoints it specifies GET calls for license or README.md or files. If there is a new thing added to the model layer artifactType, would it continue to work or this new type will need to be added in the AI Model Processor. For example:
/additions/someNewthing would continue to work or it would needed to be added in AI Model Processor? If it does, then it can be problematic as it will need code change, upgrade of Harbor etc.

Can we leverage artifactType to be sent in an HTTP accept header. something like:

X-Accept-AI: application/vnd.cnai.model.someNewThing.v1.tar;version=1.3

There is already precedence of such header for vulnerabilities X-Accept-Vulnerabilties.

AI Model Processor can make use of this header along with value in path after additions to generically return the layer requested.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more general mechanism may require a comprehensive restructuring of this part, currently each processor needs to implement the corresponding interface, and for different additions, corresponding code needs to be written for parsing, so a generic way may not be within the scope of this proposal.

1. As a user with replication permission of the artifact, I can replicate the AI model to other OCI registries.
2. As a user with preheat permission of the artifact, I can preheat the AI model to the p2p cluster.
3. As a user with retention permission of the artifact, I can set the retention policy for the AI model.
4. As a user with signature permission of the artifact, I can sign the AI model with the signature.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the actual "signing" process is now out of the scope of Harbor, and it would be better to mention the way the artifact is signed (for example, using co-sign..) if it's part of the user stories.


## To Be Discussed

Should we think about offering the capability to open files? like `config.json` and `tokenizer.json`, but how should we handle model files? One approach could be to not display them or only show certain parameter information in the header, similar to what HuggingFace does.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO in short term we only need to provide the capability of "downloading" the file. Partially showing the file will also require some change in the API, in addition to UI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the "open" capability needs to do more design and API changes, btw, the "downloading" may also need the API change as it require backend return the stream for frontend. So I suggest let's put "open" & "downloading" in the phase2.

## Non-Goals

1. Ensure the current version is stable and there will be no subsequent changes, and no compatibility issues.
Because the current model spec is still in a relatively early stage, there may be iterations and updates in the future.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we ensure backward-compatibilities in future updates? If not we need a way to manage it, for example add a switch and disable the processor by default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current version, I think we may not need to consider compatibility issues for the time being, but starting from some future stable version, backward compatibility should be guaranteed.

proposals/new/AI-model-processor.md Outdated Show resolved Hide resolved
proposals/new/AI-model-processor.md Outdated Show resolved Hide resolved
proposals/new/AI-model-processor.md Outdated Show resolved Hide resolved
}
```

### Endpoints
Copy link
Contributor

@reasonerjt reasonerjt Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions regarding the implementation of these APIs.

In the spec it mentions the annotations are optional. So we should also clarify what to do with a model that does not provide the annotations.

Esp. for the filelist API, the spec doesn't require a 1:1 mapping between a file and a layer(It says SHOULD though), and there can be cases when the annotation org.cnai.model.filepath is not added to any or every layer, shall we also consider that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this value is in the annotation, it might not be suitable as a MUST. Therefore, the handling in Harbor can be considered simpler: for layers with this annotation, return them; for those without, ignore them.

@chlins chlins force-pushed the feat/ai-model-processor branch from c38da39 to fc4e42c Compare February 13, 2025 07:48
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.

6 participants