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

cohere[minor]: Add support for tool calling cohere #5810

Conversation

Anirudh31415926535
Copy link
Contributor

@Anirudh31415926535 Anirudh31415926535 commented Jun 19, 2024

This PR supports tool calling in js for cohere models.

High level feature introductions:

  1. Parse chat history parameter in request to now include tool messages
  2. Handle parsing logic separately as required for multi hop tool calling and single step tool calling
  3. Add tests to ensure tool calling works as expected
  4. Bump default model version to 'command-r-plus'
  5. Relevant doc changes to remove hardcoded 'model=command' and to showcase tool calling support

Copy link

vercel bot commented Jun 19, 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 Jul 9, 2024 7:14pm
langchainjs-docs ✅ Ready (Inspect) Visit Preview Jul 9, 2024 7:14pm

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. auto:enhancement A large net-new component, integration, or chain. Use sparingly. The largest features labels Jun 19, 2024
@@ -0,0 +1,79 @@
import { ChatCohere } from "@langchain/cohere";
Copy link

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 highlight that the recent code change explicitly accesses an environment variable using process.env. This is an important point for maintainers to review. Keep up the great work! 🚀

@@ -36,7 +36,8 @@
"license": "MIT",
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 a new dependency "zod" has been added to the "dependencies" section, which is a change in the project's dependencies. This comment is to flag the change for maintainers to review. Great work!

package.json Outdated
@@ -69,5 +69,9 @@
"eslint --cache --fix"
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 this PR adds new dependencies ("jest-cli" and "zod") to the project's "dependencies" section in package.json. This is flagged for your review to ensure the impact of these changes, whether they are peer, dev, or hard dependencies. Keep up the great work! 🚀

@@ -9,7 +9,7 @@
"ES2022.Object",
"DOM"
],
"module": "ES2020",
"module": "NodeNext",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you revert these? Should not be in this PR.

@@ -25,7 +25,7 @@
"test": "NODE_OPTIONS=--experimental-vm-modules jest --testPathIgnorePatterns=\\.int\\.test.ts --testTimeout 30000 --maxWorkers=50%",
"test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch --testPathIgnorePatterns=\\.int\\.test.ts",
"test:single": "NODE_OPTIONS=--experimental-vm-modules yarn run jest --config jest.config.cjs --testTimeout 100000",
"test:int": "NODE_OPTIONS=--experimental-vm-modules jest --testPathPattern=\\.int\\.test.ts --testTimeout 100000 --maxWorkers=50%",
"test:int": "NODE_OPTIONS=--experimental-vm-modules jest --testPathPattern=\\chat_models.int\\.test.ts --testTimeout 100000 --maxWorkers=50%",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert

package.json Outdated
@@ -69,5 +69,9 @@
"eslint --cache --fix"
],
"*.md": "prettier --config .prettierrc --write"
},
"dependencies": {
"jest-cli": "^29.7.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert

@@ -135,7 +196,7 @@ export class ChatCohere<

client: CohereClient;

model = "command";
model = "command-r-plus";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is technically breaking

Choose a reason for hiding this comment

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

if we're going to break here, can we make model a required param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @billytrend-cohere We're making the model a required param for Embedding, and Reranker. Imo it is better to leave model as a non-required param for chat, since the implication of an unforeseen model bump isn't huge compared to embedding/rerank.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would still prefer this to be required (look at what happened with gpt-4o) but if you feel strongly I'm fine with keeping it.

@jacoblee93
Copy link
Collaborator

Thanks for this!

Could you try scoping the changes down a bit? Appears to be a lot of unrelated diffs.

Also, don't forget to run yarn format and yarn lint!

@@ -445,3 +762,5 @@ export class ChatCohere<
interface CohereLLMOutput {
estimatedTokenUsage?: TokenUsage;
}

/* eslint-enable @typescript-eslint/no-explicit-any */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessary

return req;
}

getCurrChatTurnMessages(messages: BaseMessage[]): BaseMessage[] {
Copy link
Collaborator

@jacoblee93 jacoblee93 Jul 2, 2024

Choose a reason for hiding this comment

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

See above, you might also consider adding this as a function outside of the class:

https://github.com/langchain-ai/langchainjs/blob/main/libs/langchain-anthropic/src/chat_models.ts#L105

But these helper methods definitely shouldn't be public

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should also add a override bindTools() method, it'll make this support other methods for free:

https://github.com/langchain-ai/langchainjs/blob/main/libs/langchain-anthropic/src/chat_models.ts#L589

// eslint-disable-next-line no-instanceof/no-instanceof
message instanceof HumanMessage ||
// eslint-disable-next-line no-instanceof/no-instanceof
message instanceof SystemMessage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use e.g. message._getType() === "human" instead.

@Anirudh31415926535
Copy link
Contributor Author

@jacoblee93 Hey, thanks so much for the review and comments! I've addressed them! Could you please help to take a look again if possible and then we can try to see if we can get this PR and the other one released by this week? Thanks so much! 💯

@jacoblee93
Copy link
Collaborator

Yes will look today, thanks for the hard work!

@jacoblee93 jacoblee93 changed the title feat: Add support for tool calling cohere cohere[minor]: Add support for tool calling cohere Jul 9, 2024
@jacoblee93
Copy link
Collaborator

Pushed some refactoring/lint/format stuff, will merge alongside the embedding PR and minor bump the package.

@jacoblee93
Copy link
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:enhancement A large net-new component, integration, or chain. Use sparingly. The largest features lgtm PRs that are ready to be merged as-is size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants