-
Notifications
You must be signed in to change notification settings - Fork 524
feat: implement Bedrock v2 client #2343
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
base: main
Are you sure you want to change the base?
Conversation
|
@claude Review this PR. Summarize the features supported from bedrock v2 client and compare with https://github.com/ag2ai/ag2/blob/main/autogen/oai/bedrock.py |
|
Claude finished @priyansh4320's task in 3m 3s —— View job PR Review: Bedrock V2 Client ImplementationSummaryThis PR successfully implements a next-generation Bedrock V2 client ( Recommendation: ✅ Approve with minor suggestions Feature Comparison: V2 vs Legacy ClientBedrock V2 Client Features (
|
| Feature | V1 (bedrock) | V2 (bedrock_v2) | Notes |
|---|---|---|---|
| Response Format | ChatCompletion |
UnifiedResponse |
V2 is richer |
| Text Content | ✅ Flattened | ✅ Typed blocks | V2 preserves structure |
| Tool Calling | ✅ | ✅ | Same underlying API |
| Structured Outputs | ✅ Limited | ✅ Full support | V2 has better API |
| Image Input | ✅ | ✅ | Same support |
| Image Output | ✅ | ✅ | V2 has ImageContent blocks |
| System Prompts | ✅ | ✅ | Same support |
| Cost Tracking | ✅ | ✅ | Same pricing data |
| Retry Config | ✅ | ✅ | Identical |
| Auth Methods | ✅ | ✅ | Identical |
| Model Parameters | ✅ | ✅ | Same parameters |
| Streaming | ❌ | ❌ | Both warn and disable |
| Rich Content Access | ❌ | ✅ | V2 only |
| Forward Compatibility | ❌ | ✅ | V2 has GenericContent |
| Direct Property Access | ❌ | ✅ | V2 only |
| Backward Compat | N/A | ✅ | V2 has create_v1_compatible() |
Recommendations
For Approval
✅ Approve this PR - The implementation is solid, well-tested, and provides significant value:
- Modernized architecture with
UnifiedResponse - Better developer experience
- Forward compatibility
- Maintains backward compatibility via
create_v1_compatible()
For Future Work
-
Improve Test Coverage Metrics
- Add
# pragma: no coverto integration-only paths - Expand unit test edge case coverage
- Document coverage strategy
- Add
-
Reduce Code Duplication
- Extract shared structured output logic to utilities
- Share schema normalization code
-
Update Pricing Database
- Add more recent model pricing
- Add comment encouraging
priceparameter usage
-
Consider Deprecation Path for V1
- Add deprecation warning to V1 client in future release
- Provide migration tools/documentation
Questions for Author
- Is the low coverage (21.64%) acceptable given that many tests require AWS credentials?
- Should we extract the shared structured output logic now or in a follow-up PR?
- Are there plans to eventually deprecate the V1 client?
Files Reviewed:
- ✅ autogen/llm_clients/bedrock_v2.py (793 lines)
- ✅ autogen/oai/bedrock.py (915 lines)
- ✅ test/llm_clients/test_bedrock_v2.py (1049 lines)
- ✅ website/docs/user-guide/models/amazon-bedrock.mdx
- ✅ autogen/oai/client.py (registration logic)
Codecov Report❌ Patch coverage is
... and 20 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Why are these changes needed?
The current Bedrock client (api_type: "bedrock") uses the legacy ModelClient interface, which returns flattened ChatCompletion responses. This limits access to rich content types and creates inconsistencies across providers.
Related issue number
#2346
Checks