Skip to content
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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

zhizhoualan
Copy link

Purpose

  • ...

Does this introduce a breaking change?

[ ] Yes
[x ] No

Pull Request Type

What kind of change does this Pull Request introduce?
Add sample code and images for GPT-4V.

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install
  • Test the code

What to Check

Verify that the following are valid

  • ...

Other Information

{
"GPT-4V_MODEL":"<GPT-4V Model Name>",
"OPENAI_API_BASE":"https://<Your Azure Resource Name>.openai.azure.com",
"OPENAI_API_VERSION":"<OpenAI API Version>",

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

Copy link

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

Copy link
Author

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",
Copy link

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

Copy link
Author

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."
Copy link

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?

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants