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
types/model: make Name.Filepath substitute colons in host with ("%") #4107
base: main
Are you sure you want to change the base?
Conversation
@@ -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), |
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.
Do we want to do this only on windows? OR we may need a migration path for non windows users.
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.
Probably will need a migration path. Switching back and forth will make things complicated. I recommend we stick with one way.
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.
only doing this for windows risk the same problem as sha256:
on ntfs filesystem. this should probably be for all operating systems
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.
fwiw the number of users affected would be low since most will be using the default host
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.
Updated with migration. PTAL.
53241e1
to
cb3f445
Compare
if err != nil { | ||
return err | ||
} | ||
if info.IsDir() { |
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.
Why not rename just the directory?
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 was easier to reason about how to find the host once I had a file (e.g. tag).
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 believe we should only need to replace %
for the dirs in ~/.ollama/models/manifests/
, not sure if the code below this is required?
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.
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?
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 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.
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"}},
cb3f445
to
32a1246
Compare
This makes the filepath legal on all supported platforms. Fixes #4088
32a1246
to
61b287c
Compare
if err != nil { | ||
return err | ||
} | ||
if err := fixManifests(manifestsDir); err != 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 think we should add a comment to specify that this is a migration and why it exists.
if err != nil { | ||
return err | ||
} | ||
if info.IsDir() { |
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 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.
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"}},
This makes the filepath legal on all supported platforms.
Fixes #4088