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.

Sign up for GitHub

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

Jump to bottom

types/model: make Name.Filepath substitute colons in host with ("%") #4107

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

types/model: make Name.Filepath substitute colons in host with ("%") #4107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 42 additions & 0 deletions server/fixblobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package server
import (
"os"
"path/filepath"
"runtime"
"strings"
)

Expand All @@ -24,3 +25,44 @@ func fixBlobs(dir string) error {
return nil
})
}

// fixManifests walks the provided dir and replaces (":") to ("%") for all
// manifest files on non-Windows systems.
func fixManifests(dir string) error {
if runtime.GOOS == "windows" {
return nil
}
return filepath.Walk(dir, func(oldPath string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not rename just the directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was easier to reason about how to find the host once I had a file (e.g. tag).

Copy link
Member

Choose a reason for hiding this comment

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

I believe we should only need to replace % for the dirs in ~/.ollama/models/manifests/, not sure if the code below this is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should only change the host part of the path. @mxyng @jmorganca Do you have any suggestions for making it more clear? It's my understanding this is a migration script only, not a critical path, so I gave it a quick pass to get us form a to b.

This patch comes with tests. Do you have additional test cases in mind that prove this renames more than the host part of the path?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what Jeff is saying is that we can simplify this a bit to only rename the manifest directories, and not worry about their contents. The ports should only exist on the manifest directories since we don't allow them within the model names/tags.

image

More concretely we wont need this test:

		{path: []string{"x:y/h:p/n/m/t"}, want: []string{"x:y/h%p/n/m/t"}},

return nil
}

var partNum int
newPath := []byte(oldPath)
for i := len(newPath) - 1; i >= 0; i-- {
if partNum > 3 {
break
}
if partNum == 3 {
if newPath[i] == ':' {
newPath[i] = '%'
break
}
continue
}
if newPath[i] == '/' {
partNum++
}
}

newDir, _ := filepath.Split(string(newPath))
if err := os.MkdirAll(newDir, 0o755); err != nil {
return err
}

return os.Rename(oldPath, string(newPath))
})
}
49 changes: 49 additions & 0 deletions server/fixblobs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,55 @@ func TestFixBlobs(t *testing.T) {
}
}

func TestFixManifests(t *testing.T) {
cases := []struct {
path []string
want []string
}{
{path: []string{}, want: []string{}},
{path: []string{"h/n/m/t"}, want: []string{"h/n/m/t"}},
{path: []string{"h:p/n/m/t"}, want: []string{"h%p/n/m/t"}},
{path: []string{"x:y/h:p/n/m/t"}, want: []string{"x:y/h%p/n/m/t"}},
}

for _, tt := range cases {
t.Run(strings.Join(tt.path, "|"), func(t *testing.T) {
hasColon := slices.ContainsFunc(tt.path, func(s string) bool { return strings.Contains(s, ":") })
if hasColon && runtime.GOOS == "windows" {
t.Skip("skipping test on windows")
}

rootDir := t.TempDir()
for _, path := range tt.path {
fullPath := filepath.Join(rootDir, path)
fullDir, _ := filepath.Split(fullPath)

t.Logf("creating dir %s", fullDir)
if err := os.MkdirAll(fullDir, 0o755); err != nil {
t.Fatal(err)
}

t.Logf("writing file %s", fullPath)
if err := os.WriteFile(fullPath, nil, 0o644); err != nil {
t.Fatal(err)
}
}

if err := fixManifests(rootDir); err != nil {
t.Fatal(err)
}

got := slurpFiles(os.DirFS(rootDir))

slices.Sort(tt.want)
slices.Sort(got)
if !slices.Equal(got, tt.want) {
t.Fatalf("got = %v, want %v", got, tt.want)
}
})
}
}

func slurpFiles(fsys fs.FS) []string {
var sfs []string
fn := func(path string, d fs.DirEntry, err error) error {
Expand Down
7 changes: 7 additions & 0 deletions server/routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,13 @@ func Serve(ln net.Listener) error {
if err := fixBlobs(blobsDir); err != nil {
return err
}
manifestsDir, err := GetManifestPath()
if err != nil {
return err
}
if err := fixManifests(manifestsDir); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add a comment to specify that this is a migration and why it exists.

return err
}

if noprune := os.Getenv("OLLAMA_NOPRUNE"); noprune == "" {
// clean up unused layers and manifests
Expand Down
4 changes: 2 additions & 2 deletions types/model/name.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func ParseNameBare(s string) Name {
}

scheme, host, ok := strings.Cut(s, "://")
if ! ok {
if !ok {
host = scheme
}
n.Host = host
Expand Down Expand Up @@ -243,7 +243,7 @@ func (n Name) Filepath() string {
panic("illegal attempt to get filepath of invalid name")
}
return strings.ToLower(filepath.Join(
n.Host,
strings.Replace(n.Host, ":", "%", 1),
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to do this only on windows? OR we may need a migration path for non windows users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably will need a migration path. Switching back and forth will make things complicated. I recommend we stick with one way.

Copy link
Contributor

Choose a reason for hiding this comment

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

only doing this for windows risk the same problem as sha256: on ntfs filesystem. this should probably be for all operating systems

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw the number of users affected would be low since most will be using the default host

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with migration. PTAL.

n.Namespace,
n.Model,
n.Tag,
Expand Down
15 changes: 12 additions & 3 deletions types/model/name_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestParseNameParts(t *testing.T) {
Model: "model",
Tag: "tag",
},
wantFilepath: filepath.Join("host:port", "namespace", "model", "tag"),
wantFilepath: filepath.Join("host%port", "namespace", "model", "tag"),
},
{
in: "host/namespace/model:tag",
Expand All @@ -47,7 +47,7 @@ func TestParseNameParts(t *testing.T) {
Model: "model",
Tag: "tag",
},
wantFilepath: filepath.Join("host:port", "namespace", "model", "tag"),
wantFilepath: filepath.Join("host%port", "namespace", "model", "tag"),
},
{
in: "host/namespace/model",
Expand All @@ -65,7 +65,7 @@ func TestParseNameParts(t *testing.T) {
Namespace: "namespace",
Model: "model",
},
wantFilepath: filepath.Join("host:port", "namespace", "model", "latest"),
wantFilepath: filepath.Join("host%port", "namespace", "model", "latest"),
},
{
in: "namespace/model",
Expand Down Expand Up @@ -127,6 +127,15 @@ func TestParseNameParts(t *testing.T) {
},
wantValidDigest: true,
},
{
in: "y.com:443/n/model",
want: Name{
Host: "y.com:443",
Namespace: "n",
Model: "model",
},
wantFilepath: filepath.Join("y.com%443", "n", "model", "latest"),
},
}

for _, tt := range cases {
Expand Down