-
Notifications
You must be signed in to change notification settings - Fork 50.4k
feat: update itens to use variable like parameters #878
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
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for contributing @felipementel! We will review the pull request and get back to you soon. |
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.
Pull Request Overview
This pull request improves configuration management by replacing hardcoded deployment and model names with environment variables in two Jupyter notebooks. This change enhances flexibility and follows best practices for configurable deployments.
- Replaced hardcoded "gpt-35-turbo" deployment name with environment variable in chat application notebook
- Replaced hardcoded "text-embedding-ada-002" model name with environment variable in search application notebook
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
07-building-chat-applications/python/aoai-assigment-simple.ipynb | Updates deployment name to use AZURE_OPENAI_DEPLOYMENT environment variable |
08-building-search-applications/python/aoai-assignment.ipynb | Updates model name to use AZURE_OPENAI_EMBEDDINGS_DEPLOYMENT environment variable |
Comments suppressed due to low confidence (1)
08-building-search-applications/python/aoai-assignment.ipynb:1
- If the environment variable AZURE_OPENAI_EMBEDDINGS_DEPLOYMENT is not set,
os.getenv()
will returnNone
, which will cause the subsequentclient.embeddings.create()
call to fail. Consider providing a default value or adding validation to ensure the model is not None.
{
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -20,7 +20,7 @@ | |||
"assert RESOURCE_ENDPOINT, \"ERROR: Azure OpenAI Endpoint is missing\"\n", | |||
"assert \"openai.azure.com\" in RESOURCE_ENDPOINT.lower(), \"ERROR: Azure OpenAI Endpoint should be in the form: \\n\\n\\t<your unique endpoint identifier>.openai.azure.com\"\n", | |||
"openai.api_base = RESOURCE_ENDPOINT\n", | |||
"deployment = \"gpt-35-turbo\" # replace with your deployment name" | |||
"deployment = os.environ['AZURE_OPENAI_DEPLOYMENT']" |
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.
Using os.environ['AZURE_OPENAI_DEPLOYMENT']
will raise a KeyError if the environment variable is not set. Consider using os.getenv('AZURE_OPENAI_DEPLOYMENT')
with a default value or proper error handling to provide a better user experience.
"deployment = os.environ['AZURE_OPENAI_DEPLOYMENT']" | |
"deployment = os.getenv('AZURE_OPENAI_DEPLOYMENT', '').strip()\n", | |
"assert deployment, \"ERROR: Azure OpenAI Deployment name is missing\"" |
Copilot uses AI. Check for mistakes.
This pull request updates environment variable usage in two Jupyter notebooks to improve configurability and align with best practices. Specifically, it replaces hardcoded deployment names with environment variables.
Updates to environment variable usage:
07-building-chat-applications/python/aoai-assigment-simple.ipynb
, replaced the hardcoded deployment name"gpt-35-turbo"
with the environment variableos.environ['AZURE_OPENAI_DEPLOYMENT']
for better flexibility.08-building-search-applications/python/aoai-assignment.ipynb
, replaced the hardcoded model name'text-embedding-ada-002'
with the environment variableos.getenv("AZURE_OPENAI_EMBEDDINGS_DEPLOYMENT")
to support dynamic configuration.