resolve subscriber#1000
Conversation
📝 WalkthroughWalkthroughThe changes enhance subscriber resolution logic for order and trade event dispatching. A helper function in the broker module enables event dispatch when strategy context is missing by defaulting to a sole subscriber. The Tradier broker integration extends this with strategy name computation and backfilling logic during order reconciliation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.5)lumibot/brokers/tradier.py************* Module pylintrc ... [truncated 79790 characters] ... ] lumibot/brokers/broker.py************* Module pylintrc ... [truncated 116424 characters] ... ] Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lumibot/brokers/broker.py (1)
2298-2302:⚠️ Potential issue | 🔴 CriticalUse correct pattern for colored error logging:
colored()wraps the message, not passed as kwarg toerror().Line 2302 passes
color=as a keyword argument toerror(), but this parameter is silently ignored by the standard logger. The color formatting is ineffective, and the pattern is inconsistent with the correct usage throughout the codebase (e.g., line 2310 in the same file).Proposed fix
- self.logger.error(f"Subscriber {order.strategy} not found", color="red") + self.logger.error(colored(f"Subscriber {order.strategy} not found", "red"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lumibot/brokers/broker.py` around lines 2298 - 2302, The logger.error call is incorrectly passing color="red" as a kwarg (which standard logging ignores); update the else branch where self._resolve_subscriber(order.strategy) returns None to wrap the message with colored(...) instead of passing color=, e.g. build the string "Subscriber {order.strategy} not found" using colored(...) and pass that single formatted string to self.logger.error; ensure you reference the same symbols (self._resolve_subscriber, order.strategy, self.logger.error) and remove the color= keyword usage to match the pattern used elsewhere (e.g., the file's other logging lines).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lumibot/brokers/tradier.py`:
- Around line 1311-1315: Add regression tests and brief documentation/comments
for the new strategy-resolution behavior around _strategy_name and _subscribers:
write unit tests that exercise the code path that sets strategy_name from
self._subscribers when _strategy_name is falsy and len(self._subscribers)==1,
tests that validate tag-to-subscriber repair (including normalization of
underscores/non-alphanumeric chars) and tests that verify unresolved tags fall
back to the sole subscriber; add these tests invoking the method that consumes
raw_orders and calls _parse_broker_order_dict (use the same inputs used by the
existing polling code) and assert the resolved strategy_name and subscriber
attribution; update docs/ with a short note about the invariants and add an
inline comment near the resolution logic (the block referencing _strategy_name,
_subscribers, and _parse_broker_order_dict and the tag-repair code around lines
~1341-1354) explaining the why/invariants for future readers.
---
Outside diff comments:
In `@lumibot/brokers/broker.py`:
- Around line 2298-2302: The logger.error call is incorrectly passing
color="red" as a kwarg (which standard logging ignores); update the else branch
where self._resolve_subscriber(order.strategy) returns None to wrap the message
with colored(...) instead of passing color=, e.g. build the string "Subscriber
{order.strategy} not found" using colored(...) and pass that single formatted
string to self.logger.error; ensure you reference the same symbols
(self._resolve_subscriber, order.strategy, self.logger.error) and remove the
color= keyword usage to match the pattern used elsewhere (e.g., the file's other
logging lines).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad4646bc-410a-43da-bbd9-8aa296188b36
📒 Files selected for processing (2)
lumibot/brokers/broker.pylumibot/brokers/tradier.py
| strategy_name = self._strategy_name | ||
| if not strategy_name and len(self._subscribers) == 1: | ||
| strategy_name = self._subscribers[0].name | ||
| for order_row in raw_orders: | ||
| order = self._parse_broker_order_dict(order_row, strategy_name=self._strategy_name) | ||
| order = self._parse_broker_order_dict(order_row, strategy_name=strategy_name) |
There was a problem hiding this comment.
Add regression tests for the new strategy-resolution behavior.
This changes core polling attribution behavior (sole-subscriber fallback + tag-based strategy repair), but there’s no accompanying regression coverage in the provided changes. Please add tests covering at least:
- missing
strategy_namewith exactly one subscriber, - tag-to-subscriber repair (including underscore/non-alnum normalization),
- unresolved tag fallback to sole subscriber.
As per coding guidelines "Document and test behavioral changes: update relevant docs in docs/ and add regression tests in the same commit; add comments explaining 'why/invariants' for non-obvious logic."
Also applies to: 1341-1354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lumibot/brokers/tradier.py` around lines 1311 - 1315, Add regression tests
and brief documentation/comments for the new strategy-resolution behavior around
_strategy_name and _subscribers: write unit tests that exercise the code path
that sets strategy_name from self._subscribers when _strategy_name is falsy and
len(self._subscribers)==1, tests that validate tag-to-subscriber repair
(including normalization of underscores/non-alphanumeric chars) and tests that
verify unresolved tags fall back to the sole subscriber; add these tests
invoking the method that consumes raw_orders and calls _parse_broker_order_dict
(use the same inputs used by the existing polling code) and assert the resolved
strategy_name and subscriber attribution; update docs/ with a short note about
the invariants and add an inline comment near the resolution logic (the block
referencing _strategy_name, _subscribers, and _parse_broker_order_dict and the
tag-repair code around lines ~1341-1354) explaining the why/invariants for
future readers.
1 participant
This makes a few fixes to prevent "subscriber not found" errors. The Tradier broker polls Tradier every 5 seconds. We can get into situations where we poll before the strategy's name is added as a subscriber. These fixes attempt to prevent that.
Summary by CodeRabbit