fix(llm_agent): add duck typing fallback for BaseLlm in bundled environments#35
fix(llm_agent): add duck typing fallback for BaseLlm in bundled environments#35Alex980102 wants to merge 1 commit intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
3574f3c to
0468123
Compare
…onments When packages are bundled (e.g., with esbuild, webpack), the BaseLlm class identity may differ between the bundled code and external packages, causing instanceof to fail. This duck-type check ensures compatibility.
0468123 to
4213492
Compare
- Move LLMRegistry.register() pattern to Quick Start (stable) - Add 'Known Issue' section explaining instanceof bundling problem - Link to upstream PR google/adk-js#35 - Mark AIGateway() direct usage with workaround reference - Examples already use the stable pattern
- Move LLMRegistry.register() pattern to Quick Start (stable) - Add 'Known Issue' section explaining instanceof bundling problem - Link to upstream PR google/adk-js#35 - Mark AIGateway() direct usage with workaround reference - Examples already use the stable pattern
| if (this.model instanceof BaseLlm) { | ||
| return this.model; | ||
| } | ||
| // Duck typing fallback for bundled environments where instanceof fails. | ||
| if (this.model && typeof this.model === 'object' && | ||
| 'model' in this.model && | ||
| 'generateContentAsync' in this.model && | ||
| typeof (this.model as BaseLlm).generateContentAsync === 'function') { | ||
| return this.model as BaseLlm; | ||
| } |
There was a problem hiding this comment.
I was thinking of another a bit more robust approach here using the symbols.
in the BaseLlm:
const BASE_MODEL_SYMBOL = Symbol('baseModel');
export function isBaseLlm(obj: unknown): obj is BaseLlm {
return typeof obj === 'object' && obj !== null && BASE_MODEL_SYMBOL in obj && obj[BASE_MODEL_SYMBOL] === true;
}
/**
* The BaseLLM class.
*/
export abstract class BaseLlm {
readonly [BASE_MODEL_SYMBOL] = true;
// ...
}Then use the isBaseLlm function here:
| if (this.model instanceof BaseLlm) { | |
| return this.model; | |
| } | |
| // Duck typing fallback for bundled environments where instanceof fails. | |
| if (this.model && typeof this.model === 'object' && | |
| 'model' in this.model && | |
| 'generateContentAsync' in this.model && | |
| typeof (this.model as BaseLlm).generateContentAsync === 'function') { | |
| return this.model as BaseLlm; | |
| } | |
| if (isBaseLlm(this.model)) { | |
| return this.model; | |
| } |
There was a problem hiding this comment.
Thanks for the feedback, @kalenkevich! I see the Symbol approach with isBaseLlm() is already implemented in main. Should I close this PR since the fix is already in place?
There was a problem hiding this comment.
Hello, sorry forgot to mention it here.
Yes please, close the PR. Thanks for your issue.
Summary
This PR fixes an issue where custom
BaseLlmsubclasses fail to be recognized in bundled environments.Fixes #36
Problem
When packages are bundled (e.g., with
adk-devtools, webpack, esbuild), theBaseLlmclass identity differs between the bundled code and external packages. This causesinstanceof BaseLlmto returnfalsefor valid subclasses, resulting in:Solution
Add a duck typing fallback in
canonicalModelgetter that checks for requiredBaseLlmproperties wheninstanceoffails.Testing
Related