fix: check for .bin files during auto-detection of serving backend - #1809
Conversation
d7b06c4 to
4bdde20
Compare
booxter
left a comment
There was a problem hiding this comment.
Style change suggested; code-wise it's good I think.
Also, please re-wrap commit message lines to 50/72 rule. Thank you.
| with open( | ||
| safetensors_model_path / "model.safetensors", "a+", encoding="UTF-8" | ||
| ) as f: | ||
| model_file = ( |
There was a problem hiding this comment.
with open(model_path / f"model.{model_file_type}", ...):
pass
? (pass will be equivalent to f.write("") I think.)
There was a problem hiding this comment.
what would the benefit of making that change be?
There was a problem hiding this comment.
2 lines vs 10 lines? The nested ternary if-else is also rather convoluted.
There was a problem hiding this comment.
Personally I'm not a fan of this approach, as it increases the valid values for file types from just safetensors and bin to allow all valid strings.
We should instead keep the values constricted to just safetensors and bin as those are types which we explicitly support, and instead throw a ValueError if the provided model type ever deviates.
In all likelihood, this function shouldn't be used outside of this codebase or even that much, so we shouldn't run into this error in the first place.
There was a problem hiding this comment.
So we can just make the following change:
| model_file = ( | |
| if model_file_type not in ("safetensors", "bin"): | |
| raise ValueError(f"model file type is not supported: {model_file_type}") | |
| with open(os.path.join(model_path, f"model.{model_file_type}"), "a+", encoding="UTF-8"): | |
| pass |
There was a problem hiding this comment.
since this is test case, and if you'd like to raise an error, you can also assert model_file_type in (...)
There was a problem hiding this comment.
(that said, I think there's a test variant here that actually passes "other".)
There was a problem hiding this comment.
@booxter ah sorry - I totally misread what you were suggesting
have updated the code to match that, thanks! 🙏
@RobotSail this is just a test case so I think we do need to create valid and invalid scenarios to properly test the code
Interesting @booxter , wasn't familiar with this rule but it seems like we haven't really been following it in this repository |
|
@jaideepr97 Yes, commit messages in this repo are most often than not non-compliant with 50/72. I don't think the project ever made an explicit decision that the rule should be enforced, but it shouldn't stop us from applying the best practice. Core @russellb should the project formally document some rules for commit messages? |
Yes. I started instructlab/dev-docs#110 haven't prioritized getting back to it to finish it up, though. |
4bdde20 to
727e478
Compare
|
updated the commit to follow 50/72 rule @booxter Thanks for all the inputs, everyone! |
727e478 to
df01f54
Compare
Current check for auto-detection of backend requires a vLLM friendly model's directory to contain *.safetensors files to qualify as valid. However, we should also accept the presence of *.bin files in place of *.safetensors files as training outputs .bin checkpoints that can be served through vLLM. Signed-off-by: Jaideep Rao <jrao@redhat.com>
df01f54 to
e5c46d3
Compare
Current check for auto-detection of backend requires a vLLM friendly model's directory to contain
*.safetensorsfiles to qualify as valid. However, we should also accept the presence of*.binfiles in place of*.safetensorsfiles as training outputs.bincheckpoints that can be served through vLLMChecklist:
conventional commits.