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): Spin-up Database manager on rest.py #8832

Conversation

majdyz
Copy link
Contributor

@majdyz majdyz commented Nov 27, 2024

AutoGPT-lib is used in both Rest API & Execution Manager, due to the interdependency of the module, some unexpected infra changes were inferred: DatabaseManager service is required in both RestAPI & ExecutionManager

The scope of this PR is to clear this out.

Changes 🏗️

  • Initiated DatabaseManager on RestAPI service.
  • Removed DATABASEMANAGER_HOST settings.

Out of scope:
Decouple DatabaseManager service from RestAPI service, and use direct async prisma query instead.

Checklist 📋

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • ...
Example test plan
  • 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

For configuration changes:

  • .env.example is updated or already compatible with my changes
  • docker-compose.yml is updated or already compatible with my changes
  • I have included a list of my configuration changes in the PR description (under Changes)
Examples of configuration changes
  • Changing ports
  • Adding new services that need to communicate with each other
  • Secrets or environment variable changes
  • New or infrastructure changes such as databases

@majdyz majdyz requested a review from a team as a code owner November 27, 2024 15:29
@majdyz majdyz requested review from Bentlybro and removed request for a team November 27, 2024 15:29
@github-actions github-actions bot added the platform/backend AutoGPT Platform - Back end label Nov 27, 2024
Copy link

netlify bot commented Nov 27, 2024

Deploy Preview for auto-gpt-docs-dev canceled.

Name Link
🔨 Latest commit 05ad996
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs-dev/deploys/67474a07d9082b0008afb622

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

Architecture Change
Adding DatabaseManager to rest.py changes the service architecture. Verify this doesn't create circular dependencies or race conditions between rest_server and executor services.

Lock Key Change
The lock key structure was simplified by removing db_manager. Verify this change doesn't break lock isolation between different database managers.

Copy link

netlify bot commented Nov 27, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit 05ad996
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/67474a0756267800086ae1a3

@majdyz majdyz enabled auto-merge (squash) November 27, 2024 15:31
@majdyz
Copy link
Contributor Author

majdyz commented Nov 27, 2024

@aarushik93 I decoupled changes of dbmanager from the rest of the change in #8751, to expedite this fix.

Copy link
Member

@ntindle ntindle left a comment

Choose a reason for hiding this comment

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

Some of these changes also match up with moving Supabase credentials provider to backend

@majdyz majdyz merged commit ae9bd87 into dev Nov 27, 2024
19 checks passed
@majdyz majdyz deleted the zamilmajdy/open-1980-update-rest-server-to-have-a-db-manager-process-origin branch November 27, 2024 16:39
@majdyz
Copy link
Contributor Author

majdyz commented Nov 27, 2024

@ntindle I think you are referring to #8751

@ntindle
Copy link
Member

ntindle commented Nov 27, 2024

No there's an open pr to move the credentials providers to the backed

@ntindle
Copy link
Member

ntindle commented Nov 27, 2024

#8648

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants