8000 models - bedrock - remove signaling by pgrayy · Pull Request #429 · strands-agents/sdk-python · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

models - bedrock - remove signaling #429

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 12, 2025
Merged

models - bedrock - remove signaling #429

merged 1 commit into from
Jul 12, 2025

Conversation

pgrayy
Copy link
Member
@pgrayy pgrayy commented Jul 12, 2025 •
edited
Loading

Description

The bedrock model provider runs converse stream in a separate thread so as not to block the async event loop. This stream thread pushes events onto a queue for the main thread to read and subsequently yield up to the agent caller. For each new event, the worker thread waits until the main thread signals that it is done yielding. This was an idea taken from the iterative tool implementation that would allow for a form of return of control. The problem however is if the main thread terminates before the stream thread completes, the stream thread can get blocked waiting for a signal.

The fix is to remove signaling. Unlike iterative tool invocation, return of control is not needed for model event streaming. The stream thread can continue queuing events from the model invocation while the main thread reads the events at its leisure. There is no harm in letting the events buffer on the queue. Bedrock is doing this anyway server side.

Related Issues

Identified during 0.3.0 bug bash.

Documentation PR

N/A

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli

  • I ran hatch run prepare
  • Ran hatch test tests_integ/models/test_model_bedrock.py
  • Also ran the following test script:
import asyncio

import pydantic
from strands import Agent

class Color(pydantic.BaseModel):
    """Describes a color."""

    name: str


async def func_a():
    print("FUNC_A STARTING")
    agent = Agent(callback_handler=None)
    result = await agent.structured_output_async(Color, "What is the color of the sky?")
    print(f"FUNC_A: {result}")


async def func_b():
    print("FUNC_B STARTING")
    agent = Agent(callback_handler=None)
    result = await agent.structured_output_async(Color, "What is the color of mars?")
    print(f"FUNC_B: {result}")


async def func_c():
    print("FUNC_C STARTING")
    agent = Agent(callback_handler=None)
    result = await agent.structured_output_async(Color, "Please don't call any tools")
    print(f"FUNC_C: {result}")


async def main():
    await asyncio.gather(func_a(), func_b(), func_c())


asyncio.run(main())

func_c is intentionally setup to raise an exception. If it does so before func_a and func_b complete, the stream threads from the func_a and func_b bedrock model instances will get stuck on the signal.wait. Removing signaling fixed the issue. The stream thread is allowed to complete even if the main thread goes down.

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

pgrayy
pgrayy commented Jul 12, 2025
View reviewed changes
@pgrayy pgrayy marked this pull request as ready for review July 12, 2025 02:59
@pgrayy pgrayy merged commit 812b1d3 into strands-agents:main Jul 12, 2025
22 checks passed
@pgrayy pgrayy deleted the bedrock-fix branch July 12, 2025 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cagataycali cagataycali cagataycali approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pgrayy @cagataycali
0