Remove caching logic from lib/
#779
Conversation
🦋 Changeset detectedLatest commit: 1683cb9 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR removes all LLM caching logic from the library level, centralizing caching at the observe level instead. The changes involve removing the entire caching infrastructure including cache classes and related functionality from LLM clients.
- Removes entire caching infrastructure (
BaseCache,ActionCache,LLMCache) and all cache-related files fromlib/cache/ - Removes caching logic from all LLM clients (OpenAI, Anthropic, Google, Groq, Cerebras, AISDK) including constructor params and cache operations
- Removes cache cleanup code from
StagehandPageextract/observe operations - Removes
enableCachingparameter fromLLMProviderconstructor while maintaining flag at Stagehand class level - Preserves core LLM client functionality for API calls, response handling and error management
13 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| this.modelName = modelName; | ||
| this.clientOptions = clientOptions; |
There was a problem hiding this comment.
style: Redundant assignment of modelName since it's already set in super constructor
| this.modelName = modelName; | |
| this.clientOptions = clientOptions; | |
| this.clientOptions = clientOptions; |
| @@ -121,7 +112,7 @@ export class OpenAIClient extends LLMClient { | |||
| throw new StagehandError("Temperature is not supported for o1 models"); | |||
| } | |||
|
|
|||
| const { image, requestId, ...optionsWithoutImageAndRequestId } = options; | |||
| const { requestId, ...optionsWithoutImageAndRequestId } = options; | |||
There was a problem hiding this comment.
logic: The image property is still being destructured in the variable name but was removed from the actual destructuring
| public clientOptions: ClientOptions; | ||
|
|
||
| constructor({ | ||
| enableCaching = false, | ||
| cache, | ||
| modelName, | ||
| clientOptions, | ||
| }: { | ||
| logger: (message: LogLine) => void; |
There was a problem hiding this comment.
style: The logger parameter is required in the constructor type but not used
why
This needs a refactor -- caching should be done at the
observelevel, not at the LLM inference level. In order to do the refactor though, we should clean up this logicwhat changed
Removed existing LLM caching logic
test plan
Evals + running custom client examples