convert.py safetensors updates #4043
Conversation
| @@ -1036,7 +1036,8 @@ def load_some_model(path: Path) -> ModelPlus: | |||
| # Be extra-friendly and accept either a file or a directory: | |||
| if path.is_dir(): | |||
| # Check if it's a set of safetensors files first | |||
| files = list(path.glob("model-00001-of-*.safetensors")) | |||
| globs = ["model-00001-of-*.safetensors", "model.safetensors"] | |||
There was a problem hiding this comment.
Why not just *.safetensors? That's the common approach.
There was a problem hiding this comment.
I thought the same thing, but that could, under some absurd circumstances cause problems. We never know what people do with their stuff... Might wait for someone with another option though
There was a problem hiding this comment.
@AlpinDale The glob looks like it's deliberately trying to target the first part of the set with model-00001-of-*. If it was just *.safetensors then you could get model-99999-of-99999.safetensors which is probably not what you want to load.
There was a problem hiding this comment.
Python indexes the files alphabetically when using a glob, so that is a non-issue. I'm simply pointing out that this way of doing it is unconventional and I've not seen any other project do this.
There was a problem hiding this comment.
Python indexes the files alphabetically when using a glob
I'm pretty sure that's not the case. The documentation doesn't even mention order: https://docs.python.org/3/library/pathlib.html#pathlib.Path.glob
Note also that their examples are like sorted(Path('.').glob('*.py')) which would be redundant if it was guaranteed to be already sorted.
I'm simply pointing out that this way of doing it is unconventional
That may be the case, but your proposed change would break it. There's actually a
1046 │ if len(files) > 1:
1047 │ raise # ...a couple lines down. This is specifically supposed to pull in the first file of the set, not all of them.
There was a problem hiding this comment.
This looks like a good improvement in its current state.
@afrideva Was there anything else you want to change before the pull gets merged?
Looks good to me |
* add safetensors to convert.py help message * Check for single-file safetensors model * Update convert.py "model" option help message * revert convert.py help message change
* add safetensors to convert.py help message * Check for single-file safetensors model * Update convert.py "model" option help message * revert convert.py help message change
Encountered several repos with a single file "model.safetensors" that would fail to convert, example: https://huggingface.co/mesolitica/malaysian-tinyllama-1.1b-16384-instructions/tree/main
Found #3097 while searching for existing PR's, should be safe to close now.