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

Standardize Strategy configuration pattern #2114

Closed
3 tasks done
stefansimik opened this issue Dec 14, 2024 · 4 comments
Closed
3 tasks done

Standardize Strategy configuration pattern #2114

stefansimik opened this issue Dec 14, 2024 · 4 comments
Assignees
Labels
improvement Improvement to existing functionality

Comments

@stefansimik
Copy link
Contributor

stefansimik commented Dec 14, 2024

Background

Currently, most strategy examples copy config values into strategy attributes:

class TimeBasedStrategy(Strategy):
    def __init__(self, config: TimeBasedStrategyConfig):
        self.primary_bar_type = config.primary_bar_type   # HERE: Copying config data (in next 3 lines)
        self.trade_size = config.trade_size
        self.timezone = config.timezone
    
    def on_start(self):
        self.subscribe_bars(self.primary_bar_type)        # HERE: referencing copied attribute of strategy 

However, using the config object directly provides better separation of concerns:

class TimeBasedStrategy(Strategy):
    def __init__(self, config: TimeBasedStrategyConfig):
        self.config = config

    def on_start(self):
        self.subscribe_bars(self.config.primary_bar_type)  # HERE - using `config` directly

While both approaches are valid, standardizing on direct config usage will improve code clarity and maintainability.

Proposal

Standardize on using direct config usage pattern (self.config) in examples and documentation because it:

  1. Clearly separates input configuration from other state-keeping variables Strategy
  2. Reduces code duplication
  3. Aligns with upcoming Rust implementation
  4. Makes it clearer where configuration values originate
  5. Results in cleaner, more maintainable code

Implementation Tasks

  • Update all example strategies to use direct config pattern
  • Update documentation to reflect this pattern
  • Add note clarifying that config isn't required (attributes can still be assigned directly)
@stefansimik stefansimik added the enhancement New feature or request label Dec 14, 2024
@cjdsellers
Copy link
Member

Hi @stefansimik

Thanks for raising the issue.

Per the Discord discussion, I think this is a cleaner approach and will standardize with the Rust side.

One refinement is that the config attribute from the Actor base class can be used through self.config. This is assigned from the config passed in during init super().__init__(config).

Many thanks for your help on this 🙏.

@cjdsellers cjdsellers moved this to In progress in NautilusTrader Kanban Board Dec 14, 2024
@cjdsellers cjdsellers changed the title Standardize Strategy Configuration Pattern Standardize Strategy configuration pattern Dec 15, 2024
@cjdsellers cjdsellers added improvement Improvement to existing functionality and removed enhancement New feature or request labels Dec 15, 2024
@stefansimik
Copy link
Contributor Author

Hi @cjdsellers
ready to send PR.

Doing it for the first time here (and after longer time of not sending any PR on github,
so just to make sure everything goes smoothly.... any recommendations?

This is just one-time question, next time I will know :-)

@cjdsellers
Copy link
Member

Hi @stefansimik

Using develop as the base branch is fine. Yep, having a separate branch you're making the PR from is also the correct way to do it.

@cjdsellers
Copy link
Member

Apologies, I should have mentioned the pre-commit. Details in the CONTRIBUTING.md or maybe an environment setup doc.

@cjdsellers cjdsellers moved this from In progress to Done in NautilusTrader Kanban Board Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement to existing functionality
Projects
Development

No branches or pull requests

2 participants