-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
azure-openai[patch]: fix azureOpenAIApiKey not working #4964
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -260,23 +260,22 @@ export class AzureChatOpenAI | |||
fields?.azureOpenAIApiDeploymentName) ?? |
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 team, just a heads up that I've flagged a change in the PR that explicitly accesses an environment variable using getEnvironmentVariable
. It's important for maintainers to review this to ensure it aligns with our environment variable handling practices.
@@ -57,23 +57,22 @@ export class AzureOpenAIEmbeddings | |||
fields?.azureOpenAIEndpoint ?? |
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 team, I've flagged a change in the PR for maintainers to review. The code explicitly accesses and reads environment variables via getEnvironmentVariable
, so it's important to ensure that this change aligns with our environment variable handling practices. Let me know if you need any further assistance with this.
@@ -127,23 +127,22 @@ export class AzureOpenAI< | |||
fields?.azureOpenAIApiDeploymentName ?? |
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 team, I've flagged this PR for your review to ensure that the changes related to accessing environment variables via getEnvironmentVariable
are thoroughly checked. Please take a look and let me know if you need any further assistance.
@@ -793,6 +793,20 @@ test("Test ChatOpenAI token usage reporting for streaming calls", async () => { | |||
} |
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 there! 👋 I've reviewed the code and noticed that the added test "Test Azure ChatOpenAI with key credentials" explicitly accesses environment variables using the getEnvironmentVariable
function. I've flagged this for your review to ensure proper handling of environment variables. Keep up the great work!
@@ -70,6 +70,20 @@ test("Test OpenAIEmbeddings.embedQuery with TokenCredentials", async () => { | |||
expect(typeof res[0]).toBe("number"); |
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 there! 👋 I noticed that the added test case in this PR explicitly accesses environment variables using the getEnvironmentVariable
function. I've flagged this for your review to ensure proper handling of environment variables. Keep up the great work! 🚀
@@ -327,6 +327,20 @@ test("Test OpenAI with Token credentials ", async () => { | |||
console.log({ res }); |
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 there! I noticed that the recent change in the code explicitly accesses environment variables using the getEnvironmentVariable
function. I've flagged this for your review to ensure that the handling of environment variables aligns with best practices. Keep up the great work!
Thank you! |
Fixes #4955