Add integration tests for Bedrock Runtime #37
Conversation
jonathan343
left a comment
jonathan343
left a comment
There was a problem hiding this comment.
Thanks Alessandra, this looks pretty good already. Just have a few questions
clients/aws-sdk-bedrock-runtime/tests/integration/test_bidirectional_streaming.py
Outdated
Show resolved
Hide resolved
clients/aws-sdk-bedrock-runtime/tests/integration/test_non_streaming.py
Outdated
Show resolved
Hide resolved
| if not isinstance(out, InvokeModelWithBidirectionalStreamOutputChunk): | ||
| continue |
There was a problem hiding this comment.
What else are we receiving in the stream that we're skipping here?
There was a problem hiding this comment.
The stream can yield both normal OutputChunk events and modeled error events (InternalServerException, ValidationException, ThrottlingException, etc.). Those error types don't have a bytes_ payload, so the isinstance check narrows the type and keeps pyright happy.
There was a problem hiding this comment.
Can this be successful then if we get an error chunk as the last part of our stream and just ignore it? I don't immediately remember how termination of the stream works.
There was a problem hiding this comment.
Good catch. You're right that if we receive error events after getting some successful chunks the test would still pass since we just continue and the assertions only check that we got some output. Should we fail explicitly on error events? or is it acceptable for the stream to send partial results followed by an error?
There was a problem hiding this comment.
A successful stream shouldn't terminate with errors so I think we'd want to consider that a failure case. I know there have been a lot of unintuitive behaviors with the Bedrock APIs and how they surface errors. I would not be surprised if we get back partial data before it fails, so we should account for that in the test.
If we're seeing typing issues, let's take a look at that. I wonder if we're missing partial definitions or type unions somewhere.
nateprewitt
left a comment
nateprewitt
left a comment
There was a problem hiding this comment.
Thanks @alexgromero!
* Add integration tests for Bedrock Runtime (#37) * Add integration tests for Transcribe Streaming (#38) * Update integration tests to use the latest Amazon Nova 2 models (#39) * Update to latest bedrock-runtime model * Release: aws-sdk-bedrock-runtime-0.3.0 * Release: AWS SDK Transcribe Streaming 0.3.0 * Release: AWS SDK Sagemaker Runtime HTTP2 0.3.0 * Release: aws-sdk-python 0.3.0 --------- Co-authored-by: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Co-authored-by: jonathan343 <43360731+jonathan343@users.noreply.github.com>
3 participants
Issue #, if available:
Description of changes:
This PR adds comprehensive integration tests for Amazon Bedrock Runtime client covering all streaming patterns:
converseconverse_streaminvoke_model_with_bidirectional_streamBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.