Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Split forge.models.config into forge.models.config and forge.models.state #7000

Closed
Pwuts opened this issue Mar 11, 2024 · 9 comments
Closed
Labels
code quality ⬆️ PRs that improve code quality Forge Roadmapped Issues that were spawned by roadmap items Stale

Comments

@Pwuts
Copy link
Member

Pwuts commented Mar 11, 2024

  • Actionable for 🚀 AutoGPT Roadmap - Empowering Agent Builders 👷 #6970

    Move clear-cut library code from AutoGPT to Forge (or to /dev/null if Forge already has a better version)

    • ...
    • autogpt.core.configuration
    • ...
  • ⛔ Blocks [all other "Port ... to Forge" items]

  • Separation
    Currently, autogpt.core.configuration handles both configuration and state management. It makes sense to separate those functions into forge.config and forge.state. For example:

    # BEFORE
    class MyComponent(Configurable[MySettings]):
        default_settings = MySettings(
            configuration=MyConfig(
                enable_coolness=True
            ),
            some_stateful_stuff=MyPartialState(
                coolness_counter=0,
                max_coolness=math.inf,
            ),
        )
    
    # AFTER
    C = TypeVar("C", bound=SystemConfiguration)
    class Configurable(Generic[C]):
        config_type: ClassVar[type[C]]
        config: C
    
        @classmethod
        def _make_config(cls, **overrides) -> C:
            """
            Builds the configuration from the following sources (in order of precedence):
            1. Overrides
            2. Environment variables
            3. Default values on the config_type
            """
            # insert implementation here
    
        def __init__(self, **kwargs):
            self.config = self._make_config(**kwargs)
            super(Configurable, self).__init__(**kwargs)
    
    S = TypeVar("S", bound=SystemState)
    class Stateful(Generic[S]):
        _state_type: ClassVar[type[S]]
        _state: S
    
        def __init__(self, state: Optional[S] = None, **kwargs):
            self._state = state or self._state_type()
            super(Stateful, self).__init__(**kwargs)
    
        def export_state(self) -> S:
            return self._state
    
    class MyConfig(SystemConfiguration):
        enable_coolness: bool = UserConfigurable(default=True)
        max_coolness: int = math.inf
    
    class MyState(SystemState):
        coolness_counter: int = 0
    
    class Component(Configurable[MyConfig], Stateful[MyState]):
        def __init__(self):
            super(Component, self).__init__()
    
        def some_method(self):
            if self.config.enable_coolness:
                if self._state.coolness_counter + 1 > self.config.max_coolness:
                    raise ValueError("too cool!")
                self._state.coolness_counter += 1
    
    class AltComponent(Configurable[MyConfig], Stateful[MyState]):
        def __init__(self):
            super(AltComponent, self).__init__()
    
            self.counter = self._state.coolness_counter
    
        def some_method(self):
            if self.config.enable_coolness:
                if self.counter + 1 > self.config.max_coolness:
                    raise ValueError("too cool!")
                self.counter += 1
    
        def export_state(self) -> MyState:
            return MyState(coolness_counter=self.counter)

    Configurable+SystemConfiguration and Stateful+SystemState will share a large portion of their implementations, because both support the same kind of nesting and related functionality.

Background

Notes

  • In the future it may make sense to move to an externally imported config library. This is purposefully not included in this issue to keep the task manageable.
@Pwuts Pwuts added code quality ⬆️ PRs that improve code quality Forge Roadmapped Issues that were spawned by roadmap items labels Mar 11, 2024
Copy link
Contributor

github-actions bot commented May 1, 2024

This issue has automatically been marked as stale because it has not had any activity in the last 50 days. You can unstale it by commenting or removing the label. Otherwise, this issue will be closed in 10 days.

@github-actions github-actions bot added the Stale label May 1, 2024
Copy link
Contributor

This issue was closed automatically because it has been stale for 10 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 12, 2024
@ntindle
Copy link
Member

ntindle commented May 12, 2024

Unstale

@ntindle ntindle reopened this May 12, 2024
@github-actions github-actions bot removed the Stale label May 13, 2024
Copy link
Contributor

github-actions bot commented Jul 2, 2024

This issue has automatically been marked as stale because it has not had any activity in the last 50 days. You can unstale it by commenting or removing the label. Otherwise, this issue will be closed in 10 days.

@github-actions github-actions bot added the Stale label Jul 2, 2024
@Pwuts Pwuts changed the title Port and split autogpt.core.configuration into forge.config and forge.state Port and split forge.models.config into forge.models.config and forge.models.state Jul 2, 2024
@Pwuts
Copy link
Member Author

Pwuts commented Jul 2, 2024

Partially implemented in #7106

@Pwuts
Copy link
Member Author

Pwuts commented Jul 2, 2024

@kcze do you think splitting is useful/necessary? Otherwise we can close this.

@Pwuts Pwuts changed the title Port and split forge.models.config into forge.models.config and forge.models.state Split forge.models.config into forge.models.config and forge.models.state Jul 2, 2024
@github-actions github-actions bot removed the Stale label Jul 3, 2024
@kcze
Copy link
Contributor

kcze commented Jul 3, 2024

@kcze do you think splitting is useful/necessary? Otherwise we can close this.

I think it's useful, config.py is quite large.
We could rename *Settings to *State in the same PR as well.

Copy link
Contributor

This issue has automatically been marked as stale because it has not had any activity in the last 50 days. You can unstale it by commenting or removing the label. Otherwise, this issue will be closed in 10 days.

@github-actions github-actions bot added the Stale label Aug 23, 2024
Copy link
Contributor

github-actions bot commented Sep 2, 2024

This issue was closed automatically because it has been stale for 10 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
code quality ⬆️ PRs that improve code quality Forge Roadmapped Issues that were spawned by roadmap items Stale
Projects
None yet
Development

No branches or pull requests

3 participants