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

Move all application code to an application subpackage #5026

Merged
merged 5 commits into from
Jul 21, 2023

Conversation

collijk
Copy link
Contributor

@collijk collijk commented Jul 20, 2023

Background

This is a redo of #4962 now that we've merged #4799. I've limited the scope here a bit more so that I'm not introducing any logic changes in this PR. Goals:

  • Move all application code (code with user interaction or code that is responsible for application logic like running the agent loop) into the autogpt.app subpackage
  • Make sure no code outside autogpt.app references code inside autogpt.app (The app depends on the library but not the other way around).
  • Make sure no code outside of autogpt.app references logger.typewriter_log (our defacto UI tool).

This is an almost complete success. The only code that imports from autogpt.app is test code (fine, we can segregate test suites later). There is one instance of an invocation of logger.typewriter_log in the creation of the default prompt generator which would have required much more in depth changes than moving files or functions between files, so I left it for a separate PR.

This doesn't make the app code clean. It's pretty much a disaster in there. But now it's a disaster all in one pile that we can carefully sort through.

Changes

  • autogpt.cli -> autogpt.app.cli
  • autogpt.configurator -> autogpt.app.configurator
  • autogpt.main -> autogpt.app.main
  • autogpt.main.COMMAND_CATEGORIES -> autogpt.commands.COMMAND_CATEGORIES
  • autogpt.llm.utils.check_model -> autogpt.app.configurator.check_model
  • autogpt.logs.utils.* -> autogpt.app.main.*
  • autogpt.prompts.prompt.construct_main_ai_config -> autogpt.app.main.construct_main_ai_config
  • Remove top level main.py
  • Remove top level tests.py

Documentation

X

Test Plan

CI + Manual testing

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.
  • I have run the following commands against my code to ensure it passes our linters:
    black .
    isort .
    mypy
    autoflake --remove-all-unused-imports --recursive --ignore-init-module-imports --ignore-pass-after-docstring autogpt tests --in-place

@netlify
Copy link

netlify bot commented Jul 20, 2023

Deploy Preview for auto-gpt-docs ready!

Name Link
🔨 Latest commit 4a887cb
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/64bad3cd2e4f7c0008c6e4d1
😎 Deploy Preview https://deploy-preview-5026--auto-gpt-docs.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@collijk collijk added this to the v0.4.6 Release milestone Jul 20, 2023
@collijk
Copy link
Contributor Author

collijk commented Jul 21, 2023

We are failing on rate limit errors :-(

@Pwuts
Copy link
Member

Pwuts commented Jul 21, 2023

We are failing on rate limit errors :-(

That would mean the PR introduces prompting changes, otherwise it would use the cassettes and not encounter this issue.

@collijk collijk added re-arch code quality ⬆️ PRs that improve code quality labels Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@8111770). Click here to learn what that means.
Patch coverage: 49.41% of modified lines in pull request are covered.

❗ Current head 3ce2805 differs from pull request most recent head 4a887cb. Consider uploading reports for the commit 4a887cb to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #5026   +/-   ##
=========================================
  Coverage          ?   51.03%           
=========================================
  Files             ?      119           
  Lines             ?     4961           
  Branches          ?      662           
=========================================
  Hits              ?     2532           
  Misses            ?     2234           
  Partials          ?      195           
Impacted Files Coverage Δ
autogpt/__main__.py 0.00% <0.00%> (ø)
autogpt/app/cli.py 0.00% <0.00%> (ø)
autogpt/app/setup.py 73.07% <ø> (ø)
autogpt/llm/utils/__init__.py 45.16% <ø> (ø)
autogpt/logs/__init__.py 100.00% <ø> (ø)
autogpt/prompts/prompt.py 100.00% <ø> (ø)
autogpt/app/main.py 39.74% <42.85%> (ø)
autogpt/app/configurator.py 37.64% <100.00%> (ø)
autogpt/commands/__init__.py 100.00% <100.00%> (ø)

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

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Jul 21, 2023
@github-actions
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 removed the conflicts Automatically applied to PRs with merge conflicts label Jul 21, 2023
@github-actions
Copy link
Contributor

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

@collijk collijk merged commit 669e66a into master Jul 21, 2023
@collijk collijk deleted the refactor/move-app-code-to-app-subpackage branch July 21, 2023 19:01
@collijk collijk mentioned this pull request Jul 21, 2023
6 tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think we moved the working_directory again when we moved this file one level down into the app directory

Line 132: working_directory=Path(file).parent.parent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality ⬆️ PRs that improve code quality re-arch size/l
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants