fix(autogen-ext): skip LangChain callback-manager (run_manager) when inferring tool args schema - #7994
Conversation
…inferring tool args schema LangChainToolAdapter inferred a pydantic args model from the callable's signature when no args_schema was provided. LangChain injects a `run_manager` (CallbackManagerForToolRun / its async variant) into a tool's `_run`/\_arun`, and pydantic cannot generate a schema for that type, so construction raised 'Unable to generate pydantic-core schema for CallbackManagerForToolRun' (e.g. GoogleDriveSearchTool, microsoft#6385). Skip parameters whose annotation resolves to a LangChain callback-manager type (also by the conventional 'run_manager' name as a fallback for unresolvable string annotations). The adapter never passed a callback manager to the underlying tool, so omitting it from the schema is correct. Adds a regression test using a tool without an explicit args_schema. Fixes microsoft#6385
Gnani Rahul Nutakki (gnanirahulnutakki)
left a comment
There was a problem hiding this comment.
Thanks for fixing this adapter failure. I reproduced the original path and found two follow-ups before the change is complete: LangChain also reserves callbacks, which still triggers the same schema-generation crash, and the new test needs presence guards for optional schema keys to pass Pyright. I included minimal fixes and focused validation results inline.
| # fallback for unresolvable (string) annotations, by the | ||
| # conventional parameter name. | ||
| annotation = type_hints.get(k, v.annotation) | ||
| if _is_callback_manager_annotation(annotation) or k == "run_manager": |
There was a problem hiding this comment.
Could we filter callbacks by name here as well? The pinned langchain-core defines FILTERED_ARGS = ("run_manager", "callbacks"), and Callbacks is a sequence of callback handlers, so _is_callback_manager_annotation does not match it. I reproduced this with a no-schema tool whose _run accepts callbacks: Callbacks = None: constructing LangChainToolAdapter raises PydanticSchemaGenerationError for BaseCallbackHandler. Using k in ("run_manager", "callbacks") and extending this regression test to include callbacks fixes the crash; focused pytest, Pyright, mypy, and Ruff all pass.
There was a problem hiding this comment.
Addressed in c1b1fc2: the inferred-schema path now filters both reserved LangChain parameters, run_manager and callbacks, and the regression suite covers the callbacks case. The current head also includes the pinned Ruff formatting fix. Please re-review.
|
|
||
| schema = adapter.schema | ||
| assert schema["name"] == "NoSchema" | ||
| props = schema["parameters"]["properties"] |
There was a problem hiding this comment.
Pyright reports three reportTypedDictNotRequiredAccess errors here because parameters, properties, and required are optional TypedDict keys. The earlier test in this file already uses the safe pattern: assert each key is present before indexing it. Adding assert "parameters" in schema, assert "properties" in schema["parameters"], and assert "required" in schema["parameters"] makes targeted Pyright pass with 0 errors.
There was a problem hiding this comment.
Addressed in c1b1fc2: the test now asserts parameters, properties, and required are present before indexing the optional TypedDict keys. Targeted tests and type checks pass on the current head. Please re-review.
…nal schema keys in test Address review feedback on microsoft#7994: - Filter the reserved 'callbacks' parameter (in addition to 'run_manager') when inferring the args schema. LangChain's FILTERED_ARGS is ('run_manager', 'callbacks'); a 'Callbacks' annotation is not matched by _is_callback_manager_annotation, so a no-schema tool whose _run accepts 'callbacks' raised PydanticSchemaGenerationError. Filter it by name. - Add presence assertions for the optional TypedDict keys ('parameters', 'properties', 'required') in the existing test to satisfy Pyright (reportTypedDictNotRequiredAccess), matching the safe pattern used earlier in the file. - Add a regression test (test_langchain_tool_adapter_skips_callbacks) covering the 'callbacks' case.
Gnani Rahul Nutakki (gnanirahulnutakki)
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c1b1fc24cfb4. Both findings are resolved: callbacks is filtered alongside run_manager, and the tests now guard optional TypedDict keys before indexing. The three focused tests, targeted Pyright, targeted mypy, and Ruff all pass locally.
ErenAta16 (ErenAta16)
left a comment
There was a problem hiding this comment.
Verified the detection logic against the cases it needs to cover, replicating the helper standalone:
bare CallbackManagerForToolRun -> detected
Optional[CallbackManagerForToolRun] -> detected
Union[AsyncCallbackManagerForToolRun, None] -> detected
int -> not detected (control)
Optional[int] -> not detected (control)
inspect.Parameter.empty -> not detected
The Union unwrapping matters because Optional[CallbackManagerForToolRun] is how LangChain's own tool templates declare it, so handling only the bare type would have missed the common case. Covering types.UnionType alongside typing.Union also picks up X | None syntax, which is what a tool written on 3.10+ will use.
The name-based fallback is doing real work rather than being defensive padding, and it's worth spelling out why in case someone later reads it as redundant. This module has from __future__ import annotations, so annotations arrive as strings, and a string annotation can't be resolved by issubclass:
is_cb("Optional[CallbackManagerForToolRun]") -> False
That's precisely the situation the run_manager name check catches. Without it the type check would silently do nothing in exactly the module the fix targets, which would be a nasty way to have a "working" fix that isn't. Worth a brief comment saying the fallback exists because of the postponed-annotations import, since that link isn't obvious from the code alone.
Wrapping the langchain_core.callbacks.manager import in try/except ImportError is also right: this adapter is an optional extra, so the helper shouldn't hard-require the symbols to exist at import time.
Two small things:
The test tool declares run_manager on both _run and _arun. Worth confirming the fix path is exercised for the sync and async signatures separately, since _arun takes AsyncCallbackManagerForToolRun and a fix that only unwrapped the sync type would still pass a test that only checks the adapter constructs.
Since the reported failure is a hard exception at adapter-construction time (Unable to generate pydantic-core schema for ...CallbackManagerForToolRun), it'd be worth asserting the inferred schema doesn't contain run_manager in addition to asserting construction succeeds. Constructing successfully proves the crash is gone; asserting the field is absent proves it was excluded rather than coerced into something the model would then be asked to supply.
…allback - assert the inferred schema explicitly excludes run_manager/callbacks - add a test case covering the async CallbackManagerForToolRun variant so both sync and async callback-manager types are exercised separately - document why the run_manager/callbacks name fallback exists: this module uses from __future__ import annotations, so annotations arrive as strings and get_type_hints can fail to resolve them, leaving string annotations that the issubclass check cannot match Signed-off-by: godququ5-code <godququ5@gmail.com>
| return a + b | ||
|
|
||
| async def _arun( | ||
| self, a: int, b: int, run_manager: Optional[AsyncCallbackManagerForToolRun] = None |
There was a problem hiding this comment.
The repository-pinned formatter still fails on exact head 315e4f04: uv run --frozen ruff format --check packages/autogen-ext/tests/tools/test_langchain_tools.py reports this file would be reformatted and collapses both new _run/_arun signatures onto one line. Please run the pinned formatter before CI. The 4 focused tests, targeted Pyright and mypy, and Ruff lint otherwise pass locally.
There was a problem hiding this comment.
Addressed in 7b5e64d: the test file was formatted with the repository-pinned Ruff version. The current head keeps the callback filtering and regression coverage unchanged.
Reformat the test file with the repo-pinned ruff (0.4.8). Pure formatting change (signature line collapsing); the callback-manager filtering logic and sync/async regression coverage are unchanged. Signed-off-by: godququ5-code <godququ5@gmail.com>
|
Fixed the pinned Ruff formatting issue in test_langchain_tools.py. The callback-manager filtering logic and sync/async regression coverage are unchanged. Targeted tests and type checks pass locally. |
Gnani Rahul Nutakki (gnanirahulnutakki)
left a comment
There was a problem hiding this comment.
Re-verified exact head 7b5e64de. The incremental delta only applies the repository-pinned formatting: ruff format --check and Ruff lint now pass, all 4 focused adapter tests pass, Pyright reports 0 errors, and mypy reports no issues in the adapter and its test. The callback-manager filtering logic and sync/async regression coverage are unchanged from the previously verified head, so the formatter finding is resolved.
|
Formatting fix noted, and good that the detection logic and the sync/async coverage stayed untouched in that pass. My earlier review points still stand as written; nothing in a Ruff-only commit changes them. |
|
Thanks for the follow-up. I checked the current head
The current head also includes the repository-pinned Ruff formatting fix. Could you please re-review the current head and let me know if any point still needs a change? |
|
I rechecked the live PR state: the head is still |
|
Hi Victor Dibia (@victordibia) chetantoshniwal Eric Zhu (@ekzhu) — gentle ping on this one. This PR has been ready for review since July 28 (head It fixes a real adapter crash reported in #6385: The change filters out LangChain's injected callback-manager arguments ( I understand AutoGen is in maintenance mode and that new features aren't the priority — this is a contained bug fix with test coverage, so I hope it still fits. Could one of you take a look when you have a moment? Happy to make any further adjustments, or to close this if you'd prefer the fix land in Microsoft Agent Framework instead. Thanks! |
ErenAta16 (ErenAta16)
left a comment
There was a problem hiding this comment.
Re-reviewed at 7b5e64de as asked. Two commits landed after my last pass, 315e4f04 and 7b5e64de, and the point I raised is addressed in a better form than the one I asked for.
I had asked for a comment explaining that the name-based fallback exists because this module has from __future__ import annotations. What landed instead resolves the annotations first:
try:
type_hints = get_type_hints(callable_)
except Exception:
type_hints = {}so the type check now works on real types in the common case rather than relying on the name check to carry it. The name check stays as the fallback. That ordering is the right way round.
I checked whether the fallback is still load-bearing or has become dead code, since that is the thing that would make the belt-and-braces look redundant to a later reader. It is load-bearing, and the comment's stated reason is accurate:
module-level tool
raw annotation 'Optional[CallbackManagerForToolRun]'
get_type_hints OK -> typing.Optional[...CallbackManagerForToolRun]
locally-defined tool
raw annotation 'Optional[LocalCB]'
get_type_hints NameError: name 'LocalCB' is not defined
A tool class defined inside a function has annotations naming things that are not in module globals, so get_type_hints raises, the except leaves type_hints empty, the annotation stays a string, and issubclass cannot see it. The run_manager name check is the only thing catching that case. So both halves are needed and neither is padding.
The comment that landed is also more useful than what I suggested, because it names LangChain's own constant:
LangChain itself excludes these via
FILTERED_ARGS = ("run_manager", "callbacks")
That is the right thing to anchor to. It tells a future reader that the two names are not a guess about LangChain's conventions but a mirror of a constant upstream, which is what makes the name-based half defensible rather than a heuristic.
One thing worth keeping in mind rather than changing: except Exception around get_type_hints will also swallow a genuine bug in a user's annotations, silently falling back to name matching. That is the correct trade here, since a tool with one unresolvable annotation should not make the whole adapter unusable, but it does mean a mistyped annotation degrades quietly instead of surfacing. Not worth a change in this PR.
From my side this is ready. Victor Dibia (@victordibia) Eric Zhu (@ekzhu) the diff is contained to one helper plus the field loop, callbacks and run_manager are both filtered, and the tests cover _run and _arun in sync and async form.
Summary
Fixes #6385.
LangChainToolAdapterinferred a pydantic args model from the callable's signature when a tool provided noargs_schema. LangChain injects arun_manager(CallbackManagerForToolRun/ its async variant) into a tool's_run/_arun, and pydantic cannot generate a schema for that type, so constructing the adapter raisedUnable to generate pydantic-core schema for ...CallbackManagerForToolRun(e.g.GoogleDriveSearchTool)._langchain_adapter.py, when inferring fields, skip parameters whose annotation resolves to a LangChain callback-manager type. Detection unwrapsOptional[...]/Union[...]and also falls back to the conventionalrun_managerparameter name for unresolvable (string) annotations (the module usesfrom __future__ import annotations).run_managerfrom the schema is correct — the tool still runs without it (it defaults toNone).test_langchain_tool_adapter_skips_run_manager) using a tool without an explicitargs_schema.Test plan
pytest python/packages/autogen-ext/tests/tools/test_langchain_tools.py— passes (2 tests).