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

feat(backend): RedisEventQueue into Pub/Sub #8387

Merged
merged 8 commits into from
Oct 27, 2024

Conversation

majdyz
Copy link
Contributor

@majdyz majdyz commented Oct 21, 2024

Background

There are two issues with the current Redis event queue implementation:

  1. It's a queue, which means there can only be 1 event subscriber.
  2. It's implemented in a suboptimal way on ws_api:
    • The subscription loop lives in a fast API task (which is supposed to be async).
    • The task is implemented with a continuous sync loop with sleep.

Changes 🏗️

  • Converted RedisEventQueue --> RedisEventBus, and provided two versions, the async & sync version.
  • The WebSocket service / fast API will use the async version for subscribing to the event update.
  • The ExecutionManager will use the sync version for publishing the events.

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 21, 2024 16:07
@majdyz majdyz requested a review from a team as a code owner October 21, 2024 16:07
@majdyz majdyz requested review from Bentlybro and removed request for a team October 21, 2024 16:07
@github-actions github-actions bot added platform/backend AutoGPT Platform - Back end size/l labels Oct 21, 2024
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The Redis connection details (host, port, password) are being loaded from environment variables. Ensure these are properly secured and not exposed in logs or error messages.

⚡ Recommended focus areas for review

Error Handling
The error handling in the _deserialize_message method could be improved. It currently catches all exceptions, which might mask specific issues.

Connection Management
The connection management for both synchronous and asynchronous Redis connections could be refactored to reduce code duplication.

Exception Handling
The event_broadcaster function catches all exceptions. Consider handling specific exceptions separately for better error reporting and recovery.

@majdyz majdyz requested a review from a team October 21, 2024 16:11
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Oct 22, 2024
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Oct 23, 2024
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@majdyz majdyz requested a review from Pwuts October 23, 2024 04:46
@majdyz majdyz requested review from ntindle, Swiftyos and Pwuts and removed request for Bentlybro and Pwuts October 24, 2024 14:05
@majdyz majdyz merged commit 1e620fd into dev Oct 27, 2024
7 checks passed
@majdyz majdyz deleted the zamilmajdy/secrt-957-rediseventqueue-into-pubsub branch October 27, 2024 16:49
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.

3 participants