types/model: make Name.Filepath substitute colons in host with ("%") #4107
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.
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.
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.
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.
fwiw the number of users affected would be low since most will be using the default host
There was a problem hiding this comment.
Updated with migration. PTAL.
bmizerany
force-pushed
the
bmizerany/filepathwithcoloninhost
branch
from
53241e1
to
cb3f445
Compare
| if err != nil { | ||
| return err | ||
| } | ||
| if info.IsDir() { |
There was a problem hiding this comment.
Why not rename just the directory?
There was a problem hiding this comment.
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.
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.
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.
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
bmizerany
force-pushed
the
bmizerany/filepathwithcoloninhost
branch
from
32a1246
to
61b287c
Compare
| if err != nil { | ||
| return err | ||
| } | ||
| if err := fixManifests(manifestsDir); err != nil { |
There was a problem hiding this comment.
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.
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