-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: optimize Bedrock model configuration performance [close: #5419, #5420] #5434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: optimize Bedrock model configuration performance [close: #5419, #5420] #5434
Conversation
1. Add ModelConfigCache class for model config caching strategy\n2. Remove deprecated legacy model definitions\n3. Optimize cross-region inference and custom ARN handling\n4. Add comprehensive test suite for cache functionality
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @KevinZhao I reviewed your PR and left a couple of suggestions.
Let me know if you have any questions!
| logger.debug("Computed and cached new model config", { | ||
| ctx: "bedrock", | ||
| modelId: config.id, | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering about memory management for long-running applications. Since cache entries are never evicted, could this potentially lead to memory issues if many different model configurations are used over time? Would it make sense to implement a cache size limit or TTL (time-to-live) for entries?
| return config | ||
| } | ||
|
|
||
| private computeModelConfig(): ModelConfigResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add error handling for the cache operations? While Map operations are generally safe, if we extend this cache in the future or if there are edge cases, having try-catch blocks could prevent unexpected failures from affecting the main flow.
| ConverseCommand: vi.fn(), | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using as any to access private properties, would it be cleaner to either make these properties protected for testing purposes or use a testing utility that provides type-safe access to private members? This would improve type safety in the tests.
| expect(getModelByIdSpy).toHaveBeenCalledWith("anthropic.claude-3-5-sonnet-20241022-v2:0", "inference-profile") | ||
|
|
||
| // Verify that getModel returns the updated model info | ||
| // 在这里模拟 invokedModelId 后模型的 inputPrice 被更新为 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we translate these Chinese comments to English for consistency? The comments explain that this simulates the model's inputPrice being updated to 8 after invokedModelId, which is a key part of the test verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description mentions performance optimization, but I don't see any benchmarks or performance metrics. Would it be helpful to add performance tests that demonstrate the actual improvement? This would validate the optimization claims and help prevent performance regressions in the future.
Related GitHub Issue
Closes: #5419, #5420
Roo Code Task Context (Optional)
Description
Creating simple caching for model configuration to avoid multi invocation of getModel()
Remove some legacy models or embedding models.
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
Introduces caching for model configurations in
AwsBedrockHandler, removes deprecated models, and adds tests for cache functionality.ModelConfigCacheclass inbedrock.tsfor caching model configurations.AwsBedrockHandlerusesModelConfigCacheto cache results ofgetModel().ProviderSettings.bedrockModelsinbedrock.ts.bedrock-cache-strategy.spec.tsto test caching functionality.AwsBedrockHandler.This description was created by
for e81ff4d. You can customize this summary. It will automatically update as commits are pushed.