-
Notifications
You must be signed in to change notification settings - Fork 378
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
Add GPT-4V sample code and images #66
base: main
Are you sure you want to change the base?
Conversation
{ | ||
"GPT-4V_MODEL":"<GPT-4V Model Name>", | ||
"OPENAI_API_BASE":"https://<Your Azure Resource Name>.openai.azure.com", | ||
"OPENAI_API_VERSION":"<OpenAI API Version>", |
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 should document where to find this configuration setting. For example this link has a list of supported versions:
https://learn.microsoft.com/en-us/azure/ai-services/openai/reference
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 should also just put a default API version here based on what we know we'll be using at announce. There's no reason to force someone to go figure this out themselves
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 should document where to find this configuration setting. For example this link has a list of supported versions: https://learn.microsoft.com/en-us/azure/ai-services/openai/reference
We documented in the readme file.
"openai_api_base = config_details['OPENAI_API_BASE']\n", | ||
"\n", | ||
"# The API key for your Azure OpenAI resource.\n", | ||
"openai_api_key = os.getenv(\"OPENAI_API_KEY\")\n", |
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.
Can we just use the config file for all settings? Using the environment variables just adds one more step to setup
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 agree that simplifying the setup process is important. However, prioritizing the security of sensitive data like 'key' is crucial. Keeping it in environment variables rather than in a configuration file offers an additional layer of security.
"### Setup Parameters\n", | ||
"\n", | ||
"\n", | ||
"Here we will load the configurations from _config.json_ file to setup deployment_name, openai_api_base, openai_api_key and openai_api_version." |
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 think we should just have clear instructions here saying "Go fill in the confg.json file in the same directory with your deployment information to enable this sample to get started.
Alternatively we could just skip the JSON and just show the variables you need to set in the code section below. For these kind of light-wieght examples couldn't we just skip having a config file?
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.
To maintain consistency with other sample code, we are using a config file. By the way, I have already created a new notebook containing all shared functions.
Basic_Samples/GPT-4V/video_chatcompletions_example_restapi.ipynb
Outdated
Show resolved
Hide resolved
Basic_Samples/GPT-4V/video_chatcompletions_example_restapi.ipynb
Outdated
Show resolved
Hide resolved
675f8b0
to
55e591d
Compare
Purpose
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
Add sample code and images for GPT-4V.
How to Test
What to Check
Verify that the following are valid
Other Information