Implement a JSON responding OpenAI LLM as JSONOpenAILLM #331
Conversation
There was a problem hiding this comment.
Thanks @burtenshaw! I left some comments, also we use pytest for the tests instead of unittest (other than using the mock module), if you could update those. Once the tests pass, looks good to me
Co-authored-by: Agus <agustin.piqueres@gmail.com>
src/distilabel/llm/openai.py
Outdated
| self.json_supporting_models = [ | ||
| "gpt-4-0125-preview", | ||
| "gpt-4-turbo-preview", | ||
| "gpt-4-1106-preview", | ||
| "gpt-3.5-turbo-1106", | ||
| ] | ||
| assert model in self.json_supporting_models, f"Provided `model` does not support JSON input, \ | ||
| available models are {self.json_supporting_models}" |
There was a problem hiding this comment.
Maybe it's just better to override the available_models property instead?
There was a problem hiding this comment.
i.e. pull the models from OpenAI and filter the default models so that just those are used, and keep the fine-tunes, as we're unsure about those
There was a problem hiding this comment.
Meaning the same property as available_models within OpenAILLM but with a slight filtering on the proprietary models offered by OpenAI
There was a problem hiding this comment.
@alvarobartt Thanks for your feedback.
like this?
@cached_property
def available_models(self) -> List[str]:
"""Returns the list of available models in your OpenAI account."""
all_available_models = super().available_models
json_supporting_models = [
"gpt-4-0125-preview",
"gpt-4-turbo-preview",
"gpt-4-1106-preview",
"gpt-3.5-turbo-1106",
]
return list(set(all_available_models) & set(json_supporting_models))It makes sense to combine the functionality. I'm just worried about making the logging clear when the model name is wrong.
In the situation where the user gives an incorrect model name, we should make it clear to the user whether it's incorrect because it doesn't support JSON or it isn't available on their account.
There was a problem hiding this comment.
I initially went for the separate approach because the account is asserted, then JSON compatibility is asserted.
🤔 I can of course just log in the available_models method if there are no JSON compatible model in the account's models.
There was a problem hiding this comment.
Hmm fair I get your point now :/ Then you can leave it as is for the moment, and if any issue arises we can revisit, but the current is fine too!
Co-authored-by: Alvaro Bartolome <alvarobartt@gmail.com>
|
Would close #328 |
* Implement JSON OpenAILLM by inheriting from OpenAILLM * mock and test JSONOPenAILLM * refactor test to decouple available models * add documentation for JSONOpenAILLM * expose JSONOpenAILLM in distilabel.llms * sort and format * sort and format * Update tests/llm/test_openai.py Co-authored-by: Agus <agustin.piqueres@gmail.com> * switch testing from unittest to pytest * sorting * extra assert in test_generate * format * refactor tests to mock out OpenAI client * Update src/distilabel/llm/openai.py Co-authored-by: Alvaro Bartolome <alvarobartt@gmail.com> * combine available models method * use mocked openai in tests * testing: move assert raise catch to fixture * test: refactor fixture to not use globals * refactor mocking for simplicity share assertion --------- Co-authored-by: Agus <agustin.piqueres@gmail.com> Co-authored-by: Alvaro Bartolome <alvarobartt@gmail.com>
This pr implements a new class in
distilabel.llm.openainamedJSONOpenAILLM.The class uses the
json_responsefeature from Open AI and validates that the model selected is present in a hard coded list of models that support this feature.