fix(interop/langchain): route async Langchain tools through arun#2790
Closed
genisis0x wants to merge 4 commits into
Closed
fix(interop/langchain): route async Langchain tools through arun#2790genisis0x wants to merge 4 commits into
genisis0x wants to merge 4 commits into
Conversation
LangChainInteroperability.convert_tool wrapped every Langchain tool in a sync function that called langchain_tool.run(...). For async-native tools this blocked the calling event loop and discarded the tool's arun implementation, even when AG2 was running under a_initiate_chat. Detect async-native tools and wrap them with a coroutine function that awaits arun: * StructuredTool / Tool: rely on the `coroutine` attribute, which is None for sync wrappers and the original async function for async ones. The class-level _arun is always overridden, so checking _arun alone misclassifies sync StructuredTools. * Direct BaseTool subclasses: no `coroutine` attribute, so fall back to checking whether `_arun` was overridden away from BaseTool's default thread-pool delegate and is a coroutine function. Sync tools keep the existing wrapper untouched. AG2's Tool already accepts coroutine functions as func_or_tool, so the async path needs no further plumbing. Test coverage: * TestLangChainInteroperabilityAsyncTool: @langchain_tool on an async function produces an async wrapper and invoking it calls arun. * TestLangChainInteroperabilityBaseToolAsync: a custom BaseTool that overrides only _arun is detected and routed through arun; its sync _run raises so the test fails if the wrapper falls back to .run. Closes ag2ai#1402
|
|
…typos Resolve CI failures on PR ag2ai#2790: - mypy `[misc]` "All conditional function variants must have identical signatures" on the inline `if/else` defining `func` twice. Replace the inline redefinition with two distinct nested callables (`_async_func`, `_sync_func`) and pick one based on `_is_async_langchain_tool`. - typos hook flagged `mis-classify` in the helper docstring. Replace with `misclassify` (one word, not in the typo dictionary). - ruff auto-moves the `Callable` import from `typing` to `collections.abc` per project rules. Behaviour is unchanged. The existing test classes already exercise both branches: sync StructuredTool, async StructuredTool, BaseTool subclass with overridden `_arun`.
…error paths Codecov flagged the original PR at 16.66% patch coverage. Add three tests that lift that to 96% locally and exercise every branch the fix introduced: - TestLangChainInteroperabilityBaseToolSync — direct BaseTool subclass with only _run overridden. Pins the False branch of _is_async_langchain_tool: no `coroutine` attribute, _arun unchanged from the BaseTool default, so convert_tool must fall back to the synchronous wrapper and the resulting Tool.func must be a plain function rather than a coroutine function. - TestLangChainInteroperabilityConvertToolErrors::test_rejects_non_langchain_tool exercises the first ValueError guard. - TestLangChainInteroperabilityConvertToolErrors::test_rejects_extra_kwargs exercises the second ValueError guard. Only the get_unsupported_reason fallback (returned when langchain is not installed) remains uncovered locally; covering it requires unloading langchain from sys.modules, which would interfere with the sibling tests in this file.
Codecov Report❌ Patch coverage is
... and 154 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Collaborator
|
Thanks for the fix — the Flagging a conflict: PR #2806 (by @obchain) covers the same fix to |
Collaborator
|
Thanks for the async Langchain fix — it addresses the same issue as PR #2806 which was opened shortly before and has already been approved. PR #2806 also includes tests that cover the async path via the interop No action needed on your end — this is just a heads-up that the fix is already in flight. |
3 tasks
marklysze
approved these changes
May 17, 2026
Collaborator
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Clean fix for #1402. The two-case detection in _is_async_langchain_tool is the right approach:
- StructuredTool/Tool path: checking
tool.coroutine is not Noneis the reliable signal — the class always overrides_arun, so a plain MRO check would misclassify sync-wrapped tools as async. - BaseTool subclass path: comparing
cls_arun is not base_arun+iscoroutinefunctionis precise and avoids false positives from the default delegating_arun.
Note: PR #2618 (Ricardo-M-L, April 11) addresses the same issue but uses MRO-only detection which misses the StructuredTool case. This implementation is more complete.
CI is clean and tests cover both paths. ✓
Ricardo-M-L
added a commit
to Ricardo-M-L/ag2
that referenced
this pull request
May 20, 2026
Review feedback on ag2ai#2618 (thanks @marklysze): the MRO-only check misclassified every callable wrapped with `@langchain_core.tools.tool` as async, because `StructuredTool` / `Tool` always override `_arun` at the class level — the override internally delegates to the `coroutine` attribute (None for sync callables, the async function for async ones). Sync wrappers therefore got routed through `ainvoke` unnecessarily. Split `_langchain_tool_has_async_implementation` into two paths: 1. StructuredTool / Tool — check `tool.coroutine is not None` directly. This is the only reliable signal; the class-level `_arun` override would otherwise force a false positive. 2. BaseTool subclasses — keep the existing MRO walk (the original case the PR was written for). Same two-case detection as ag2ai#2790's `_is_async_langchain_tool` helper. Adds two regression tests: - `test_structured_tool_sync_wrapped_callable_is_not_async` — fails on the old MRO-only code, passes here. - `test_structured_tool_async_wrapped_callable_is_async` — pins that async wrappers stay correctly classified via `coroutine`.
Collaborator
|
@genisis0x Thanks for the pull request! This PR #2806 closes the issue. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1402.
LangChainInteroperability.convert_toolwraps every Langchain tool in a sync function that callslangchain_tool.run(...), even when the wrapped tool is async-native. Inside anawait user.a_initiate_chat(...)flow this blocks the calling event loop and bypasses the tool'sarunimplementation.This PR detects async-native tools and wraps them with a coroutine function that awaits
arun. AG2'sToolalready accepts coroutine functions asfunc_or_tool, so no further plumbing is needed.Detection
Two cases are handled separately because Langchain models them differently:
@tool-decorated tools — the only reliable signal is thecoroutineattribute, which isNonefor sync wrappers and the original async function for async ones. These classes always override_arun, so a class-level_aruncheck would misclassify sync StructuredTools as async (this is what the first iteration of this patch did, and it brokeTestLangChainInteroperabilityWithoutPydanticInput::test_convert_toollocally).BaseToolsubclasses — nocoroutineattribute. Fall back to checking whether_arunwas overridden away fromBaseTool's default thread-pool delegate and is a coroutine function.Sync tools keep the existing sync wrapper unchanged.
Tests
New test classes:
TestLangChainInteroperabilityAsyncTool—@langchain_toolon an async function produces an async wrapper (inspect.iscoroutinefunction(tool.func)is True) and awaiting it actually callsarun.TestLangChainInteroperabilityBaseToolAsync— a customBaseToolsubclass that overrides only_arunis detected and routed througharun. Its sync_runraisesRuntimeError, so the test fails if the wrapper falls back torun.The two pre-existing sync tests (
TestLangChainInteroperability::test_convert_toolandTestLangChainInteroperabilityWithoutPydanticInput::test_convert_tool) continue to pass, confirming sync tools still take the sync path.Test plan
pytest test/interop/langchain/test_langchain.py -k 'not test_with_llm'— all green@langchain_toolstill wrapped with a sync function (existing tests verify this)@langchain_toolwrapped with a coroutine function and awaited viaarunBaseToolsubclass with overridden_arunrouted througharun