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

Have api manager use singleton pattern #3269

Conversation

collijk
Copy link
Contributor

@collijk collijk commented Apr 26, 2023

Background

The api manager came in a little hot and one of the things we missed was it's inconsistent method of being used as a singleton. The class was instantiated at the module level and then the instance was imported in other modules. This PR revises the singleton implementation to use tools standard in the package.

Changes

  • Make APIManager a subclass of AbstractSingleton
  • Remove the module level instance
  • Do a little cleanup of configuration usage (the debug stuff was well-intentioned but doing the job of the logging system)
  • Update manager usage across the rest of the package.

Documentation

N/A

Test Plan

Small CI updates, but otherwise this class is already covered.

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Patch coverage: 68.75% and project coverage change: -7.30 ⚠️

Comparison is base (4241fbb) 50.73% compared to head (bef6c1d) 43.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3269      +/-   ##
==========================================
- Coverage   50.73%   43.44%   -7.30%     
==========================================
  Files          65       65              
  Lines        2935     2935              
  Branches      493      492       -1     
==========================================
- Hits         1489     1275     -214     
- Misses       1326     1590     +264     
+ Partials      120       70      -50     
Impacted Files Coverage Δ
autogpt/memory/base.py 72.22% <ø> (-2.78%) ⬇️
autogpt/prompts/prompt.py 0.00% <0.00%> (-56.87%) ⬇️
autogpt/chat.py 22.78% <50.00%> (-41.32%) ⬇️
autogpt/llm_utils.py 45.87% <66.66%> (-12.08%) ⬇️
autogpt/api_manager.py 93.18% <88.88%> (+1.87%) ⬆️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vercel
Copy link

vercel bot commented Apr 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview Apr 26, 2023 4:34pm

@vercel vercel bot temporarily deployed to Preview April 26, 2023 15:58 Inactive
autogpt/api_manager.py Show resolved Hide resolved
@ntindle
Copy link
Member

ntindle commented Apr 26, 2023

Wait why does the debug flag exist at all? Can’t we pass —debug and logger handle it

@ntindle ntindle merged commit 0ff471a into Significant-Gravitas:master Apr 26, 2023
@collijk collijk deleted the bugfix/api-manager-use-singleton-pattern branch April 26, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants