feat: Integrate Azure OpenAI API. #402

L4zyy · 2023-12-07T05:17:44Z

Description

This PR integrate the Azure OpenAI API to CAMEL.

Motivation and Context

Integrate Azure OpenAI API, close #231

I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds core functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (update in the documentation)
Example (update in the folder of example)

Implemented Tasks

Add Azure OpenAI model.
Add Azure OpenAI model tests.
Run though role playing example using Azure OpenAI API.

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

I have read the CONTRIBUTION guide. (required)
My change requires a change to the documentation.
I have updated the tests accordingly. (required for a bug fix or a new feature)
I have updated the documentation accordingly.

L4zyy · 2023-12-09T13:05:38Z

Hi, @lightaime, I've checked all the failed tests and nearly all of them are "openai.APIConnectionError: Connection error.". I found a comment in this MR may related to my problem. I run some of the failed tests locally and all of them are passed. Could you help me with this?

There are also two other failed test.

"Failed: OPENWEATHERMAP_API_KEY environment variable is not set.", Is this fail with the same reason?
"openai.OpenAIError: Missing credentials. Please pass one of api_key, azure_ad_token, azure_ad_token_provider, or the AZURE_OPENAI_API_KEY or AZURE_OPENAI_AD_TOKEN environment variables.". This test failed since my MR needs new environment variables to run with Azure OpenAI API(I update the README about this). Is there any way to update environment variables when testing?

lightaime

Thanks @L4zyy for the PR! Overall looks good. I have some questions

About the AzureOpenAIModel, do we need a seperate class for azure openai models? It seems the only differences between AzureOpenAIModel and OpenAIModel will be the creation of self._client. Should we just support azure models with OpenAIModel?
Should we just introduce enums in https://github.com/camel-ai/camel/blob/master/camel/types/enums.py to support the client creation? We can introduce somethings like: ModelType.Azure_GPT_3_5_TURBO and so on.

@dandansamax @Wendong-Fan any suggesitions?

lightaime · 2023-12-10T03:25:20Z

camel/models/azure_openai_model.py

+azure_model_name_to_type = {
+    "gpt-35-turbo": ModelType.GPT_3_5_TURBO,
+    "gpt-35-turbo-16k": ModelType.GPT_3_5_TURBO_16K,
+    "gpt-4": ModelType.GPT_4,
+    "gpt-4-32k": ModelType.GPT_4_32K,
+    "gpt-4-1106-preview": ModelType.GPT_4_TURBO,
+}


What is this used for?

Code for an older implementation, will be remove in the next commit.

lightaime · 2023-12-10T03:27:12Z

camel/models/azure_openai_model.py

+        assert self.deployment_name is not None, \
+            "Azure model deployment name is not provided."
+
+        self.model_name = os.environ.get('AZURE_MODEL_TOKEN_LIMIT', 8192)


Why is it called model_name?

Apologize for this typo, will be correct in the next commit.

lightaime · 2023-12-10T03:28:25Z

camel/models/azure_openai_model.py

+        self.deployment_name = os.environ.get('AZURE_MODEL_DEPLOYMENT_NAME',
+                                              'gpt-3.5-turbo-1106')


Do we need self.deployment_name? Could we just use self.model_type.value?

This is explained in the comment below. I will try to find a better way to do it.

lightaime · 2023-12-10T03:35:04Z

camel/models/model_factory.py

+            use_azure_api = 'USE_AZURE_API' in os.environ
+            if use_azure_api:
+                model_class = AzureOpenAIModel
+            else:
+                model_class = OpenAIModel


We soemtime may want to use Azure openai models for some agents while openAI models for other agents. Using environment variable for the ModelFactory may not be a very good idea.

I will try to find a better way to do it in the next commit.

Wendong-Fan · 2023-12-10T09:21:11Z

Thanks @L4zyy for the PR! Overall looks good. I have some questions

About the AzureOpenAIModel, do we need a seperate class for azure openai models? It seems the only differences between AzureOpenAIModel and OpenAIModel will be the creation of self._client. Should we just support azure models with OpenAIModel?

Should we just introduce enums in https://github.com/camel-ai/camel/blob/master/camel/types/enums.py to support the client creation? We can introduce somethings like: ModelType.Azure_GPT_3_5_TURBO and so on.

@dandansamax @Wendong-Fan any suggesitions?

Hi @lightaime @L4zyy,
I would suggest a separate class for azure openai for clear distinction and flexibility of future changes since azure may have different configurations or have unique features with openai (maybe solver feature update compared with openai);
Using enums is a great approach, I agree with moving azure_model_name_to_type to enums.py

L4zyy · 2023-12-10T09:28:57Z

Thanks @L4zyy for the PR! Overall looks good. I have some questions

About the AzureOpenAIModel, do we need a seperate class for azure openai models? It seems the only differences between AzureOpenAIModel and OpenAIModel will be the creation of self._client. Should we just support azure models with OpenAIModel?

Should we just introduce enums in https://github.com/camel-ai/camel/blob/master/camel/types/enums.py to support the client creation? We can introduce somethings like: ModelType.Azure_GPT_3_5_TURBO and so on.

@dandansamax @Wendong-Fan any suggesitions?

These are my initial ideas for integrating Azure OpenAI API. However, the API allows users to deploy models like GPT-3/GPT-4 with different deployment names. Therefore, when calling the self._client.chat.completions.create function, we should use the deployment name instead of the model name. This is why I want to use a new class for Azure OpenAI API.

In this MR, there is some unused code left over from when I was trying other ways to integrate the API. I apologize for that and will remove it in the next commit.

After considering your review, I think it would still be better to implement this integration in the OpenAIModel. I will try to integrate it in the OpenAIModel and commit a new implementation later.

I still have a question about the tests. Is it okay to leave some tests failed? If so, which tests should I pass before I can call for a review and merge to the master?

Thank you very much for the review.😊

L4zyy · 2023-12-10T09:37:18Z

Thanks @L4zyy for the PR! Overall looks good. I have some questions

About the AzureOpenAIModel, do we need a seperate class for azure openai models? It seems the only differences between AzureOpenAIModel and OpenAIModel will be the creation of self._client. Should we just support azure models with OpenAIModel?

Should we just introduce enums in https://github.com/camel-ai/camel/blob/master/camel/types/enums.py to support the client creation? We can introduce somethings like: ModelType.Azure_GPT_3_5_TURBO and so on.

@dandansamax @Wendong-Fan any suggesitions?

Hi @lightaime @L4zyy, I would suggest a separate class for azure openai for clear distinction and flexibility of future changes since azure may have different configurations or have unique features with openai (maybe solver feature update compared with openai); Using enums is a great approach, I agree with moving azure_model_name_to_type to enums.py

Thank you very much for your suggestions. I will consider whether it is better to integrate with a new class. Since the deployment name is user-specific, I think it may not be feasible to move it to ‘enums.py’ unless we ask the user to deploy the model with the same name as OpenAI? Instead, I will try to move this to the 'model_config_dict' instead of using an environment variable and see if it’s a good idea.

dandansamax · 2023-12-11T12:42:24Z

I still have a question about the tests. Is it okay to leave some tests failed? If so, which tests should I pass before I can call for a review and merge to the master?

No problem. The tests failed because doing MR from your fork repo cannot use Github secrets in the main repo. Just leave the CI tests failing if you has passed tests in the local env.

dandansamax · 2023-12-11T12:55:10Z

Thank you very much for your suggestions. I will consider whether it is better to integrate with a new class. Since the deployment name is user-specific, I think it may not be feasible to move it to ‘enums.py’ unless we ask the user to deploy the model with the same name as OpenAI? Instead, I will try to move this to the 'model_config_dict' instead of using an environment variable and see if it’s a good idea.

Hi @L4zyy, thank you for your contribution. Acutually, I don't think putting it in the model_config_dict is a good idea. model_config_dict stores some bottom-end parameters of model and should be comptiable for all GPT models. How about add a optional arguments in OpenAIModel and ModelFactory? so that it can be easily changed by the user.

L4zyy · 2023-12-11T19:40:35Z

Thank you very much for your suggestions. I will consider whether it is better to integrate with a new class. Since the deployment name is user-specific, I think it may not be feasible to move it to ‘enums.py’ unless we ask the user to deploy the model with the same name as OpenAI? Instead, I will try to move this to the 'model_config_dict' instead of using an environment variable and see if it’s a good idea.

Hi @L4zyy, thank you for your contribution. Acutually, I don't think putting it in the model_config_dict is a good idea. model_config_dict stores some bottom-end parameters of model and should be comptiable for all GPT models. How about add a optional arguments in OpenAIModel and ModelFactory? so that it can be easily changed by the user.

Thank you for the advice. I don’t think it’s worth adding a parameter in ‘ModelFactory’ and ‘ChatAgent’ only for Azure API. I have three plans to re-implement this integration:

Add several ‘AZURE_{OPENAI_MODEL_TYPE}’ model types and ‘is_azure’ function to enums.py. Then, change the Azure model type to the corresponding OpenAI model type at ‘ModelFactory’ and create an OpenAIModel.
Since the Azure API is just OpenAI API with a different deployment name, I think we can still add the ‘use_azure_openai_api=True’ and ‘deployment_name’ in ‘model_config_dict’. Then, pop these parameters out in ‘OpenAIModel._init_’ and pass the rest of the parameters to ‘self.model_config_dict’ variable.
We can add only one ‘AZURE’ model type in ‘enums.py’ and add a new AzureOpenAIModel class. We can decide how we parse the model_config_dict in its '_init_' function.

I prefer the third method since it is compatible with future differences between Azure API and OpenAI API. Meanwhile, it leaves the original OpenAIModel class unchanged. The first method will introduce extra variables in OpenAIModel and add an if statement in the run method, which makes it unclean. While the second method will have the same defect and introduce some redundant enums in ‘enums.py’ (Cause we need to add Azure API for every supported GPT model version.)

I have implemented the third method and updated the MR. I have run through nearly all tests I can run locally, and these are the tests that cannot pass locally:

test/functions/test_search_functions.py::test_google_api, which needs 'GOOGLE_API_KEY'
test/embeddings/test_openai_embedding.py::test_openai_embedding, which I don't know why it failed since I haven't changed any related code.
test/agents/test_chat_agent.py::test_chat_agent_stream_output, failed at AssertionError: assert 0 > 0, which I don't know why it failed.

Which one do you think is more appropriate for this integration? @lightaime @Wendong-Fan @dandansamax

dandansamax · 2023-12-12T08:09:26Z

Thank you for the advice. I don’t think it’s worth adding a parameter in ‘ModelFactory’ and ‘ChatAgent’ only for Azure API.

Thank you for your feedback. However, I think changing model name is a common operation. For exmaple, if I want to use OpenAI legacy APIs, it's very diffcult in the current implementation. But if we can pass model name to OpenAIModel, it's much easier for user to do it.

Also this feature can be widely used in the open source models.

I still don't think modifying model_config_dict is a good idea. This variable should keep constant to ensure that camel would pass exactly same model parameters to the backend API as the user set. If you really want to put all model configs in one dict, how about refactor the model_config_dict part by adding a "general backend config" containing model_config_dict with other backend parameters? What do you think?

I have implemented the third method and updated the MR. I have run through nearly all tests I can run locally, and these are the tests that cannot pass locally:

Ensure your newly created tests can be passed and it will be fine.

L4zyy · 2023-12-12T09:07:31Z

Thank you for your feedback. However, I think changing model name is a common operation. For exmaple, if I want to use OpenAI legacy APIs, it's very diffcult in the current implementation. But if we can pass model name to OpenAIModel, it's much easier for user to do it.

Also this feature can be widely used in the open source models.

I agree with you that adding another parameter to pass extra information would make the API integration much easier and flexible. However, I am not familiar with the implementation of the open-source model class, and I don’t want to introduce too much variation for this single API integration. If it’s okay to add a new parameter for this integration, I will re-implement it later.

I still don't think modifying model_config_dict is a good idea. This variable should keep constant to ensure that camel would pass exactly same model parameters to the backend API as the user set. If you really want to put all model configs in one dict, how about refactor the model_config_dict part by adding a "general backend config" containing model_config_dict with other backend parameters? What do you think?

I see. I will pass the extra information in the new parameter and see if it's better.

Ensure your newly created tests can be passed and it will be fine.

Okay, thanks for the clarification. 😊

L4zyy · 2023-12-13T19:10:24Z

Hi @dandansamax, I have re-implemented the integration as we discussed. The new implementation allows us to pass extra backend information through the backend_config_dict parameter. However, this version involved changes to a lot more files than the previous one. Specifically, I had to modify files related to the model factory, chat agent, and their tests. I have passed all tests that are not related to function tests (which may require their function keys) and open source models (since I have trouble downloading the weights of vicuna models). Please take a look and let me know your suggestions! 😊

Wendong-Fan · 2024-03-09T03:27:45Z

Hi @L4zyy , thanks for the contribution! I'm going to review this PR but the pipeline still has error, would you like to fix this first? Thank!

L4zyy · 2024-03-09T08:48:05Z

Hi @L4zyy , thanks for the contribution! I'm going to review this PR but the pipeline still has error, would you like to fix this first? Thank!

Certainly, I will update my code and address these errors later.

Wendong-Fan · 2024-04-06T14:08:03Z

close this pr since another new pr opened #499

L4zyy force-pushed the feature/azure-openai-api branch from 91cdf91 to 24e9e0c Compare December 8, 2023 04:21

lightaime assigned L4zyy Dec 8, 2023

lightaime added the Model Related to backend models label Dec 8, 2023

L4zyy force-pushed the feature/azure-openai-api branch 2 times, most recently from 92fbe8e to 871b506 Compare December 9, 2023 09:56

lightaime reviewed Dec 10, 2023

View reviewed changes

L4zyy added 8 commits December 12, 2023 03:38

feat: Integrate Azure OpenAI API.

6b1f041

fix: Fix typo in README and remove argparse in role_playing.

66676d4

fix: Change default model type for AzureOpenAIModel.

1ef94d1

fix: Isort.

a6d324d

fix: Reformat with yapf.

84cb4ee

fix: Reformat with yapf==0.32.0.

80a837f

fix: Reimplement integration.

19e300a

fix: Fix test and examples files for yapf.

a0829c7

L4zyy force-pushed the feature/azure-openai-api branch from 7eaa39d to a0829c7 Compare December 11, 2023 19:38

fix: Clean up unused global variable in azure_openai_model.py.

564db37

feat: Re-implement the Azure API integration.

126d857

Merge branch 'master' into feature/azure-openai-api

1e73e1f

dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jan 14, 2024

Wendong-Fan self-requested a review March 9, 2024 03:27

L4zyy mentioned this pull request Apr 4, 2024

feat: Integrate Azure OpenAI API. (new) #499

Closed

12 tasks

Wendong-Fan closed this Apr 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Integrate Azure OpenAI API. #402

feat: Integrate Azure OpenAI API. #402

L4zyy commented Dec 7, 2023

L4zyy commented Dec 9, 2023 •

edited

Loading

lightaime left a comment

lightaime Dec 10, 2023

L4zyy Dec 10, 2023

lightaime Dec 10, 2023

L4zyy Dec 10, 2023

lightaime Dec 10, 2023

L4zyy Dec 10, 2023

lightaime Dec 10, 2023

L4zyy Dec 10, 2023

Wendong-Fan commented Dec 10, 2023

L4zyy commented Dec 10, 2023

L4zyy commented Dec 10, 2023

dandansamax commented Dec 11, 2023

dandansamax commented Dec 11, 2023

L4zyy commented Dec 11, 2023

dandansamax commented Dec 12, 2023

L4zyy commented Dec 12, 2023

L4zyy commented Dec 13, 2023

Wendong-Fan commented Mar 9, 2024

L4zyy commented Mar 9, 2024

Wendong-Fan commented Apr 6, 2024

		self.deployment_name = os.environ.get('AZURE_MODEL_DEPLOYMENT_NAME',
		'gpt-3.5-turbo-1106')

feat: Integrate Azure OpenAI API. #402

feat: Integrate Azure OpenAI API. #402

Conversation

L4zyy commented Dec 7, 2023

Description

Motivation and Context

Types of changes

Implemented Tasks

Checklist

L4zyy commented Dec 9, 2023 • edited Loading

lightaime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wendong-Fan commented Dec 10, 2023

L4zyy commented Dec 10, 2023

L4zyy commented Dec 10, 2023

dandansamax commented Dec 11, 2023

dandansamax commented Dec 11, 2023

L4zyy commented Dec 11, 2023

dandansamax commented Dec 12, 2023

L4zyy commented Dec 12, 2023

L4zyy commented Dec 13, 2023

Wendong-Fan commented Mar 9, 2024

L4zyy commented Mar 9, 2024

Wendong-Fan commented Apr 6, 2024

L4zyy commented Dec 9, 2023 •

edited

Loading