removes tokenizer.model from required safetensors model files" - #1793
Conversation
a3fccd7 to
a8129ba
Compare
| f.write("") | ||
| if valid: | ||
| with open( | ||
| safetensors_model_path / "tokenizer.model", "a+", encoding="UTF-8" |
There was a problem hiding this comment.
needed to remove this from the test too haha
a8129ba to
fc3872e
Compare
|
Requires test fix that's updated in #1791 |
leseb
left a comment
There was a problem hiding this comment.
On paper, the change makes sense to me, please:
- look into the test failures
- update your commit message to match the PR description
Thanks!
jaideepr97
left a comment
There was a problem hiding this comment.
@JamesKunstle LGTM as well, sorry for breaking this for you!
will approve after all CI checks pass
fc3872e to
e7a84f6
Compare
@leseb I updated the commit message- the current test failure is because I'm fixing the code and the test at once- how do we handle this failure state? |
@JamesKunstle not sure I follow, it's pretty standard to make code and test changes in the same PR, no? The CI should just run the test as is in your branch |
You're right, was typing too quickly. I think this actually breaks the test- will investigate. |
tokenizer.model isn't required for a model directory to be a .safetensors model- this broke model loading for me because the checkpoints that we save don't have this file. This appears to be a common, but not a required file. (look at metaai/llama3-8b) Signed-off-by: James Kunstle <jkunstle@redhat.com>
e7a84f6 to
240f1cc
Compare
|
answer: test setup should've written json_object to config.json rather than empty string. |
tokenizer.modelisn't required for a model directory to be a.safetensorsmodel- this broke model loading for me because the checkpoints that we save don't have this file. This appears to be a common, but not a required file. (look at metaai/llama3-8b)