-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
partners[patch]: Handle setting background callbacks to false in all relevant int tests #6006
Conversation
…relevant int tests
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -1,10 +1,14 @@ | |||
/* eslint-disable no-process-env */ |
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 an environment variable via process.env
. I've flagged this for your review to ensure it aligns with our environment variable handling practices. Keep up the great work!
@@ -10,6 +12,9 @@ import { | |||
import { getEnvironmentVariable } from "@langchain/core/utils/env"; |
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 chat_models.int.test.ts
file explicitly accesses an environment variable using process.env
. I've flagged this for your review to ensure it aligns with the project's standards. Keep up the great work!
@@ -1,30 +1,45 @@ | |||
/* eslint-disable no-process-env */ |
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! 👋 This is a friendly flag to bring attention to the addition and manipulation of environment variables in the code changes. It's important for maintainers to review these changes to ensure they align with best practices and don't introduce any unexpected behavior. Keep up the great work! 🚀
@@ -1,7 +1,11 @@ | |||
/* eslint-disable no-promise-executor-return */ |
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 this PR includes changes related to accessing environment variables via process.env
. I've flagged this for your review to ensure it aligns with our best practices. Let me know if you need any further assistance with this.
@@ -1,3 +1,5 @@ | |||
/* eslint-disable no-process-env */ |
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 the addition of the comment disabling the eslint rule for no-process-env
in the code. This is just a heads up for the maintainers to review the potential use of process.env
. Keep up the great work!
@@ -10,6 +12,9 @@ import { | |||
import { getEnvironmentVariable } from "@langchain/core/utils/env"; |
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 llms.int.test.ts directly accesses an environment variable using process.env
. I've flagged this for your review to ensure it aligns with our environment variable handling practices. Let me know if you need further assistance with this!
@@ -1,7 +1,12 @@ | |||
/* eslint-disable no-process-env */ |
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 an environment variable via process.env
. I've flagged this for your review to ensure it aligns with our environment variable usage guidelines. Let me know if you need further assistance with this.
@@ -6,6 +8,9 @@ import { NewTokenIndices } from "@langchain/core/callbacks/base"; | |||
import { OpenAIChat } from "../legacy.js"; |
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 this PR includes a change that accesses an environment variable using process.env
. The added line saves the original value of the 'LANGCHAIN_CALLBACKS_BACKGROUND' environment variable. This comment is to flag the change for maintainers to review. Keep up the great work!
Updates all relevant integration tests which use callbacks to first set
LANGCHAIN_CALLBACKS_BACKGROUND
tofalse
and cleanup at the end by resetting to whatever value it was originally.I opted to not update deprecated tests such as those in the
langchain-azure-openai
package.