refactor: simplify AmazonBedrockAiProvider by removing unused parameters #1488
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the AmazonBedrockAiProvider class by removing private fields that were considered unused: temperature, topP, and region. The changes simplify the constructor and inference configuration.
- Removed temperature, topP, and region private fields
- Simplified BedrockRuntimeClient initialization by removing explicit region configuration
- Reduced inferenceConfig to only specify maxTokens
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.bedrockRuntimeClient = new BedrockRuntimeClient({ | ||
| region: this.region, | ||
| }); | ||
| this.bedrockRuntimeClient = new BedrockRuntimeClient(); |
There was a problem hiding this comment.
Removing the explicit region configuration means the BedrockRuntimeClient will now rely on the AWS SDK's default region resolution mechanism (environment variables, AWS config files, or instance metadata). This could lead to inconsistent behavior across different deployment environments if the region is not explicitly configured elsewhere. Consider whether the region should remain explicit to ensure consistent behavior, or document the requirement for proper AWS configuration in deployment environments.
| this.bedrockRuntimeClient = new BedrockRuntimeClient(); | |
| const region = process.env.AWS_REGION ?? process.env.AWS_DEFAULT_REGION; | |
| if (!region) { | |
| throw new Error( | |
| 'AWS region is not configured. Please set AWS_REGION or AWS_DEFAULT_REGION environment variable.', | |
| ); | |
| } | |
| this.bedrockRuntimeClient = new BedrockRuntimeClient({ region }); |
| modelId: this.modelId, | ||
| messages: conversation, | ||
| inferenceConfig: { maxTokens: this.maxTokens, temperature: this.temperature, topP: this.topP }, | ||
| inferenceConfig: { maxTokens: this.maxTokens }, |
There was a problem hiding this comment.
Removing the temperature and topP parameters from the inferenceConfig changes the behavior of the AI model. Without explicit values, AWS Bedrock will use its default values, which may differ from the previously configured values (temperature: 0.7, topP: 0.9). This could lead to inconsistent AI responses compared to the previous implementation. Consider whether the default AWS values align with your requirements for response consistency and creativity, or if these parameters should be retained with explicit values.
| inferenceConfig: { maxTokens: this.maxTokens }, | |
| inferenceConfig: { maxTokens: this.maxTokens, temperature: 0.7, topP: 0.9 }, |
No description provided.