Skip to content

# 🔥🕊️ Rise of the Phoenix: Event Loop Refactor #106

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

Merged
merged 1 commit into from
May 26, 2025

Conversation

fede-dash
Copy link
Contributor

@fede-dash fede-dash commented May 25, 2025

🔨 Modularizing Event Loop Logic

Commit: 8cb8422
Author: Fede Kamelhar
Date: May 25, 2025
Message: Modularizing Event Loop

✨ What’s Included

  1. Extracted Tool Execution Logic

Moved all logic related to handling tool_use into a dedicated private function: _handle_tool_execution().

Benefit:
Improves readability, simplifies event_loop_cycle(), and separates concerns for better maintainability and testing.

New function:

def _handle_tool_execution(...) -> Tuple[StopReason, Message, EventLoopMetrics, Dict[str, Any]]:
if stop_reason == "tool_use":
    return _handle_tool_execution(...)
  1. Centralized Retry Configuration

Introduced constants for retry settings, replacing hardcoded values:

MAX_ATTEMPTS = 6
INITIAL_DELAY = 4
MAX_DELAY = 240  # 4 minutes

Benefit:
Easier tuning of retry behavior and removal of magic numbers.

  1. Type Hint Improvements

Refined function signatures for stronger typing:
• initialize_state() now accepts a Dict[str, Any] instead of **kwargs.
• callback_handler is now typed as Callable[..., Any].

Benefit:
Improves IDE support, readability, and static analysis.

  1. Small Refactors & Cleanup
    • Replaced inline retry configuration with named constants.
    • Removed redundant state handling and improved control flow.
    • Added a detailed docstring to _handle_tool_execution().

  2. Updated Tests

Updated test_initialize_state() to use the new function signature:

kwargs = strands.event_loop.event_loop.initialize_state(kwargs)

Benefit:
Keeps tests aligned with refactored API for better coverage and accuracy.

@fede-dash fede-dash changed the title Applying Black to increase readability and Modularizing Event Loop # 🔥🕊️ Rise of the Phoenix: Event Loop Refactor & Revival May 25, 2025
@fede-dash fede-dash marked this pull request as ready for review May 25, 2025 05:47
@fede-dash fede-dash requested a review from a team as a code owner May 25, 2025 05:47
Copy link
Member

@awsarron awsarron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fede-dash, great to see you here! Thank you for your contribution ❤️

Would it be possible to separate out the formatting changes from this PR? We're using ruff with hatch fmt for formatting and linting (https://github.com/strands-agents/sdk-python/blob/main/CONTRIBUTING.md) so it seems the black rules that were applied in this PR don't match with what we have configured in the project (https://github.com/strands-agents/sdk-python/blob/main/pyproject.toml).

The event_loop improvements and modularization are great, would love to get those merged.

@fede-dash fede-dash changed the title # 🔥🕊️ Rise of the Phoenix: Event Loop Refactor & Revival # 🔥🕊️ Rise of the Phoenix: Event Loop Refactor May 25, 2025
@fede-dash fede-dash requested a review from awsarron May 25, 2025 18:52
@fede-dash
Copy link
Contributor Author

Comments addressed

Copy link
Member

@awsarron awsarron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

hatch run test-lint is showing these errors:

cmd [1] | ruff check
All checks passed!
cmd [2] | mypy -p src
src/strands/event_loop/event_loop.py:213: error: Argument 1 to "EventLoopException" has incompatible type "str"; expected "Exception"  [arg-type]
src/strands/event_loop/event_loop.py:224: error: Argument 6 to "_handle_tool_execution" has incompatible type "ToolConfig | None"; expected "ToolConfig"  [arg-type]
Found 2 errors in 1 file (checked 51 source files)

Other than that, looks great. I gave it a test locally as well ☺️

@awsarron awsarron self-assigned this May 25, 2025
@fede-dash
Copy link
Contributor Author

lints passing now - will do that moving forward

hatch run test-lint
cmd [1] | ruff check
All checks passed!
cmd [2] | mypy -p src
Success: no issues found in 51 source files

@awsarron awsarron enabled auto-merge (squash) May 26, 2025 01:15
@awsarron awsarron merged commit a331e63 into strands-agents:main May 26, 2025
10 checks passed
@awsarron
Copy link
Member

Thank you @fede-dash 🚀 !!

@fede-dash
Copy link
Contributor Author

🔥🕊️ Always will be!

awsarron pushed a commit to awsarron/sdk-python that referenced this pull request May 26, 2025
awsarron added a commit that referenced this pull request May 26, 2025
* models - openai - argument none (#97)

* docs(readme): add open PRs badge + link to samples repo + change 'Docs' to 'Documentation' (#100)

* docs(readme): add logo (#101)

* docs(readme): add logo, title, badges, links to other repos, standardize headings (#102)

* style(readme): use dark logo for clearer visibility when system is using light color scheme (#104)

* fix(readme): use logo that changes color automatically depending on user's color preference scheme (#105)

* feat(handlers): add reasoning text to callback handler and related tests (#109)

* feat(handlers): add reasoning text to callback handler and related tests

* feat(handlers): removed redundant comment in .gitignore file

* feat(handlers): Updated reasoningText type as (Optional[str]

* feat: Add dynamic system prompt override functionality (#108)

* Modularizing Event Loop (#106)

* fix(telemetry): fix agent span start and end when using Agent.stream_async() (#119)

* feat: Update SlidingWindowConversationManager (#120)

* v0.1.5

---------

Co-authored-by: Patrick Gray <[email protected]>
Co-authored-by: Gokhan (Joe) Gultekin <[email protected]>
Co-authored-by: Shubham Raut <[email protected]>
Co-authored-by: fede-dash <[email protected]>
Co-authored-by: Nick Clegg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants