refactor(llm): unify invoke/ainvoke dispatch across providers - #626
Open
matteomedioli wants to merge 3 commits into
Open
matteomedioli wants to merge 3 commits into
matteomedioli wants to merge 3 commits into
Conversation
matteomedioli
force-pushed
the
matteo/gemini-llm-sync-async-core-split-GENKGB-1699
branch
2 times, most recently
from
September 14, 2026 13:40
7ce315b to
73ec23b
Compare
LLMBase.invoke/ainvoke were abstract dispatchers every subclass had to reimplement. Makes them concrete: they branch on input type (str vs list[LLMMessage]) and delegate to four narrow abstract hooks (_invoke_v1, _invoke_v2, _ainvoke_v1, _ainvoke_v2). All eight providers (AnthropicLLM, OpenAILLM, GeminiLLM, CohereLLM, MistralAILLM, OllamaLLM, BedrockLLM, VertexAILLM) migrate onto this contract, each sharing one request builder and response parser per input version plus a _call_sync/_call_async transport pair that differs only in whether the SDK call is awaited. Removes the duplicated sync/async dispatch and request/response logic that previously lived four times per provider. Also fixes bugs the duplication let drift out of sync: - BedrockLLM now inherits from LLMBase (previously LLMInterface/ LLMInterfaceV2 directly); its async path no longer stacks a sync-side retry inside the thread-pool call on top of its own async retry layer. - OllamaLLM's async path spreads model_params into client.chat the same way the sync path always did, instead of passing it verbatim as the options value. - CohereLLM's v2 error paths raise LLMGenerationError(e) from e consistently, preserving the original SDK exception. Breaking (targets 2.0): a subclass overriding invoke/ainvoke directly must migrate to overriding the four hooks instead. The public invoke/ainvoke/invoke_with_tools/ainvoke_with_tools calling contract for callers is unchanged.
matteomedioli
force-pushed
the
matteo/gemini-llm-sync-async-core-split-GENKGB-1699
branch
from
September 14, 2026 13:44
73ec23b to
ce86eae
Compare
matteomedioli
deleted the
matteo/gemini-llm-sync-async-core-split-GENKGB-1699
branch
September 14, 2026 13:57
matteomedioli
restored the
matteo/gemini-llm-sync-async-core-split-GENKGB-1699
branch
September 14, 2026 14:04
matteomedioli
marked this pull request as ready for review
September 15, 2026 07:52
matteomedioli
force-pushed
the
matteo/gemini-llm-sync-async-core-split-GENKGB-1699
branch
from
September 15, 2026 12:40
a82c87d to
ee58fbd
Compare
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
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.
Description
Refactors
LLMBase.invoke/ainvokefrom an abstract dispatcher every subclass had to reimplement into a concrete dispatcher: it branches on input type (strvslist[LLMMessage]) and delegates to four narrow abstract hooks (_invoke_v1,_invoke_v2,_ainvoke_v1,_ainvoke_v2). All eight providers (AnthropicLLM,OpenAILLM,GeminiLLM,CohereLLM,MistralAILLM,OllamaLLM,BedrockLLM,VertexAILLM) migrated onto this contract, each now sharing one request builder and response parser per input version, plus a pair of_call_sync/_call_asynctransport hooks that differ only in whether the SDK call is awaited. This removes the duplicated sync/async dispatch and request/response logic that previously lived four times per provider.Along the way, this fixes bugs the duplication had let drift out of sync:
BedrockLLMnow inherits fromLLMBase(previouslyLLMInterface/LLMInterfaceV2directly), aligning its hierarchy with every other provider. Its async path no longer stacks a sync-side rate-limit retry inside the thread-pool-executed call on top of its own async retry layer.OllamaLLM's async path now spreadsmodel_paramsinto theclient.chatcall the same way the sync path always did, instead of passing it verbatim as theoptionsvalue — this fixed a double-nestedoptionsbug whenmodel_paramscarried a sibling key (e.g.{"options": {...}, "format": "json"}).CohereLLM's v2 error paths now raiseLLMGenerationError(e) from econsistently, preserving the original SDK exception (previously a generic"Error calling cohere"message on v2 only).Breaking (targets 2.0): a subclass overriding
invoke/ainvokedirectly (a supported extension point forBaseAnthropicLLM/BaseOpenAILLM/BaseGeminiLLM) must migrate to overriding the four hooks instead. The publicinvoke/ainvoke/invoke_with_tools/ainvoke_with_toolscalling contract for callers is unchanged.Note:
GeminiLLM's usage-tracking fix (response.usage_metadata->LLMResponse.usage) landed separately in #621 and is already onmain; this branch is rebased on top of it, not re-introducing it.Type of Change
Complexity
Complexity: Medium
How Has This Been Tested?
Checklist
The following requirements should have been met (depending on the changes in the branch):