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

fix(backend): Avoid long synchronous call to block FastAPI event-loop #8429

Merged
merged 5 commits into from
Oct 27, 2024

Conversation

majdyz
Copy link
Contributor

@majdyz majdyz commented Oct 24, 2024

Background

We are doing some synchronous calls (pyro calls) on our FastAPI handler, which can potentially block the event loop and render the API unavailable.

Changes 🏗️

  • Remove async on function handlers that call pyro & don't require async calls.
  • Uses asyncio.to_thread to execute pyro calls that mix with async calls.

Testing 🔍

Note

Only for the new autogpt platform, currently in autogpt_platform/

  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

@majdyz majdyz requested a review from aarushik93 October 24, 2024 16:43
@majdyz majdyz requested a review from a team as a code owner October 24, 2024 16:43
@majdyz majdyz requested review from Torantulino and removed request for a team October 24, 2024 16:43
@github-actions github-actions bot added platform/backend AutoGPT Platform - Back end size/m labels Oct 24, 2024
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Potential Race Condition
The use of asyncio.to_thread for canceling execution might introduce a race condition if the cancellation is not handled properly in the execution manager.

Error Handling
The execute_graph method has been changed from async to sync, but there's no error handling for potential exceptions that might occur during execution.

Async/Sync Mismatch
The execute_graph call has been changed from async to sync, but it's still being used in an async context. This might lead to unexpected behavior.

@majdyz majdyz requested a review from Swiftyos October 25, 2024 06:37
@majdyz majdyz changed the title fix(backend): Avoid long synchronous call to block FatAPI event-loop fix(backend): Avoid long synchronous call to block FastAPI event-loop Oct 25, 2024
@majdyz majdyz requested a review from ntindle October 25, 2024 13:48
@majdyz majdyz merged commit 8938209 into dev Oct 27, 2024
7 checks passed
@majdyz majdyz deleted the zamilmajdy/avoid-sync-call-blocking-event-loop branch October 27, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants