Skip to content

Conversation

@MementoRC
Copy link
Owner

@MementoRC MementoRC commented Feb 8, 2025

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:

Tests performed by the developer:

Tips for QA testing:

Summary by Sourcery

New Features:

  • Add an ExecutorFactory to create executors dynamically based on their configuration.

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 8, 2025

Reviewer's Guide by Sourcery

This PR refactors the executor instantiation process by introducing an ExecutorFactory, which leverages a registration decorator to map executor configuration types to their respective executor implementations, replacing the original if/else logic. It also updates the ExecutorBase constructor signature to support an optional connectors parameter.

Updated Class Diagram for Executor Factory and Executors

classDiagram
  class ExecutorFactory {
    - _registry: dict[Type[ExecutorConfigBase], Type[ExecutorBase]]
    + register_executor(config_type: Type[ExecutorConfigBase]) : decorator
    + create_executor(strategy: Any, config: ExecutorConfigBase, update_interval: float) : ExecutorBase
  }

  class ExecutorBase {
    + __init__(strategy: ScriptStrategyBase, config: ExecutorConfigBase, update_interval: float, connectors: List[str]?)
    + start() : void
  }

  class ArbitrageExecutor {
  }
  class DCAExecutor {
  }
  class GridExecutor {
  }
  class PositionExecutor {
  }
  class TWAPExecutor {
  }
  class XEMMExecutor {
  }

  ExecutorFactory ..> ExecutorBase : creates
  ExecutorBase <|-- ArbitrageExecutor
  ExecutorBase <|-- DCAExecutor
  ExecutorBase <|-- GridExecutor
  ExecutorBase <|-- PositionExecutor
  ExecutorBase <|-- TWAPExecutor
  ExecutorBase <|-- XEMMExecutor

  note for ExecutorFactory "Uses registration decorators to map ExecutorConfig types to Executor implementations."
Loading

File-Level Changes

Change Details Files
Refactor executor creation logic in the orchestrator.
  • Replaced the lengthy if/else chain with a call to ExecutorFactory.create_executor.
  • Simplified executor instantiation in the ExecutorOrchestrator class.
hummingbot/strategy_v2/executors/executor_orchestrator.py
Introduce ExecutorFactory for centralized executor instantiation.
  • Added a new ExecutorFactory class to encapsulate logic for creating executor instances based on configuration types.
  • Implemented a registration decorator to map ExecutorConfigBase types to ExecutorBase implementations.
  • Provided a create_executor method that validates and instantiates the appropriate executor.
hummingbot/strategy_v2/executors/executor_factory.py
Adopt decorator-based registration for executor classes.
  • Decorated executor classes (Arbitrage, DCA, Grid, Position, TWAP, and XEMM) with @ExecutorFactory.register_executor to register them with the ExecutorFactory.
  • Removed redundant creation logic within each executor class in favor of factory registration.
hummingbot/strategy_v2/executors/arbitrage_executor/arbitrage_executor.py
hummingbot/strategy_v2/executors/dca_executor/dca_executor.py
hummingbot/strategy_v2/executors/grid_executor/grid_executor.py
hummingbot/strategy_v2/executors/position_executor/position_executor.py
hummingbot/strategy_v2/executors/twap_executor/twap_executor.py
hummingbot/strategy_v2/executors/xemm_executor/xemm_executor.py
Update ExecutorBase constructor signature.
  • Modified the constructor to reorder parameters.
  • Introduced an optional connectors parameter to allow for more flexible initialization.
hummingbot/strategy_v2/executors/executor_base.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @MementoRC - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding a check in ExecutorFactory.register_executor to prevent accidental overwrites of an already registered executor for a given config type.
  • Ensure that the ordering and usage of parameters (like connectors) in ExecutorBase.init remain consistent across all executor implementations to avoid positional argument issues.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

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