Skip to content
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

Bedrock ConverseStreamResponse provides no easy way to read the streamed response without blocking the calling .NET thread #3542

Open
1 task
tlecomte opened this issue Nov 4, 2024 · 5 comments
Labels
bug This issue is a bug. module/sdk-generated p2 This is a standard priority issue v4

Comments

@tlecomte
Copy link

tlecomte commented Nov 4, 2024

Describe the bug

The common way to read from Bedrock ConverseStreamResponse is apparently to call response.Stread.AsEnumerable(). This achieves the goal of streaming the response completions as they are received, but at the cost of keeping the calling .NET thread busy. So it can lead to thread exhaustion.

Ideally we need a new variant that does not block the thread, so maybe an async enumerator exposed as response.Stread.AsAsyncEnumerable().

Regression Issue

  • Select this option if this issue appears to be a regression.

Expected Behavior

I expect to have an easy method that gives an IAsyncEnumerable from ConverseStreamResponse, and which is implemented without blocking the calling thread.

Current Behavior

The current API surface in ConverseStreamResponse encourages the use of response.Stread.AsEnumerable() which blocks the calling thread.

Reproduction Steps

N/A

Possible Solution

This problem was mentioned in #3360 (comment) for a further improvement, with a suggested implementation. I believe the suggested implementation is imperfect, but it can be a basis for a solution.

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

AWSSDK.BedrockRuntime 3.7.404.8

Targeted .NET Platform

.NET 8

Operating System and version

Docker

@tlecomte
Copy link
Author

tlecomte commented Nov 4, 2024

This is in principle how it could be implemented, using an extension, and with System.Threading.Channels:

public static class ConverseStreamOutputExtensions
{
        public async IAsyncEnumerable<IEventStreamEvent> AsAsyncEnumerable(ConverseStreamOutput output, [EnumeratorCancellation] CancellationToken cancellationToken)
        {
            var channel = Channel.CreateUnbounded<IEventStreamEvent>();

            void OnEventReceived(object? sender, EventStreamEventReceivedArgs<IEventStreamEvent> args)
            {
                var _ = channel.Writer.TryWrite(args.EventStreamEvent);

                // the metadata event is the last one of the event stream
                if (args.EventStreamEvent is ConverseStreamMetadataEvent)
                {
                    channel.Writer.Complete();
                }
            }

            void OnExceptionReceived(object? sender, EventStreamExceptionReceivedArgs<BedrockRuntimeEventStreamException> args)
            {
                var _ = channel.Writer.TryComplete(args.EventStreamException);
            }

            Task processingTask;

            try
            {
                output.EventReceived += OnEventReceived;
                output.ExceptionReceived += OnExceptionReceived;

                processingTask = output..StartProcessingAsync();

                await foreach (var e in channel.Reader.ReadAllAsync(cancellationToken))
                {
                    yield return e;
                }
            }
            finally
            {
                output.EventReceived -= OnEventReceived;
                output.ExceptionReceived -= OnExceptionReceived;
            }

            await processingTask.ConfigureAwait(false);
        }
}

Not completely sure how such code could be added to the SDK since all the files say that there are auto-generated from the service model json...

@ashishdhingra
Copy link
Contributor

Needs review with the team.

@ashishdhingra ashishdhingra added p2 This is a standard priority issue needs-review and removed needs-triage This issue or PR still needs to be triaged. labels Nov 4, 2024
@tlecomte
Copy link
Author

tlecomte commented Nov 4, 2024

I've submitted a pull request to add support for IAsyncEnumerable in EnumerableEventStream, which makes it available for Bedrock: #3543

It's much easier than with the extension above, fortunately.

@bhoradc bhoradc added v4 and removed needs-review labels Nov 8, 2024
@shethaadit
Copy link

Hi @ashishdhingra, which SDK version will get this change?

@ashishdhingra
Copy link
Contributor

Hi @ashishdhingra, which SDK version will get this change?

@shethaadit The issue is marked with v4 label. It should be available in next major version v4 of AWS .NET SDK, which is currently in preview.

CC @normj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. module/sdk-generated p2 This is a standard priority issue v4
Projects
None yet
Development

No branches or pull requests

4 participants