Handle cases where artifact is recognized but does not contain Kitfile

Since we're adding support for model-spec artifacts (modelpacks), there
may be cases where we are handling artifacts that do not have a Kitfile
available. As a result, we need to handle the case where a config
(Kitfile) cannot be found:

* Add error type for repo utilities to signify that everything is fine,
  except no Kitfile is available
* For inspect, warn that there is no Kitfile and show the manifest
* For list, ignore metadata that we normally retrieve from the Kitfile
  (e.g. author and model name)
* For unpack, attempt to generate a dummy config to allow unpacking to
  continue:
  * Don't unpack Kitfile, as it does not exist
  * Use model-spec file path annotations to fill a fake Kitfile with
    paths to allow unpacking

Signed-off-by: Angel Misevski <[email protected]>

Author: amisevsk

pkg/cmd/inspect/inspect.go

@@ -18,6 +18,7 @@ package inspect
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/kitops-ml/kitops/pkg/artifact"
@@ -66,7 +67,7 @@ func getRemoteInspect(ctx context.Context, opts *inspectOptions) (*inspectInfo,
 
 func getInspectInfo(ctx context.Context, repository oras.Target, ref string) (*inspectInfo, error) {
 	desc, manifest, kitfile, err := util.ResolveManifestAndConfig(ctx, repository, ref)
-	if err != nil {
+	if err != nil && !errors.Is(err, util.ErrNoKitfile) {
 		return nil, err
 	}
 	version := "unknown"

pkg/cmd/list/list.go

@@ -49,9 +49,17 @@ func readInfoFromRepo(ctx context.Context, repo local.LocalRepo) ([]modelInfo, e
 	var infos []modelInfo
 	manifestDescs := repo.GetAllModels()
 	for _, manifestDesc := range manifestDescs {
-		manifest, config, err := util.GetManifestAndConfig(ctx, repo, manifestDesc)
-		if err != nil && !errors.Is(err, util.ErrNotAModelKit) {
-			return nil, err
+		manifest, config, err := util.GetManifestAndKitfile(ctx, repo, manifestDesc)
+		if err != nil {
+			if errors.Is(err, util.ErrNotAModelKit) {
+				// Shouldn't happen since this is a local repo, but either way it's not a supported artifact
+				continue
+			}
+			// Allow artifacts without Kitfiles as all that will be lacking is some metadata; we can still
+			// describe them
+			if !errors.Is(err, util.ErrNoKitfile) {
+				return nil, err
+			}
 		}
 		tags := repo.GetTags(manifestDesc)
 		// Strip localhost from repo if present, since we added it

pkg/cmd/list/remote.go

@@ -77,8 +77,8 @@ func listImageTag(ctx context.Context, repo registry.Repository, ref *registry.R
 	if err != nil {
 		return nil, fmt.Errorf("failed to resolve reference %s: %w", ref.Reference, err)
 	}
-	manifest, config, err := util.GetManifestAndConfig(ctx, repo, manifestDesc)
-	if err != nil {
+	manifest, config, err := util.GetManifestAndKitfile(ctx, repo, manifestDesc)
+	if err != nil && !errors.Is(err, util.ErrNoKitfile) {
 		return nil, fmt.Errorf("failed to read modelkit: %w", err)
 	}
 	if _, err := mediatype.ModelFormatForManifest(manifest); err != nil {

pkg/cmd/list/util.go

@@ -52,6 +52,8 @@ func (m *modelInfo) format() []string {
 	return lines
 }
 
+// fill adds information pulled from a manifest and kitfile into the modelInfo. Handles
+// cases where the Kitfile is nil
 func (m *modelInfo) fill(manifest *ocispec.Manifest, kitfile *artifact.KitFile) {
 	m.Size = getModelSize(manifest)
 	m.Author = getModelAuthor(kitfile)
@@ -67,15 +69,18 @@ func getModelSize(manifest *ocispec.Manifest) string {
 }
 
 func getModelAuthor(kitfile *artifact.KitFile) string {
-	if len(kitfile.Package.Authors) > 0 {
+	if kitfile != nil && len(kitfile.Package.Authors) > 0 {
 		return kitfile.Package.Authors[0]
 	} else {
 		return "<none>"
 	}
 }
 
 func getModelName(kitfile *artifact.KitFile) string {
-	name := kitfile.Package.Name
+	name := ""
+	if kitfile != nil {
+		name = kitfile.Package.Name
+	}
 	if name == "" {
 		name = "<none>"
 	}

pkg/cmd/list/util_test.go

@@ -0,0 +1,85 @@
+// Copyright 2024 The KitOps Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package list
+
+import (
+	"testing"
+
+	"github.com/kitops-ml/kitops/pkg/artifact"
+	"github.com/opencontainers/image-spec/specs-go"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestModelInfoFillNormal(t *testing.T) {
+	info := modelInfo{}
+	manifest := genTestManifest(100, 200, 300)
+	kitfile := &artifact.KitFile{
+		Package: artifact.Package{
+			Name:    "testmodelkit",
+			Authors: []string{"testauthor1", "testauthor2"},
+		},
+	}
+	info.fill(manifest, kitfile)
+	// Not testing size formatting here
+	assert.Equal(t, info.Size, "600 B")
+	// Should use first author in list
+	assert.Equal(t, info.Author, "testauthor1")
+	assert.Equal(t, info.ModelName, "testmodelkit")
+}
+
+func TestModelInfoFillEmptyKitfile(t *testing.T) {
+	info := modelInfo{}
+	manifest := genTestManifest(100, 200, 300)
+	kitfile := &artifact.KitFile{}
+	info.fill(manifest, kitfile)
+	// Not testing size formatting here
+	assert.Equal(t, info.Size, "600 B")
+	// Should use first author in list
+	assert.Equal(t, info.Author, "<none>")
+	assert.Equal(t, info.ModelName, "<none>")
+}
+
+func TestModelInfoFillNilKitfile(t *testing.T) {
+	info := modelInfo{}
+	manifest := genTestManifest(100, 200, 300)
+	info.fill(manifest, nil)
+	// Not testing size formatting here
+	assert.Equal(t, info.Size, "600 B")
+	// Should use first author in list
+	assert.Equal(t, info.Author, "<none>")
+	assert.Equal(t, info.ModelName, "<none>")
+}
+
+func genTestManifest(layersizes ...int64) *ocispec.Manifest {
+	var layers []ocispec.Descriptor
+	for _, s := range layersizes {
+		layers = append(layers, ocispec.Descriptor{
+			// Empty media type is sufficient for now as we don't care about mediatypes
+			// when filling
+			MediaType: ocispec.DescriptorEmptyJSON.MediaType,
+			Digest:    ocispec.DescriptorEmptyJSON.Digest,
+			Size:      s,
+		})
+	}
+	return &ocispec.Manifest{
+		Versioned: specs.Versioned{SchemaVersion: 2},
+		MediaType: ocispec.MediaTypeImageManifest,
+		Config:    ocispec.DescriptorEmptyJSON,
+		Layers:    layers,
+	}
+}

pkg/cmd/pull/pull.go

@@ -19,6 +19,7 @@ package pull
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"strings"
@@ -68,8 +69,12 @@ func runPullRecursive(ctx context.Context, localRepo local.LocalRepo, opts *pull
 }
 
 func pullParents(ctx context.Context, localRepo local.LocalRepo, desc ocispec.Descriptor, optsIn *pullOptions, pulledRefs []string) error {
-	_, config, err := util.GetManifestAndConfig(ctx, localRepo, desc)
+	_, config, err := util.GetManifestAndKitfile(ctx, localRepo, desc)
 	if err != nil {
+		if errors.Is(err, util.ErrNoKitfile) {
+			// If there's no Kitfile but it's otherwise a support artifact type, skip pulling parents as there aren't any
+			return nil
+		}
 		return err
 	}
 	if config.Model == nil || !util.IsModelKitReference(config.Model.Path) {

pkg/lib/filesystem/unpack/core.go

@@ -35,6 +35,7 @@ import (
 	"github.com/kitops-ml/kitops/pkg/lib/repo/util"
 	"github.com/kitops-ml/kitops/pkg/output"
 
+	modelspecv1 "github.com/modelpack/model-spec/specs-go/v1"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"oras.land/oras-go/v2/content"
 )
@@ -87,9 +88,19 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
 	}
 	config, err := util.GetKitfileForManifest(ctx, store, manifest)
 	if err != nil {
-		output.Infof("Could not get Kitfile: %s", err)
-	}
-	if config != nil {
+		if !errors.Is(err, util.ErrNoKitfile) {
+			return err
+		}
+		output.Logf(output.LogLevelWarn, "Could not get Kitfile: %s", err)
+		output.Logf(output.LogLevelWarn, "Functionality may be impacted")
+		// TODO: we can probably _also_ handle getting the model-spec config and using it here
+		genconfig, err := generateKitfileForModelPack(manifest)
+		if err != nil {
+			return fmt.Errorf("could not process manifest: %w", err)
+		}
+		config = genconfig
+	} else {
+		// These steps only make sense if we have a legitimate Kitfile available
 		if config.Model != nil && util.IsModelKitReference(config.Model.Path) {
 			output.Infof("Unpacking referenced modelkit %s", config.Model.Path)
 			if err := unpackParent(ctx, config.Model.Path, opts, visitedRefs); err != nil {
@@ -196,6 +207,7 @@ func unpackRecursive(ctx context.Context, opts *UnpackOptions, visitedRefs []str
 			}
 		}
 
+		// TODO: handle DiffIDs when unpacking layers
 		if err := unpackLayer(ctx, store, layerDesc, relPath, opts.Overwrite, opts.IgnoreExisting, mediaType.Compression()); err != nil {
 			return fmt.Errorf("failed to unpack: %w", err)
 		}
@@ -396,3 +408,40 @@ func getIndex(list []string, s string) int {
 	}
 	return -1
 }
+
+// generateKitfileForModelPack generates a "Kitfile" for a manifest that otherwise does not contain one.
+// This is a minimal Kitfile suitable only for unpacking, containing a path for every layer. If a layer
+// does not use the 'org.cncf.model.filepath' annotation, an error is returned.
+func generateKitfileForModelPack(manifest *ocispec.Manifest) (*artifact.KitFile, error) {
+	if format, err := mediatype.ModelFormatForManifest(manifest); err != nil || format != mediatype.ModelPackFormat {
+		return nil, fmt.Errorf("not a modelpack artifact")
+	}
+	kf := &artifact.KitFile{
+		Model: &artifact.Model{},
+	}
+	for _, desc := range manifest.Layers {
+		if desc.Annotations == nil || desc.Annotations[modelspecv1.AnnotationFilepath] == "" {
+			return nil, fmt.Errorf("unknown file path for layer: no %s annotation", modelspecv1.AnnotationFilepath)
+		}
+		filepath := desc.Annotations[modelspecv1.AnnotationFilepath]
+		mt, err := mediatype.ParseMediaType(desc.MediaType)
+		if err != nil {
+			return nil, err
+		}
+		switch mt.Base() {
+		case mediatype.ModelBaseType:
+			kf.Model.Path = filepath
+		case mediatype.ModelPartBaseType:
+			kf.Model.Parts = append(kf.Model.Parts, artifact.ModelPart{Path: filepath})
+		case mediatype.CodeBaseType:
+			kf.Code = append(kf.Code, artifact.Code{Path: filepath})
+		case mediatype.DatasetBaseType:
+			kf.DataSets = append(kf.DataSets, artifact.DataSet{Path: filepath})
+		case mediatype.DocsBaseType:
+			kf.Docs = append(kf.Docs, artifact.Docs{Path: filepath})
+		default:
+			return nil, fmt.Errorf("unknown layer type: %s", mt)
+		}
+	}
+	return kf, nil
+}

pkg/lib/kitfile/resolve.go

@@ -18,6 +18,7 @@ package kitfile
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -49,6 +50,9 @@ func GetKitfileForRef(ctx context.Context, configHome string, ref *registry.Refe
 	_, _, localKitfile, err := util.ResolveManifestAndConfig(ctx, localRepo, ref.Reference)
 	if err == nil {
 		return localKitfile, nil
+	} else if errors.Is(err, util.ErrNoKitfile) || errors.Is(err, util.ErrNotAModelKit) {
+		// We found an artifact but it does not have a Kitfile
+		return nil, err
 	}
 
 	repository, err := remote.NewRepository(ctx, ref.Registry, ref.Repository, options.DefaultNetworkOptions(configHome))

pkg/lib/repo/util/errors.go

@@ -19,3 +19,4 @@ package util
 import "errors"
 
 var ErrNotAModelKit = errors.New("reference exists but is not a modelkit")
+var ErrNoKitfile = errors.New("artifact does not contain a Kitfile")

pkg/lib/repo/util/reference.go

@@ -176,16 +176,17 @@ func RepoPath(storagePath string, ref *registry.Reference) string {
 	return filepath.Join(storagePath, ref.Registry, ref.Repository)
 }
 
-// GetManifestAndConfig returns the manifest and config (Kitfile) for a manifest Descriptor.
-// Calls GetManifest and GetConfig.
-func GetManifestAndConfig(ctx context.Context, store oras.ReadOnlyTarget, manifestDesc ocispec.Descriptor) (*ocispec.Manifest, *artifact.KitFile, error) {
+// GetManifestAndKitfile returns the manifest and config (Kitfile) for a manifest Descriptor.
+// Calls GetManifest and GetKitfileForManifest. If the manifest is retrieved but no Kitfile
+// can be found, returns the manifest and error equal to ErrNoKitfile
+func GetManifestAndKitfile(ctx context.Context, store oras.ReadOnlyTarget, manifestDesc ocispec.Descriptor) (*ocispec.Manifest, *artifact.KitFile, error) {
 	manifest, err := GetManifest(ctx, store, manifestDesc)
 	if err != nil {
 		return nil, nil, err
 	}
 	config, err := GetKitfileForManifest(ctx, store, manifest)
 	if err != nil {
-		return nil, nil, err
+		return manifest, nil, err
 	}
 	return manifest, config, nil
 }
@@ -208,6 +209,10 @@ func GetManifest(ctx context.Context, store oras.ReadOnlyTarget, manifestDesc oc
 	return manifest, nil
 }
 
+// GetKitfileForManifest returns the Kitfile for a given manifest, either by retrieving it from an
+// OCI store or by reading it from manifest annotations. If manifest type is unrecognized, returns
+// ErrNotAModelKit. If the manifest is recognized but does not contain a Kitfile (e.g. it was not
+// created by Kit), returns ErrNoKitfile.
 func GetKitfileForManifest(ctx context.Context, store oras.ReadOnlyTarget, manifest *ocispec.Manifest) (*artifact.KitFile, error) {
 	modelFormat, err := mediatype.ModelFormatForManifest(manifest)
 	if err != nil {
@@ -219,7 +224,7 @@ func GetKitfileForManifest(ctx context.Context, store oras.ReadOnlyTarget, manif
 	case mediatype.ModelPackFormat:
 		// TODO: can we (try to) generate a Kitfile from a ModelPack manifest?
 		if manifest.Annotations == nil || manifest.Annotations[constants.KitfileJsonAnnotation] == "" {
-			return nil, fmt.Errorf("ModelPack artifact does not contain a Kitfile")
+			return nil, ErrNoKitfile
 		}
 		kfstring := manifest.Annotations[constants.KitfileJsonAnnotation]
 		kitfile := &artifact.KitFile{}
@@ -228,7 +233,8 @@ func GetKitfileForManifest(ctx context.Context, store oras.ReadOnlyTarget, manif
 		}
 		return kitfile, nil
 	default:
-		return nil, fmt.Errorf("Unknown artifact type")
+		// Won't happen but necessary for completeness
+		return nil, fmt.Errorf("unknown artifact type")
 	}
 }
 
@@ -266,15 +272,16 @@ func ResolveManifest(ctx context.Context, store oras.Target, reference string) (
 }
 
 // ResolveManifestAndConfig returns the manifest and config (Kitfile) for a given reference (tag), if present
-// in the store. Calls ResolveManifest and GetConfig.
+// in the store. Calls GetManifest and GetKitfileForManifest. If the manifest is retrieved but no Kitfile
+// can be found, returns the manifest and error equal to ErrNoKitfile
 func ResolveManifestAndConfig(ctx context.Context, store oras.Target, reference string) (ocispec.Descriptor, *ocispec.Manifest, *artifact.KitFile, error) {
 	desc, manifest, err := ResolveManifest(ctx, store, reference)
 	if err != nil {
 		return ocispec.DescriptorEmptyJSON, nil, nil, err
 	}
 	config, err := GetKitfileForManifest(ctx, store, manifest)
 	if err != nil {
-		return ocispec.DescriptorEmptyJSON, nil, nil, err
+		return desc, manifest, nil, err
 	}
 	return desc, manifest, config, nil
 }