-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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]: Update bedrock to accept openai formatted tools #5852
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -35,7 +35,7 @@ | |||
"author": "LangChain", |
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 update to the "@langchain/core" dependency from "~0.2.6" to "~0.2.9" in the package.json. This seems to be a change in the hard dependency, and I'm flagging this for your review. Keep up the great work!
@@ -383,3 +385,103 @@ test.skip.each([ |
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 review because the added code explicitly accesses environment variables using process.env
. Please review this change to ensure proper handling of environment variables.
@@ -12,9 +12,11 @@ class BedrockChatStandardUnitTests extends ChatModelUnitTests< | |||
constructor() { |
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 that explicitly add environment variables via process.env
. I've flagged this for your review to ensure it aligns with our best practices. Keep up the great work!
@@ -11,8 +11,8 @@ class ChatTogetherAIStandardUnitTests extends ChatModelUnitTests< | |||
constructor() { |
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 this PR for your review as it includes a change that explicitly accesses and sets an environment variable using process.env
. Please take a look and ensure it aligns with our best practices. Thanks!
@@ -383,3 +385,103 @@ test.skip.each([ | |||
|
|||
expect(res.content.length).toBeGreaterThan(1); | |||
}); | |||
|
|||
test.skip("withStructuredOutput", 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.
@bracesproul Do you plan to add withStructuredOutput
as well, since you added this skipped tests? I was just poking around trying to understand why it's not currently supported, given that tool calling in general appears to work well with Claude on Bedrock.
No description provided.