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

refactor: clear separation of public and internal packages #39

Closed

Conversation

lifeizhou-ap
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap commented Sep 4, 2024

Glossary

  • Library: A distributed Python package (e.g., goose-ai).
  • Package: A package within a Python codebase.

Current State

goose-ai serves as:

  • A command-line application.
  • A plugin host that discovers and loads plugins.
  • A library that plugins depend on, providing base classes for creating toolkits, profiles, and notifiers.

Plugins depend on goose-ai during compile time to extend base classes such as Toolkit. However, at runtime, goose-ai dynamically loads the plugin’s functions or classes, creating an implicit dependency on the plugin.

Problem

Currently, goose-ai does not clearly distinguish between publicly accessible classes/functions and those intended for internal use. This lack of separation can lead to unintended use by plugin developers, potentially causing circular dependency issues.

Example:

  • Module A in goose-ai has two functions: load_plugin and goose_ai_util.
  • Module B in a plugin has a function render_toolkit, which references goose_ai_util.

When load_plugin in Module A calls render_toolkit to load an additional toolkit, render_toolkit in turn calls goose_ai_util in Module A. This can trigger a circular dependency problem.

Solution

To resolve this issue, restructure the goose-ai codebase to clearly delineate:

  • Packages that are publicly accessible to library users (e.g., plugins).
  • Packages that are used internally to implement goose-ai as a command-line application and plugin host.

Benefits

Maintainability

Encapsulating internal packages and implementation details makes it easier to refactor or modify goose-ai without affecting the users of the library. This ensures backward compatibility and reduces the risk of breaking changes.

Clear Interface to Prevent Misuse

Plugins will have a well-defined interface for accessing goose-ai functions and classes. This reduces the likelihood of plugin developers inadvertently using internal functions or classes, helping to prevent circular dependency problems.

How

In the src/goose directory, create three distinct packages:

  1. _internal:

    • This package is for internal use only and is responsible for:
      • Building the command-line application.
      • Discovering and loading plugins.
  2. pluginbase: (open to a better name)

    • This package is publicly accessible and contains:
      • Classes for plugins to extend.
      • Utilities to be shared with plugins.
  3. config:

    • This package is publicly accessible and contains configuration constants, such as:
      • Session file location and file extensions.
      • Profile configuration file location.

_internal can reference the pluginbase and config. But pluginbase and config cannot reference _internal, we may set some check to prevent this.

By organizing the codebase in this manner, goose-ai can offer a clear and stable API to its users while safeguarding against issues arising from the unintentional misuse of internal components.

Folder structures

Screenshot 2024-09-05 at 2 34 21 PM
Screenshot 2024-09-05 at 2 34 36 PM

…nctions

* main:
  Apply ruff and add to CI (#40)
  added some regex based checks for dangerous commands (#38)
  chore: Update publish github workflow to check package versions before publishing (#19)
…nctions

* main:
  adding in ability to provide per repo hints (#32)
@lifeizhou-ap lifeizhou-ap changed the title Lifei/refactor distinguish private public functions refactor: distinguish private public functions Sep 5, 2024
@lifeizhou-ap lifeizhou-ap changed the title refactor: distinguish private public functions refactor: clear Separation of Public and Internal Packages Sep 5, 2024
@lifeizhou-ap lifeizhou-ap changed the title refactor: clear Separation of Public and Internal Packages refactor: clear separation of public and internal packages Sep 5, 2024
RESUME_MESSAGE = "I see we were interrupted. How can I help you?"


def load_provider() -> str:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deleted this this function as it is not used

return read_config()[name]


class SessionNotifier(Notifier):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved this class in a separate file

return "openai"


def load_profile(name: Optional[str]) -> Profile:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved this function in the _internal.profile package

@@ -0,0 +1,18 @@
from .developer.developer import Developer # noqa: F401
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

use relative imports to shorten the import statement

@lifeizhou-ap lifeizhou-ap marked this pull request as ready for review September 5, 2024 05:01
@baxen
Copy link
Collaborator

baxen commented Sep 9, 2024

Hey @lifeizhou-ap! Absolutely agree with your categorization of the three things this package is providing, and we should clarify those throughout the code.

However I do consider the majority of the functionality to be "public", and so would avoid the refactor of pushing so much of the content into an _internal package. I'd instead at this point go function by function, or possibly in the future consider putting the leading underscore on a few selected files.

Some examples of why I think these are more public than they might seem:

  • build_exchange is intended to be used in other contexts such as developing an editor extension
  • Session (from the CLI) is exposed to make it possible to implement extensions to the CLI, such as a tutorial mode
  • Developer toolkit could be inherited from to add new functionality in downstream plugins

So i'd suggest we revisit this to highlight some of the private methods, especially any place you're running into circular imports!

@baxen baxen closed this Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants