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(rnd): Split Execution Manager #8008

Merged
merged 23 commits into from
Sep 10, 2024
Merged

Conversation

aarushik93
Copy link
Contributor

@aarushik93 aarushik93 commented Sep 6, 2024

Background

Running execution manager in a different pod to rest server, so we can scale independently as and when needed.

Changes 🏗️

To allow for this, updated to explicitly decide which ports to run the daemon's on (pyro choose its own dynamically).
To allow for service discovery, in three different environments:

locally, docker compose and k8s, we need a specific host (docker compose and k8s do their own service discovery and mapping based on service names), so introduced an environment variable to specify the host name (for local environments it's localhost, for k8s its the svc internal fully qualified domain name, for docker compose it's the service name).

Ports being used by the different services:

Execution Manager Daemon: 8002
Execution Scheduler Daemon: 8003
Rest Server Daemon: 8004

PR Quality Scorecard ✨

  • Have you used the PR description template?   +2 pts
  • Is your pull request atomic, focusing on a single change?   +5 pts
  • Have you linked the GitHub issue(s) that this PR addresses?   +5 pts
  • Have you documented your changes clearly and comprehensively?   +5 pts
  • Have you changed or added a feature?   -4 pts
    • Have you added/updated corresponding documentation?   +4 pts
    • Have you added/updated corresponding integration tests?   +5 pts
  • Have you changed the behavior of AutoGPT?   -5 pts
    • Have you also run agbenchmark to verify that these changes do not regress performance?   +10 pts

Copy link
Contributor

github-actions bot commented Sep 6, 2024

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 conflicts Automatically applied to PRs with merge conflicts platform/frontend AutoGPT Platform - Front end platform/backend AutoGPT Platform - Back end size/xl and removed conflicts Automatically applied to PRs with merge conflicts labels Sep 6, 2024
Copy link

netlify bot commented Sep 6, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit f8f6bbc
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/66e0071f7d85a90008ecbeba

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Sep 6, 2024
Copy link
Contributor

github-actions bot commented Sep 6, 2024

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

Copy link
Contributor

github-actions bot commented Sep 6, 2024

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

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Sep 6, 2024
@aarushik93 aarushik93 changed the title feat(rnd): Use direct URI feat(rnd): Split Execution Manager Sep 6, 2024
Copy link
Contributor

github-actions bot commented Sep 6, 2024

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 Sep 6, 2024
@aarushik93 aarushik93 force-pushed the aarushikansal/execution-manager branch from 98bf9bc to 2ce1717 Compare September 6, 2024 15:06
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Sep 6, 2024
Copy link
Contributor

github-actions bot commented Sep 6, 2024

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

@aarushik93 aarushik93 requested review from ntindle and Pwuts September 6, 2024 15:08
@aarushik93 aarushik93 marked this pull request as ready for review September 6, 2024 15:10
@aarushik93 aarushik93 requested a review from a team as a code owner September 6, 2024 15:11
@github-actions github-actions bot added the size/l label Sep 9, 2024
Swiftyos
Swiftyos previously approved these changes Sep 9, 2024
Swiftyos
Swiftyos previously approved these changes Sep 9, 2024
Pwuts
Pwuts previously approved these changes Sep 9, 2024
Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

Two (partially) unresolved items above. Otherwise good to go!

@aarushik93 aarushik93 dismissed stale reviews from Pwuts and Swiftyos via 9f2eaeb September 9, 2024 15:46
Swiftyos
Swiftyos previously approved these changes Sep 9, 2024
majdyz
majdyz previously approved these changes Sep 9, 2024
rnd/autogpt_server/autogpt_server/rest.py Outdated Show resolved Hide resolved
rnd/autogpt_server/pyproject.toml Outdated Show resolved Hide resolved
rnd/autogpt_server/test/util/test_service.py Show resolved Hide resolved
@aarushik93 aarushik93 requested a review from majdyz September 9, 2024 18:55
Comment on lines +187 to +192
By default the daemons run on the following ports:

Execution Manager Daemon: 8002
Execution Scheduler Daemon: 8003
Rest Server Daemon: 8004

Copy link
Member

Choose a reason for hiding this comment

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

Add this to docs/server too

@aarushik93 aarushik93 merged commit 0b91952 into master Sep 10, 2024
16 checks passed
@aarushik93 aarushik93 deleted the aarushikansal/execution-manager branch September 10, 2024 09:05
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.

6 participants