New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Integrate Azure OpenAI API. (new) #499
base: master
Are you sure you want to change the base?
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
35a974b
to
e9bcc7a
Compare
e9bcc7a
to
f99e584
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution and sorry for the late review, left some comments as below
export AZURE_MODEL_TYPE=<insert the ModelType enum name that match your Azure OpenAI API model type> | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems for Azure, AZURE_MODEL_TYPE
is not a must, but AZURE_ API_VERSION
is required, could you pls check this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the model type is used by token counter. I still think we should keep this variable for two reason: Firstly, Azure OpenAI API is basically an OpenAI API, I want other future function needs the model type developed on OpenAI Model class can be used directly by this class. Secondly, the deployed model type is a very important attribute for Azure OpenAI API, therefore I want it to be specified when we create this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @L4zyy , I got your point, ModelType
is required for counting token. We can pass ModelType
as parameter rather than using export to set it into environment, just like how we did for OpenAI service. But AZURE_ API_VERSION
is indeed required when we call Azure service, here in the README if you don't ask user set the AZURE_ API_VERSION
then it will always be the default value you set in your code 2023-10-01-preview
, it shouldn't be the case
camel/models/azure_openai_model.py
Outdated
backend_config_dict (Dict[str, Any]): A dictionary that contains | ||
the backend configs.(default : {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are going to use this parameter pass all args including deployment name, endpoint, api version, I would suggest make the docstring more clear here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated this in the new commit.
self.model_type = backend_config_dict.get( | ||
"model_type", os.environ.get("AZURE_MODEL_TYPE", None)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, model type is not necessary for azure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explained my concerns above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about let user set ModelType
as argument rather than set in environment?
camel/models/azure_openai_model.py
Outdated
assert model_type is not None, "Azure model type is not provided." | ||
assert (self.deployment_name | ||
is not None), "Azure model deployment name is not provided." | ||
assert (self.azure_endpoint | ||
is not None), "Azure endpoint is not provided." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For expected error conditions, explicit error handling using try-except blocks is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated this in new commit.
camel/models/azure_openai_model.py
Outdated
def check_model_config(self): | ||
r"""Check whether the model configuration contains any | ||
unexpected arguments to OpenAI API. | ||
|
||
Raises: | ||
ValueError: If the model configuration dictionary contains any | ||
unexpected arguments to OpenAI API. | ||
""" | ||
for param in self.model_config_dict: | ||
if param not in OPENAI_API_PARAMS_WITH_FUNCTIONS: | ||
raise ValueError(f"Unexpected argument `{param}` is " | ||
"input into OpenAI model backend.") | ||
|
||
@property | ||
def stream(self) -> bool: | ||
r"""Returns whether the model is in stream mode, | ||
which sends partial results each time. | ||
Returns: | ||
bool: Whether the model is in stream mode. | ||
""" | ||
return self.model_config_dict.get("stream", False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also need to check the back-end config as we checked the model config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated this in new commit.
def azure_openai_api_key_required(func: F) -> F: | ||
r"""Decorator that checks if the Azure OpenAI API key is available in the | ||
environment variables. | ||
|
||
Args: | ||
func (callable): The function to be wrapped. | ||
|
||
Returns: | ||
callable: The decorated function. | ||
|
||
Raises: | ||
ValueError: If the Azure OpenAI API key is not found in the environment | ||
variables. | ||
""" | ||
|
||
@wraps(func) | ||
def wrapper(self, *args, **kwargs): | ||
if 'AZURE_OPENAI_API_KEY' in os.environ: | ||
return func(self, *args, **kwargs) | ||
else: | ||
raise ValueError('Azure OpenAI API key not found.') | ||
|
||
return cast(F, wrapper) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for azure openai, I think not only api is required, the endpoint and api version also need to be checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think we need to check these variables here since they will be checked when initializing the AzureOpenAIModel instance. When running the commands, we should only check the AZURE_OPENAI_API_KEY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AZURE_OPENAI_API_KEY
, AZURE_ENDPOINT
, AZURE_DEPLOYMENT_NAME
, AZURE_ API_VERSION
are all required by Azure, if we add AZURE_OPENAI_API_KEY
as decorator checking item, why not follow the same logic add others? Can we move the checking in initializing to this decorator? If not why we not also also check AZURE_OPENAI_API_KEY
when initializing the AzureOpenAIModel instance? WDYT?
Dear @camel-ai/camel-maintainers |
Apologies for the delay in updating. I've addressed all your valuable feedback, responded accordingly, and pushed a new commit. If the new commit looks good, I will merge this branch to the HEAD of master branch ASAP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @L4zyy and sorry for the late reply~
export AZURE_MODEL_TYPE=<insert the ModelType enum name that match your Azure OpenAI API model type> | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @L4zyy , I got your point, ModelType
is required for counting token. We can pass ModelType
as parameter rather than using export to set it into environment, just like how we did for OpenAI service. But AZURE_ API_VERSION
is indeed required when we call Azure service, here in the README if you don't ask user set the AZURE_ API_VERSION
then it will always be the default value you set in your code 2023-10-01-preview
, it shouldn't be the case
self.model_type = backend_config_dict.get( | ||
"model_type", os.environ.get("AZURE_MODEL_TYPE", None)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about let user set ModelType
as argument rather than set in environment?
def azure_openai_api_key_required(func: F) -> F: | ||
r"""Decorator that checks if the Azure OpenAI API key is available in the | ||
environment variables. | ||
|
||
Args: | ||
func (callable): The function to be wrapped. | ||
|
||
Returns: | ||
callable: The decorated function. | ||
|
||
Raises: | ||
ValueError: If the Azure OpenAI API key is not found in the environment | ||
variables. | ||
""" | ||
|
||
@wraps(func) | ||
def wrapper(self, *args, **kwargs): | ||
if 'AZURE_OPENAI_API_KEY' in os.environ: | ||
return func(self, *args, **kwargs) | ||
else: | ||
raise ValueError('Azure OpenAI API key not found.') | ||
|
||
return cast(F, wrapper) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AZURE_OPENAI_API_KEY
, AZURE_ENDPOINT
, AZURE_DEPLOYMENT_NAME
, AZURE_ API_VERSION
are all required by Azure, if we add AZURE_OPENAI_API_KEY
as decorator checking item, why not follow the same logic add others? Can we move the checking in initializing to this decorator? If not why we not also also check AZURE_OPENAI_API_KEY
when initializing the AzureOpenAIModel instance? WDYT?
Description
This PR integrate the Azure OpenAI API to CAMEL.
(older PR: #402)
Motivation and Context
Integrate Azure OpenAI API, close #231
Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Implemented Tasks
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!