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

.Net: Bug: .NET Bedrock streaming completion is keeping the request thread busy #9519

Closed
tlecomte opened this issue Nov 4, 2024 · 3 comments · Fixed by #10159
Closed

.Net: Bug: .NET Bedrock streaming completion is keeping the request thread busy #9519

tlecomte opened this issue Nov 4, 2024 · 3 comments · Fixed by #10159
Assignees
Labels
bug Something isn't working .NET Issue or Pull requests regarding .NET code

Comments

@tlecomte
Copy link

tlecomte commented Nov 4, 2024

Describe the bug
The current implementation of the Bedrock connector is using response.Stream.AsEnumerable() in BedrockChatCompletionClient.cs#L193, which is keeping the current .NET thread busy while it reads the response stream. This could cause thread exhaustion issues.

To Reproduce
Steps to reproduce the behavior:

Expected behavior
Replace .AsEnumerable() with something that would read the response stream in an async manner.

Screenshots
N/A

Platform

  • OS: all
  • IDE: N/A
  • Language: C#
  • Source: NuGet package version 1.26.0

Additional context

@tlecomte tlecomte added the bug Something isn't working label Nov 4, 2024
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code triage labels Nov 4, 2024
@github-actions github-actions bot changed the title Bug: .NET Bedrock streaming completion is keeping the request thread busy .Net: Bug: .NET Bedrock streaming completion is keeping the request thread busy Nov 4, 2024
@tlecomte
Copy link
Author

tlecomte commented Nov 4, 2024

I realize that, ideally, the AWS .NET SDK would provide a solution out of the box. So I've also created aws/aws-sdk-net#3542 and will suggest a solution there first.

@evchaki
Copy link
Contributor

evchaki commented Nov 4, 2024

@RogerBarreto - can you take a look at this? @tlecomte - thanks for submitting this, we will take a look at updating this.

github-merge-queue bot pushed a commit that referenced this issue Jan 16, 2025
…ocking (#10159)

### Motivation and Context

This change replaces the use of `response.Stream.AsEnumerable()` with
`await foreach` for async streaming in the Amazon Bedrock SDK. The
previous method blocked the calling thread, leading to potential thread
exhaustion in high-concurrency scenarios. This update aligns with best
practices for asynchronous operations in .NET.

### Description

- Replaced blocking foreach with `await foreach` for streaming
`ConverseStreamResponse` chunks asynchronously.
- Upgraded Amazon Bedrock runtime and core SDKs to support this
functionality.

### Contribution Checklist

- [Y] The code builds clean without any errors or warnings
- [Y] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [Y] All unit tests pass, and I have added new tests where possible
- [Y] I didn't break anyone 😄

Fixes #9519

---------

Co-authored-by: Adit Sheth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working .NET Issue or Pull requests regarding .NET code
Projects
Status: Bug
5 participants