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

core: bind_tools interface on basechatmodel #20360

Merged
merged 6 commits into from
Apr 12, 2024

Conversation

efriis
Copy link
Member

@efriis efriis commented Apr 12, 2024

No description provided.

Copy link

vercel bot commented Apr 12, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Apr 12, 2024 1:29am

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. 🤖:improvement Medium size change to existing code to handle new use-cases labels Apr 12, 2024
Comment on lines 912 to 915
*,
tool_choice: Optional[
Union[Dict, str, Literal["auto", "any", "none"], bool]
] = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

also game to leave this explicit kwarg off. But feels like we'll start recommending it for models that support it and wouldn't want re-use in another way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

don't think tool_choice should be on base implementation, anthropic doesn't support

Copy link
Member Author

Choose a reason for hiding this comment

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

But it won't fail them because it's an optional kwarg.

What it will fail is someone defining bind_tools(..., tool_choice: int)

Thoughts on keeping?

Copy link
Collaborator

Choose a reason for hiding this comment

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

imo weird to have it as part of signature on implementations that don't support it. also feels like a still pretty beta feature for most providers. also seems easier to add later if things change than to try to remove if things change. but don't feel super strongly here

Copy link
Member Author

Choose a reason for hiding this comment

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

removed. Can add later easily.

@efriis efriis enabled auto-merge (squash) April 12, 2024 01:29
@efriis efriis merged commit 2928237 into master Apr 12, 2024
126 checks passed
@efriis efriis deleted the erick/core-bind-tools-interface-on-basechatmodel branch April 12, 2024 01:32
junkeon pushed a commit to UpstageAI/langchain that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants