From 97ec8cfd4ef13190f3939fbb24b6f146d570ed12 Mon Sep 17 00:00:00 2001 From: Jesse Gross Date: Wed, 7 Aug 2024 11:44:25 -0700 Subject: [PATCH 1/2] image: Clarify argument to WriteManifest is config When creating a model the config layer is appended to the list of layers and then the last layer is used as the config when writing the manifest. This change directly uses the config layer to write the manifest. There is no behavior change but it is less error prone. --- server/images.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/images.go b/server/images.go index 7ed35995..4202a413 100644 --- a/server/images.go +++ b/server/images.go @@ -625,12 +625,12 @@ func CreateModel(ctx context.Context, name model.Name, modelFileDir, quantizatio return err } - layer, err := NewLayer(&b, "application/vnd.docker.container.image.v1+json") + configLayer, err := NewLayer(&b, "application/vnd.docker.container.image.v1+json") if err != nil { return err } - for _, layer := range append(layers, layer) { + for _, layer := range append(layers, configLayer) { if layer.status != "" { fn(api.ProgressResponse{Status: layer.status}) } @@ -639,7 +639,7 @@ func CreateModel(ctx context.Context, name model.Name, modelFileDir, quantizatio old, _ := ParseNamedManifest(name) fn(api.ProgressResponse{Status: "writing manifest"}) - if err := WriteManifest(name, layer, layers); err != nil { + if err := WriteManifest(name, configLayer, layers); err != nil { return err } From 7edaf6e7e8d79a9c88419988ae98afaf3fc32f15 Mon Sep 17 00:00:00 2001 From: Jesse Gross Date: Wed, 7 Aug 2024 14:22:17 -0700 Subject: [PATCH 2/2] manifest: Store layers inside manifests consistently as values. Commit 1829fb61 ("manifest: Fix crash on startup when trying to clean up unused files (#5840)") changed the config layer stored in manifests from a pointer to a value. This was done in order to avoid potential nil pointer dereferences after it is deserialized from JSON in the event that the field is missing. This changes the Layers slice to also be stored by value. This enables consistency in handling across the two objects. --- server/images.go | 14 +++++++------- server/layer.go | 28 ++++++++++++++-------------- server/manifest.go | 16 ++++++++-------- server/model.go | 2 +- server/routes_delete_test.go | 2 +- server/upload.go | 4 ++-- 6 files changed, 33 insertions(+), 33 deletions(-) diff --git a/server/images.go b/server/images.go index 4202a413..0e753f56 100644 --- a/server/images.go +++ b/server/images.go @@ -373,7 +373,7 @@ func CreateModel(ctx context.Context, name model.Name, modelFileDir, quantizatio var messages []*api.Message parameters := make(map[string]any) - var layers []*Layer + var layers []Layer for _, c := range modelfile.Commands { mediatype := fmt.Sprintf("application/vnd.ollama.image.%s", c.Name) @@ -499,7 +499,7 @@ func CreateModel(ctx context.Context, name model.Name, modelFileDir, quantizatio if c.Name != "license" { // replace - layers = slices.DeleteFunc(layers, func(layer *Layer) bool { + layers = slices.DeleteFunc(layers, func(layer Layer) bool { if layer.MediaType != mediatype { return false } @@ -545,7 +545,7 @@ func CreateModel(ctx context.Context, name model.Name, modelFileDir, quantizatio } var err2 error - layers = slices.DeleteFunc(layers, func(layer *Layer) bool { + layers = slices.DeleteFunc(layers, func(layer Layer) bool { switch layer.MediaType { case "application/vnd.ollama.image.message": // if there are new messages, remove the inherited ones @@ -839,10 +839,10 @@ func PushModel(ctx context.Context, name string, regOpts *registryOptions, fn fu return err } - var layers []*Layer + var layers []Layer layers = append(layers, manifest.Layers...) if manifest.Config.Digest != "" { - layers = append(layers, &manifest.Config) + layers = append(layers, manifest.Config) } for _, layer := range layers { @@ -911,10 +911,10 @@ func PullModel(ctx context.Context, name string, regOpts *registryOptions, fn fu return fmt.Errorf("pull model manifest: %s", err) } - var layers []*Layer + var layers []Layer layers = append(layers, manifest.Layers...) if manifest.Config.Digest != "" { - layers = append(layers, &manifest.Config) + layers = append(layers, manifest.Config) } skipVerify := make(map[string]bool) diff --git a/server/layer.go b/server/layer.go index a2b66782..c666bd10 100644 --- a/server/layer.go +++ b/server/layer.go @@ -16,15 +16,15 @@ type Layer struct { status string } -func NewLayer(r io.Reader, mediatype string) (*Layer, error) { +func NewLayer(r io.Reader, mediatype string) (Layer, error) { blobs, err := GetBlobsPath("") if err != nil { - return nil, err + return Layer{}, err } temp, err := os.CreateTemp(blobs, "sha256-") if err != nil { - return nil, err + return Layer{}, err } defer temp.Close() defer os.Remove(temp.Name()) @@ -32,28 +32,28 @@ func NewLayer(r io.Reader, mediatype string) (*Layer, error) { sha256sum := sha256.New() n, err := io.Copy(io.MultiWriter(temp, sha256sum), r) if err != nil { - return nil, err + return Layer{}, err } if err := temp.Close(); err != nil { - return nil, err + return Layer{}, err } digest := fmt.Sprintf("sha256:%x", sha256sum.Sum(nil)) blob, err := GetBlobsPath(digest) if err != nil { - return nil, err + return Layer{}, err } status := "using existing layer" if _, err := os.Stat(blob); err != nil { status = "creating new layer" if err := os.Rename(temp.Name(), blob); err != nil { - return nil, err + return Layer{}, err } } - return &Layer{ + return Layer{ MediaType: mediatype, Digest: digest, Size: n, @@ -61,22 +61,22 @@ func NewLayer(r io.Reader, mediatype string) (*Layer, error) { }, nil } -func NewLayerFromLayer(digest, mediatype, from string) (*Layer, error) { +func NewLayerFromLayer(digest, mediatype, from string) (Layer, error) { if digest == "" { - return nil, errors.New("creating new layer from layer with empty digest") + return Layer{}, errors.New("creating new layer from layer with empty digest") } blob, err := GetBlobsPath(digest) if err != nil { - return nil, err + return Layer{}, err } fi, err := os.Stat(blob) if err != nil { - return nil, err + return Layer{}, err } - return &Layer{ + return Layer{ MediaType: mediatype, Digest: digest, Size: fi.Size(), @@ -109,7 +109,7 @@ func (l *Layer) Remove() error { } for _, m := range ms { - for _, layer := range append(m.Layers, &m.Config) { + for _, layer := range append(m.Layers, m.Config) { if layer.Digest == l.Digest { // something is using this layer return nil diff --git a/server/manifest.go b/server/manifest.go index b966ddbe..6a5d7b88 100644 --- a/server/manifest.go +++ b/server/manifest.go @@ -14,10 +14,10 @@ import ( ) type Manifest struct { - SchemaVersion int `json:"schemaVersion"` - MediaType string `json:"mediaType"` - Config Layer `json:"config"` - Layers []*Layer `json:"layers"` + SchemaVersion int `json:"schemaVersion"` + MediaType string `json:"mediaType"` + Config Layer `json:"config"` + Layers []Layer `json:"layers"` filepath string fi os.FileInfo @@ -25,7 +25,7 @@ type Manifest struct { } func (m *Manifest) Size() (size int64) { - for _, layer := range append(m.Layers, &m.Config) { + for _, layer := range append(m.Layers, m.Config) { size += layer.Size } @@ -46,7 +46,7 @@ func (m *Manifest) Remove() error { } func (m *Manifest) RemoveLayers() error { - for _, layer := range append(m.Layers, &m.Config) { + for _, layer := range append(m.Layers, m.Config) { if layer.Digest != "" { if err := layer.Remove(); errors.Is(err, os.ErrNotExist) { slog.Debug("layer does not exist", "digest", layer.Digest) @@ -95,7 +95,7 @@ func ParseNamedManifest(n model.Name) (*Manifest, error) { return &m, nil } -func WriteManifest(name model.Name, config *Layer, layers []*Layer) error { +func WriteManifest(name model.Name, config Layer, layers []Layer) error { manifests, err := GetManifestPath() if err != nil { return err @@ -115,7 +115,7 @@ func WriteManifest(name model.Name, config *Layer, layers []*Layer) error { m := Manifest{ SchemaVersion: 2, MediaType: "application/vnd.docker.distribution.manifest.v2+json", - Config: *config, + Config: config, Layers: layers, } diff --git a/server/model.go b/server/model.go index f2946a0b..ad6e4e55 100644 --- a/server/model.go +++ b/server/model.go @@ -26,7 +26,7 @@ import ( var intermediateBlobs map[string]string = make(map[string]string) type layerGGML struct { - *Layer + Layer *llm.GGML } diff --git a/server/routes_delete_test.go b/server/routes_delete_test.go index 1c950d66..82fac9f5 100644 --- a/server/routes_delete_test.go +++ b/server/routes_delete_test.go @@ -98,7 +98,7 @@ func TestDeleteDuplicateLayers(t *testing.T) { } // create a manifest with duplicate layers - if err := WriteManifest(n, config, []*Layer{config}); err != nil { + if err := WriteManifest(n, config, []Layer{config}); err != nil { t.Fatal(err) } diff --git a/server/upload.go b/server/upload.go index b5a244ea..2f115436 100644 --- a/server/upload.go +++ b/server/upload.go @@ -26,7 +26,7 @@ import ( var blobUploadManager sync.Map type blobUpload struct { - *Layer + Layer Total int64 Completed atomic.Int64 @@ -362,7 +362,7 @@ func (p *progressWriter) Rollback() { p.written = 0 } -func uploadBlob(ctx context.Context, mp ModelPath, layer *Layer, opts *registryOptions, fn func(api.ProgressResponse)) error { +func uploadBlob(ctx context.Context, mp ModelPath, layer Layer, opts *registryOptions, fn func(api.ProgressResponse)) error { requestURL := mp.BaseURL() requestURL = requestURL.JoinPath("v2", mp.GetNamespaceRepository(), "blobs", layer.Digest)