-
Notifications
You must be signed in to change notification settings - Fork 19.9k
chore: Support tool runtime injection when custom args schema is prov… #33999
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
Conversation
CodSpeed Performance ReportMerging #33999 will improve performances by 23.04%Comparing
|
| Mode | Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_async_callbacks_in_sync |
24 ms | 19.5 ms | +23.04% |
| ⚡ | WallTime | test_import_time[BaseChatModel] |
527.7 ms | 479.4 ms | +10.07% |
| ⚡ | WallTime | test_import_time[ChatPromptTemplate] |
594 ms | 539.4 ms | +10.12% |
| ⚡ | WallTime | test_import_time[InMemoryVectorStore] |
614.5 ms | 556.4 ms | +10.44% |
| ⚡ | WallTime | test_import_time[tool] |
514 ms | 462.3 ms | +11.17% |
Footnotes
-
21 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
sydney-runkle
left a comment
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.
Have a few questions about impl here, but generally I think this is the right track for a fix (basically, inspect signature, inject anyways).
Happy to pick this up and finish, just wanted to get some clarification first :).
| @functools.cached_property | ||
| def _injected_args_keys(self) -> frozenset[str]: | ||
| fn = self.func or self.coroutine | ||
| if fn is None: | ||
| return _EMPTY_SET | ||
| return frozenset( | ||
| k | ||
| for k, v in signature(fn).parameters.items() | ||
| if _is_injected_arg_type(v.annotation) | ||
| ) | ||
|
|
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.
it's clear to me that this solves the problem bc we're inspecting the signature
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.
Also I think we build these from the tool decorator so I understand it solves our specific use case 👍
## overview
The main purpose of this is to respect tool signatures that request
injected args (like `ToolRuntime`) even when the explicitly specified
`args_schema` does not.
Ex in the following example, we should still inject `runtime` despite
its absence in `ArgsSchema`
```py
class ArgsSchema(BaseModel):
some_arg: int = Field(...)
@tool(args_schema=ArgsSchema)
def my_tool(some_arg: int, runtime: ToolRuntime): ...
```
This is accompanied by
langchain-ai/langchain#34051 which has tests
that pass w/ this change. This tests injection w/ `create_agent` (more
end to end than tests added in
langchain-ai/langchain#33999.
This unblocks the injection of `ToolRuntime` into MCP tools which is
exciting bc that exposes tool call id and state, which we previously
were unable to do.
## other benefits
* Cleaner code structure w/ more helpful docs about injected args.
* Nice perf boost, we're no longer inspecting the annotations of a
tool's schema 3 different times to detect store, state, and runtime
injections.
## additional notes
1. I could see a world where we want more of this logic to reside on the
tools themselves, but tools don't now about LG specific injection types
(like `ToolRuntime`, hence having this logic here for now).
2. We could separately add validation for the case where something is
specified in `args_schema` and not in the function signature (probably
at the tool level though).
Support injection of injected args (like
InjectedToolCallId,ToolRuntime) when anargs_schemais specified that doesn't contain said args.This allows for pydantic validation of other args while retaining the ability to inject langchain specific arguments.
fixes #33646
fixes #31688
Taking a deep dive here reminded me that we definitely need to revisit our internal tooling logic, but I don't think we should do that in this PR.