fix: check for .bin files during auto-detection of serving backend

[jaideepr97]

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

Checklist:

• Commit Message Formatting: Commit titles and messages follow guidelines in the
  conventional commits.
• Changelog updated with breaking and/or notable changes for the next minor release.
• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• Integration tests have been added, if necessary.

[booxter]

Style change suggested; code-wise it's good I think.

Also, please re-wrap commit message lines to 50/72 rule. Thank you.

[booxter]

with open(model_path / f"model.{model_file_type}", ...):
    pass

? (pass will be equivalent to f.write("") I think.)

[jaideepr97]

what would the benefit of making that change be?

[booxter]

2 lines vs 10 lines? The nested ternary if-else is also rather convoluted.

[RobotSail]

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.

[RobotSail]

So we can just make the following change:

Suggested 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

[booxter]

since this is test case, and if you'd like to raise an error, you can also assert model_file_type in (...)

[booxter]

(that said, I think there's a test variant here that actually passes "other".)

[jaideepr97]

@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

[jaideepr97]

Also, please re-wrap commit message lines to 50/72 rule. Thank you.

Interesting @booxter , wasn't familiar with this rule but it seems like we haven't really been following it in this repository
I've mostly been asked to reuse the PR description as the commit msg

[booxter]

@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 git tools like git-log expect that the rule is adhered. And while we can discuss if 72 or 100 limit is the best for the project, having SOME limit and not piling the commit message into a single line shouldn't be controversial.

@russellb should the project formally document some rules for commit messages?

[russellb]

@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 git tools like git-log expect that the rule is adhered. And while we can discuss if 72 or 100 limit is the best for the project, having SOME limit and not piling the commit message into a single line shouldn't be controversial.

@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.

[Maxusmusti]

LGTM

[RobotSail]

LGTM

[leseb]

@booxter do you have request changes left? Added the hold flag until your concerns are resolved. Thanks

[jaideepr97]

updated the commit to follow 50/72 rule @booxter
sorry for the delay

Thanks for all the inputs, everyone!