-
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
community[minor]: feat: add chat models for zhipuai #4644
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -0,0 +1,444 @@ | |||
import { BaseChatModel, type BaseChatModelParams } from "@langchain/core/language_models/chat_models"; |
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 wanted to flag that this PR adds a new external HTTP request using the fetch
function in the ChatZhipuAI
class. This change should be reviewed by maintainers to ensure it aligns with our code standards and requirements.
@@ -0,0 +1,444 @@ | |||
import { BaseChatModel, type BaseChatModelParams } from "@langchain/core/language_models/chat_models"; |
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 for review. The added code explicitly accesses and reads an environment variable using the getEnvironmentVariable
function, so it's important to ensure that this is handled correctly. Let me know if you have any questions!
langchain/package.json
Outdated
@@ -1335,6 +1335,7 @@ | |||
"ignore": "^5.2.0", | |||
"ioredis": "^5.3.2", | |||
"jsdom": "*", | |||
"jsonwebtoken": "^9.0.2", |
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.
I think this is a mistake? Can you remove?
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.
I think this is a mistake? Can you remove?
@jacoblee93 Thanks, i move it to peer dependency
import { type CallbackManagerForLLMRun } from "@langchain/core/callbacks/manager"; | ||
import { getEnvironmentVariable } from "@langchain/core/utils/env"; | ||
// eslint-disable-next-line import/no-extraneous-dependencies | ||
import * as jwt from 'jsonwebtoken'; |
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.
Can we add this as an optional peer dependency instead?
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.
Can we add this as an optional peer dependency instead?
yes, thanks for checking, please help to check this again @jacoblee93
/** | ||
* Interface defining the input to the ZhipuAIChatInput class. | ||
*/ | ||
interface ZhipuAIChatInput { |
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.
Would be good to export this type - also maybe call ChatZhipuAIParams
?
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.
Would be good to export this type - also maybe call
ChatZhipuAIParams
?
yes, thank you, i renamed and also export the interface @jacoblee93
} | ||
|
||
this.apiUrl = "https://open.bigmodel.cn/api/paas/v4/chat/completions"; | ||
this.lc_serializable = true; |
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.
You don't need to set this here
Thank you! |
Fixes # (issue)