-
Notifications
You must be signed in to change notification settings - Fork 379
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
Support pulling Ollama [non-]OCI image #2539
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Xiaodong Ye <xiaodong.ye@mthreads.com>
Signed-off-by: Xiaodong Ye <xiaodong.ye@mthreads.com>
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.
Thanks!
We need to have a proper design discussion about storing non-image artifacts, and what that means.
Do not merge until that happens.
Cc: @baude
The “image volume” feature explicitly talks about image volumes. I don’t see that arbitrarily extending it to also accept other content is obviously desirable.
And even if it were desirable at the Kubernetes level, that doesn’t at all imply that the data should be stored in “images” with “layers” and OverlayFS and that it should be possible to podman run
the thing.
We really need to have a design discussion about the storage first; but even if we did accept the proposed mechanism, the c/image implementation would have to be much more extensive. This is nowhere near enough.
@@ -996,7 +997,18 @@ func (c *dockerClient) fetchManifest(ctx context.Context, ref dockerReference, t | |||
if err != nil { | |||
return nil, "", err | |||
} | |||
return manblob, simplifyContentType(res.Header.Get("Content-Type")), nil | |||
// Extra check for the content type in the manifest | |||
if contentType == "text/plain" { |
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.
- It is fundamentally unsound to override an externally-supplied MIME type of some data by parsing the data in another way and trusting its contents. The whole point of having the MIME type separate is that it directs how the data is to be interpreted. I’m unhappy about such special cases, and I’m especially unhappy about adding them in entirely new use cases where we could just choose not to do that. Why is this necessary here?
- The
docker
transport has no business interpreting image contents this way; that’s the responsibility of the generic code. - … and the generic code already has a heuristic fallback for
text/plain
; this would override/break that.
@@ -43,6 +64,8 @@ func SupportedSchema2MediaType(m string) error { | |||
switch m { | |||
case DockerV2ListMediaType, DockerV2Schema1MediaType, DockerV2Schema1SignedMediaType, DockerV2Schema2ConfigMediaType, DockerV2Schema2ForeignLayerMediaType, DockerV2Schema2ForeignLayerMediaTypeGzip, DockerV2Schema2LayerMediaType, DockerV2Schema2MediaType, DockerV2SchemaLayerMediaTypeUncompressed: | |||
return nil | |||
case OllamaImageModelLayerMediaType, OllamaImageAdapterLayerMediaType, OllamaImageProjectorLayerMediaType, OllamaImagePromptLayerMediaType, OllamaImageTemplateLayerMediaType, OllamaImageSystemLayerMediaType, OllamaImageParamsLayerMediaType, OllamaImageMessagesLayerMediaType, OllamaImageLicenseLayerMediaType: | |||
return nil |
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.
I don’t know why we would add all of these to v2s2; that’s a ~frozen format.
And if we did add them, we would need to adjust all of the rest of the v2s2 code to correctly interpret such data, at the very least to not parse it as an ordinary image.
Either way, it seems to me that this should either be completely opaque (a proper OCI artifact, maybe), or intentionally a completely new manifest / image format implementation.
@@ -218,9 +220,18 @@ func (s *storageImageDestination) PutBlobWithOptions(ctx context.Context, stream | |||
return info, nil | |||
} | |||
|
|||
layerFilename := func() *string { |
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.
I don’t see why a nested function is necessary here.
@@ -218,9 +220,18 @@ func (s *storageImageDestination) PutBlobWithOptions(ctx context.Context, stream | |||
return info, nil | |||
} | |||
|
|||
layerFilename := func() *string { | |||
if strings.HasPrefix(blobinfo.MediaType, manifest.OllamaImageLayerMediaTypePrefix) { | |||
filename := strings.TrimPrefix(blobinfo.MediaType, manifest.OllamaImageLayerMediaTypePrefix) |
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.
Choosing a “filename”, whatever the value is, this way makes no sense to me. What happens if there are two layers with the same MIME type in the image?
@@ -218,9 +220,18 @@ func (s *storageImageDestination) PutBlobWithOptions(ctx context.Context, stream | |||
return info, nil | |||
} |
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.
There is an early return above; in that case .filename
would not be set.
OK, that’s somewhat theoretical. But also, nothing sets .filename
on the TryReusingBlobWithOptions
code path. That one is 100% a blocker.
@@ -218,9 +220,18 @@ func (s *storageImageDestination) PutBlobWithOptions(ctx context.Context, stream | |||
return info, nil | |||
} | |||
|
|||
layerFilename := func() *string { | |||
if strings.HasPrefix(blobinfo.MediaType, manifest.OllamaImageLayerMediaTypePrefix) { | |||
filename := strings.TrimPrefix(blobinfo.MediaType, manifest.OllamaImageLayerMediaTypePrefix) |
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.
What would the storageImageSource
counterpart look like?!
Compare also containers/skopeo#2395 (and, I think, ollama/ollama#6510 ): “Ollama” itself is not even remotely an OCI-compliant registry right now. What is the ecosystem and community situation for this data format? Is this a multi-faceted interoperability effort, where all of these things are going to happily fall into place, or is this setting us up on an adversarial interoperability path? Cc: @baude , another thing to have a very clear position on. |
Agree that we should only support compliant registries. |
Yes. I faced two issues pulling from
I thought decoding the manifest JSON might make MIME type determination more robust, but it clearly looks incorrect. |
Separate from the storage discussion happening in containers/storage#2075, and assuming the Ollama format and transport will not change (will it? I have no idea)
|
The issue I’m trying to solve is how to make OCI artifacts mountable as volumes in containers (the Ollama image is a practical example, but it uses some custom MIME types—can it still be defined as an OCI artifact?). This blog post Kubernetes 1.31: Image Volume Source discusses some practical use cases that I find quite relevant. In particular, using OCI artifacts to distribute AI models (weights, parameters, system prompts, etc.) in enterprise AI inference services is crucial, especially for version management and signing. The volume image used in the blog is ➜ ./bin/skopeo inspect docker://quay.io/crio/artifact:v1
{
"Name": "quay.io/crio/artifact",
"Digest": "sha256:52c997d8906f39c5b5550dd1f05fd785fb46bbb50eb055f9df61df6dc9e3a6cd",
"RepoTags": [
"v1"
],
"Created": null,
"DockerVersion": "",
"Labels": null,
"Architecture": "amd64",
"Os": "linux",
"Layers": [
"sha256:4e80bd845ae8736c96d644d96142807d958f44505e973f6eb4145689e05e86d2"
],
"LayersData": [
{
"MIMEType": "application/vnd.oci.image.layer.v1.tar+gzip",
"Digest": "sha256:4e80bd845ae8736c96d644d96142807d958f44505e973f6eb4145689e05e86d2",
"Size": 170,
"Annotations": null
}
],
"Env": null
} However, for Ollama images or other images with custom MIME types, should we consider adding support for these, or perhaps introducing a plugin mechanism to allow vendors to implement their own solutions? ➜ ./bin/skopeo inspect docker://sh-harbor.mthreads.com/cloud-mirror/tinyllama:latest
{
"Name": "sh-harbor.mthreads.com/cloud-mirror/tinyllama",
"Digest": "sha256:2644915ede352ea7bdfaff0bfac0be74c719d5d5202acb63a6fb095b52f394a4",
"RepoTags": [
"latest"
],
"Created": "0001-01-01T00:00:00Z",
"DockerVersion": "",
"Labels": null,
"Architecture": "amd64",
"Os": "linux",
"Layers": [
"sha256:2af3b81862c6be03c769683af18efdadb2c33f60ff32ab6f83e42c043d6c7816",
"sha256:af0ddbdaaa26f30d54d727f9dd944b76bdb926fdaf9a58f63f78c532f57c191f",
"sha256:c8472cd9daed5e7c20aa53689e441e10620a002aacd58686aeac2cb188addb5c",
"sha256:fa956ab37b8c21152f975a7fcdd095c4fee8754674b21d9b44d710435697a00d"
],
"LayersData": [
{
"MIMEType": "application/vnd.ollama.image.model",
"Digest": "sha256:2af3b81862c6be03c769683af18efdadb2c33f60ff32ab6f83e42c043d6c7816",
"Size": 637699456,
"Annotations": null
},
{
"MIMEType": "application/vnd.ollama.image.template",
"Digest": "sha256:af0ddbdaaa26f30d54d727f9dd944b76bdb926fdaf9a58f63f78c532f57c191f",
"Size": 70,
"Annotations": null
},
{
"MIMEType": "application/vnd.ollama.image.system",
"Digest": "sha256:c8472cd9daed5e7c20aa53689e441e10620a002aacd58686aeac2cb188addb5c",
"Size": 31,
"Annotations": null
},
{
"MIMEType": "application/vnd.ollama.image.params",
"Digest": "sha256:fa956ab37b8c21152f975a7fcdd095c4fee8754674b21d9b44d710435697a00d",
"Size": 98,
"Annotations": null
}
],
"Env": null
} |
I think it’s necessary to be precise here. The Ollama format is not even in OCI format, and it is not distributed on an OCI registry; so it can’t be an OCI artifact. We can talk about supporting the Ollama format, and about supporting OCI artifacts, but they are not the same thing at all.
That… is also not an OCI artifact; it’s an OCI image. And if you read https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/4639-oci-volume-source :
It’s been an explicit non-goal to have a defined way to use OCI artifacts. Now, that doesn’t mean that we shouldn’t talk about it and design it. It’s clearly desirable to use something else than images for non-image data. But let’s be clear that we are designing it; not implementing a consensus spec. |
Background:
Kubernetes 1.31 introduced a new feature: Read-Only Volumes Based on OCI Artifacts. I believe this feature could be very useful for deploying a dedicated model alongside Ollama in Kubernetes.
Ollama has introduced several new media types (e.g. application/vnd.ollama.image.model) for storing GGUF models, system prompts, and more. Each layer is essentially a file and does not need to be untarred.
This PR adds a new field,
layerFilename
, toaddedLayerInfo
, and the overlay driver will handle the layer creation separately.A separate PR for
containers/storage
will be submitted later.Please see the following logs for instructions on how to mount the Ollama image as a volume: