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

azure-openai[minor]: update docs and fix openai api usage #4898

Merged
merged 15 commits into from
Mar 30, 2024

Conversation

sinedied
Copy link
Contributor

Going through the docs I noticed the Azure OpenAI docs were not up to date with the latest version I had locally, a rebase may have gone wrong with the initial PR that created the package. So here's the updated docs.

I also noticed using an OpenAI API key was not working, so I fixed that too and added some tests for it.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Mar 27, 2024
Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 30, 2024 9:27am
langchainjs-docs ✅ Ready (Inspect) Visit Preview Mar 30, 2024 9:27am

@dosubot dosubot bot added the auto:documentation Changes to documentation and examples, like .md, .rst, .ipynb files. Changes to the docs/ folder label Mar 27, 2024
@@ -266,15 +266,27 @@ export class AzureChatOpenAI
(getEnvironmentVariable("AZURE_OPENAI_API_KEY") ||
Copy link

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 an environment variable using getEnvironmentVariable("AZURE_OPENAI_API_KEY"). I've flagged this for your review to ensure it aligns with the intended functionality. Keep up the great work!

@@ -64,16 +63,28 @@ export class AzureOpenAIEmbeddings
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! 👋 I've flagged a change in the PR for maintainers to review. The added code explicitly accesses an environment variable using the getEnvironmentVariable function, so it's important to ensure that this change is handled correctly. Keep up the great work! 🚀

@@ -133,15 +133,27 @@ export class AzureOpenAI<
(getEnvironmentVariable("AZURE_OPENAI_API_KEY") ||
Copy link

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 that explicitly requires an environment variable "AZURE_OPENAI_API_KEY" using the getEnvironmentVariable function. This is to ensure the maintainers review and validate the handling of environment variables in the code.

@@ -16,6 +16,8 @@ import {
import { CallbackManager } from "@langchain/core/callbacks/manager";
Copy link

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 added an import for getEnvironmentVariable from "@langchain/core/utils/env", which suggests the code is accessing environment variables. I've flagged this for your review. Keep up the great work!

@@ -68,3 +69,16 @@ test("Test OpenAIEmbeddings.embedQuery with TokenCredentials", async () => {
const res = await embeddings.embedQuery("Hello world");
Copy link

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 explicitly accesses an environment variable using getEnvironmentVariable. I've flagged this for your review to ensure it aligns with the project's requirements. Let me know if you need further assistance!

@@ -325,3 +326,18 @@ test("Test OpenAI with Token credentials ", async () => {
const res = await model.invoke("Print hello world");
Copy link

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 the recent change in the PR for review. It looks like the code is explicitly accessing and using an environment variable, so it's important to ensure everything is handled correctly. Keep up the great work!

docs: add Azure OpenAI SDK documentation

docs: update docs and example for new package name

docs: update readme

docs: add Azure LLM integrations

docs: update doc

docs: add managed identity usage

fix: add missing model name in example

docs: add Azure OpenAI embeddings docs
BREAKING CHANGE: stripNewLines option is now disabled by default
@sinedied
Copy link
Contributor Author

I'm not sure why the docs deployment are failing, is there a command I can run locally to see what's wrong?

@jacoblee93
Copy link
Collaborator

Thank you! Build failure looks like a flake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:documentation Changes to documentation and examples, like .md, .rst, .ipynb files. Changes to the docs/ folder lgtm PRs that are ready to be merged as-is size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants