Compatibility with AzureOpenAI #67
Conversation
feat: added azure model name matching to find_model
|
Thanks for this work @NP4567-dev, I'll be reviewing it next week! 🙏 |
There was a problem hiding this comment.
Before merging, can you add some tests for AzureOpenAI clients?
You can copy and adapt the tests from test_openai.py and add them to the same file. The objective is to make sure that we stay compatible with Azure's client as well.
To create the test, you will need an Azure account, which I assume you have. Add your API key and endpoint as environment variables AZURE_OPENAI_API_KEY and AZURE_OPENAI_ENDPOINT. Also add fake env var in conftest.py. Run the tests with make test-record as stated in the newly added contribution docs.
Don't hesitate to comment if you have issues adding these tests! 🤗
ecologits/model_repository.py
Outdated
| for model in self.__models: | ||
| # To handle specific LiteLLM calling (e.g., mistral/mistral-small) | ||
| if model.provider == provider and model.name in model_name: | ||
| return model | ||
| return None | ||
| provider_models = [model for model in self.__models if model.provider == provider] | ||
| return next( | ||
| ( | ||
| model | ||
| for model in provider_models | ||
| if (model.name == model_name or model.name.replace(".", "") == model_name) | ||
| ), | ||
| next((model for model in provider_models if model.name in model_name), None), | ||
| ) |
There was a problem hiding this comment.
Instead of changing this function, I would rather add the models to the models.csv file. For instance, gpt-35-turbo will be a duplicated line of gpt-3.5-turbo. We plan to refactor the model repository to introduce aliases in the future.
There was a problem hiding this comment.
Ok perfect, adding some kind of aliases mechanic was something I tought of but felt was a bit overkill for just azure.
This function still needs to be changed at some point though. Currently it seems like gpt-4-turbo and gpt-o match with gpt-4. (I can move this to another PR/open an issue if needed as this is a separated problem). Or aliases can fix this too.
There was a problem hiding this comment.
Agree, I think this bug was introduced when adding litellm, will get it fixed in the next release for sure! 🙏
There was a problem hiding this comment.
Adding the tests is proving to be a bit more complicated than I anticipated.
Python-recording or the underlying vcrpy seems to always be trying to overwrite some cassettes.
Running "make test" raises vcr.errors.CannotOverwriteExistingCassetteExceptionon on google, huggingface and litellm tests. Even after cloning and reinstalling from scratch this still happens.
I did manage to generate a cassette for azure openai, but whenever I try to run the tests using it, the same error is raised.
Any idea on how to solve this?
There was a problem hiding this comment.
Ok thanks for letting me know. That is indeed a strange behavior... Can you push your code as it is right now? So I can try on my side, see if I have the same issue.
There was a problem hiding this comment.
@NP4567-dev I think I have found your work on your fork (branch feature/azure-tests).
The issue you have is probably because the name of the tests you've added for Azure OpenAI are equal to the tests for OpenAI.
Meaning that if you change the function names from test_openai_chat to test_azure_openai_chat in the test_azure_openai.py file, it should work properly. :)
There was a problem hiding this comment.
Yeah, this was another problem I had already fixed locally.
I did find the issue though, we have a proxy that prevented url matching on the cassettes, hence the recording attempt.
It is fixed now, I just have a few final verification to make and it should be ready to merge
* revert: roll back find_model and deleted related tests, updated data csv * feat: review changes * feat: added tests and slight fix * fix: removed extra csv rows * fix: rolled back env specific changes * fix: rolled back env specific changes
* revert: roll back find_model and deleted related tests, updated data csv * feat: review changes * feat: added tests and slight fix * fix: removed extra csv rows * fix: rolled back env specific changes * fix: rolled back env specific changes
Adressing: #60
Context:
Though the wrapped class is the same for Azure OpenAI, there is a difference in the model names: azure does not use dots in the name (chatgpt-3.5 in openai API becomes chatgpt-35 in azure).
Impacts:
Rework of the find_model method in two steps
This prevents gpt-4-turbo matching gtp-4 for example.
The choice was made not to add azure as a provider and to consider a model running on either azure or openai infrastructure has the same impacts, this could be discussed.
Side effects:
As of right now, there is no side effects (no false positive matching) but further down the line too many provider specific matching rules could become a hassle to maintain.