Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 55 additions & 3 deletions cmd/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,17 @@ package cmd
import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/spf13/cobra"
"github.com/spf13/viper"

"github.com/modelpack/modctl/pkg/backend"
"github.com/modelpack/modctl/pkg/config"

configmodelfile "github.com/modelpack/modctl/pkg/config/modelfile"
modelfilegen "github.com/modelpack/modctl/pkg/modelfile"
)

var buildConfig = config.NewBuild()
Expand Down Expand Up @@ -62,24 +67,71 @@ func init() {
flags.BoolVar(&buildConfig.Raw, "raw", true, "turning on this flag will build model artifact layers in raw format")
flags.BoolVar(&buildConfig.Reasoning, "reasoning", false, "turning on this flag will mark this model as reasoning model in the config")
flags.BoolVar(&buildConfig.NoCreationTime, "no-creation-time", false, "turning on this flag will not set createdAt in the config, which will be helpful for repeated builds")
flags.BoolVar(&buildConfig.Regenerate, "regenerate", false, "force regenerate Modelfile before building")

if err := viper.BindPFlags(flags); err != nil {
panic(fmt.Errorf("bind cache list flags to viper: %w", err))
}

}

// runBuild runs the build modctl.
func runBuild(ctx context.Context, workDir string) error {

// Determine modelfile path
modelfilePath := buildConfig.Modelfile
if modelfilePath == "" {
modelfilePath = filepath.Join(workDir, configmodelfile.DefaultModelfileName)
} else if !filepath.IsAbs(modelfilePath) {
modelfilePath = filepath.Join(workDir, modelfilePath)
}
Comment on lines +81 to +86
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The modelfilePath is not correctly resolved against the workDir. When a relative path is used for the modelfile (which is the default), os.Stat(modelfilePath) will check for the file in the current working directory of the modctl process, not in the specified workDir. This can lead to incorrect behavior, such as failing to find an existing Modelfile or generating a new one in the wrong location. The path should be explicitly joined with workDir if it's relative.

Suggested change
modelfilePath := buildConfig.Modelfile
if modelfilePath == "" {
modelfilePath = filepath.Join(workDir, configmodelfile.DefaultModelfileName)
}
modelfilePath := buildConfig.Modelfile
if modelfilePath == "" {
modelfilePath = filepath.Join(workDir, configmodelfile.DefaultModelfileName)
} else if !filepath.IsAbs(modelfilePath) {
modelfilePath = filepath.Join(workDir, modelfilePath)
}


shouldGenerate := buildConfig.Regenerate

if shouldGenerate {
fmt.Println("Regenerate flag detected. Regenerating Modelfile...")
} else {
_, err := os.Stat(modelfilePath)
if os.IsNotExist(err) {
fmt.Println("No Modelfile found. Generating automatically...")
shouldGenerate = true
} else if err != nil {
return fmt.Errorf("error checking Modelfile at %s: %w", modelfilePath, err)
}
}

if shouldGenerate {
genConfig := configmodelfile.NewGenerateConfig()

absWorkDir, err := filepath.Abs(workDir)
if err != nil {
return fmt.Errorf("failed to resolve workspace path: %w", err)
}
Comment on lines +103 to +108
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The setup for genConfig is a bit confusing. genConfig.Output is set and then converted to an absolute path inside genConfig.Convert, but this value is never used. The only relevant side effect of genConfig.Convert seems to be making genConfig.Workspace an absolute path. This can be simplified to make the intent clearer and reduce reliance on the side effects of Convert.

Suggested change
genConfig := configmodelfile.NewGenerateConfig()
genConfig.Workspace = workDir
genConfig.Output = workDir
genConfig.Overwrite = true
if err := genConfig.Convert(workDir); err != nil {
return fmt.Errorf("failed to prepare modelfile generation: %w", err)
}
genConfig := configmodelfile.NewGenerateConfig()
absWorkDir, err := filepath.Abs(workDir)
if err != nil {
return fmt.Errorf("failed to resolve workspace path: %w", err)
}
genConfig.Workspace = absWorkDir
genConfig.Overwrite = true


genConfig.Workspace = absWorkDir
genConfig.Overwrite = true

mf, err := modelfilegen.NewModelfileByWorkspace(genConfig.Workspace, genConfig)
if err != nil {
return fmt.Errorf("failed to auto-generate modelfile: %w", err)
}

content := mf.Content()
if err := os.WriteFile(modelfilePath, content, 0644); err != nil {
return fmt.Errorf("failed to write modelfile: %w", err)
}

fmt.Printf("Successfully generated %s\n", modelfilePath)
}

b, err := backend.New(rootConfig.StoargeDir)
if err != nil {
return err
}

if err := b.Build(ctx, buildConfig.Modelfile, workDir, buildConfig.Target, buildConfig); err != nil {
if err := b.Build(ctx, modelfilePath, workDir, buildConfig.Target, buildConfig); err != nil {
return err
}

fmt.Printf("Successfully built model artifact: %s\n", buildConfig.Target)

return nil
}
2 changes: 2 additions & 0 deletions pkg/config/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ type Build struct {
Raw bool
Reasoning bool
NoCreationTime bool
Regenerate bool
}

func NewBuild() *Build {
Expand All @@ -52,6 +53,7 @@ func NewBuild() *Build {
Raw: false,
Reasoning: false,
NoCreationTime: false,
Regenerate: false,
}
}

Expand Down