[SW eng] Proposal: Abstract common patterns between predictoor/trader into base_agent

[idiom-bytes]

NOTE: do not build this until we have agreement. Currently, we don't.

Problem

• There are many common patterns between base_predictoor and base_trader
• There is a good chunk of code duplication between these two
• Details like -> tuple being returned from _process_block_at_feed are starting to diverge
• Caching, clear caching, saving, and not engaging multiple times on an epoch only happens with Trader but not Predictoor agent
• run/take-step/etc... are all common functions as part of base_agent

DoD

• BasePredictoorAgent and BaseTraderAgent extend from BaseAgent
• Improved systems and quality by abstracting common patterns into BaseAgent
• Unique functions such as trade() and predict() should be perhaps moved into take_action(), and called from _process_block_at_feed()

[trentmc]

We should avoid heavy inheritance chains. They are extremely hard to maintain, because to understand a given class you need to also understand all the details of its parent clasess.

Much better is composability: have well-defined classes / objects that can be used as attributes by client agents. These client agents only need to interact with the interface of the composable agents.

I had thought about having a BaseAgent. But I explicitly chose not to, because it would become a unwieldy inheritance chain: BaseAgent <- BasePredictoorAgent <- PredictoorAgent3; and BaseAgent <- [Base]TraderAgent <- TraderAgent1.

3 levels of inheritance hierachy? Yuck.

Instead, I explicitly went the composability route

• both BasePredictoorAgent and BaseTraderAgent have feeds, a dict of Feed
• both have contracts, a dict of contract
• and beyond that, they each can have whatever they want.

So please, let's not do this.

If you would like to revise this proposal without inheritance, go for it. But if any fancier inheritance remains, I will push back.

[idiom-bytes]

Hi @trentmc, thanks for providing more insight/perspectives.

I thought that might be the case and I generally agree, I'm just worried about the amount of duplication that's taking place and how certain functionality is fragmenting.

Example: Now part of traderAgents are logging while predictoors are printing. take_step, _process_block, cache fns, and various other fns are nearly 1:1 across all agents. Traders can take action async, while predictoor core functions are synchronous.

As an additional note, due to some of our CICD tools (i.e. pylint rules), the agentN classes are growing in a way that breaks various linter rules and forces us to silence lint rather than "clean up".

I believe this is making it harder to maintain the code base especially when working broadly and deeply across flows.

Maybe we can leave this here and see how the agent permutations evolve before revisiting.

[trentmc]

Let's revisit after I get farther down #278

[trentmc]

take_step, _process_block, cache fns, and various other fns are nearly 1:1 across all agents.

take_step is specific to each agent. There is nothing to share. Same with _process_block. Same with _cache functions.

If there is stuff that could be shared between predictoor and trader agents, we should do it as via composability (or via a mixin).

I'm closing this issue.