1 - Modular : Sub-Packages - #69
Conversation
Reviewer's GuideThis PR modularizes MQTT and sub-package integration, adds a compatibility shim for api throttling, and introduces CI + pixi tasks to validate compatibility of new sub-packages while preserving existing interfaces. Sequence diagram for updated MQTT gateway startupsequenceDiagram
actor User
participant HummingbotApplication
participant mqtt_command
participant RemoteIface
participant Broker
User->>HummingbotApplication: start_mqtt_bridge
HummingbotApplication->>mqtt_command: start_mqtt_async(timeout)
mqtt_command->>mqtt_command: build BrokerConfig
mqtt_command->>mqtt_command: build GatewayConfig
mqtt_command->>RemoteIface: create_gateway(self, config, broker_config)
RemoteIface-->>mqtt_command: gateway
alt [connection successful]
mqtt_command->>gateway: start
mqtt_command->>HummingbotApplication: set _mqtt = gateway
else [connection failed]
mqtt_command->>gateway: stop
mqtt_command->>HummingbotApplication: set _mqtt = None
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the new MQTT wiring (e.g., external_events_example and ai_livestream), consider centralizing the
HummingbotApplication.main_application()/_mqttlookup in a helper to avoid repeated inline imports and reduce the risk of inconsistent null handling across different components. - The compatibility shim in
hummingbot/core/api_throttler/__init__.pyrelies onsys.modulesmutation; it would be safer to add a brief comment or guard explaining when this is expected to run to avoid surprising behavior if the module is imported beforeweb_assistantis available. - In
ci/check_subpackage_compat.py, the mode handling viasys.argv[1]could be made more robust (e.g., using argparse and validating allowed modes) to prevent confusing failures when the script is invoked with unexpected or missing arguments in CI.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new MQTT wiring (e.g., external_events_example and ai_livestream), consider centralizing the `HummingbotApplication.main_application()`/`_mqtt` lookup in a helper to avoid repeated inline imports and reduce the risk of inconsistent null handling across different components.
- The compatibility shim in `hummingbot/core/api_throttler/__init__.py` relies on `sys.modules` mutation; it would be safer to add a brief comment or guard explaining when this is expected to run to avoid surprising behavior if the module is imported before `web_assistant` is available.
- In `ci/check_subpackage_compat.py`, the mode handling via `sys.argv[1]` could be made more robust (e.g., using argparse and validating allowed modes) to prevent confusing failures when the script is invoked with unexpected or missing arguments in CI.
## Individual Comments
### Comment 1
<location path="ci/check_subpackage_compat.py" line_range="93-98" />
<code_context>
+ return failures
+
+
+def check_contracts() -> list[str]:
+ """Verify expected public API classes have required methods/attributes."""
+ failures = []
+
+ # --- CandlesFeed contract ---
+ from candles_feed.core.candles_feed import CandlesFeed
+
+ required_methods = {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Direct imports in `check_contracts` will raise unhandled ImportErrors instead of producing structured failure messages.
These direct imports in `check_contracts` aren’t guarded, so a missing or misinstalled `candles_feed` will crash the script instead of being reported as a structured failure. Consider matching `check_imports`: wrap the imports in `try/except ImportError`, record a clear failure message, and return normally so the summary and exit code capture the issue without a stack trace.
Suggested implementation:
```python
# --- CandlesFeed contract ---
try:
from candles_feed.core.candles_feed import CandlesFeed
except ImportError as e:
failures.append(
f"FAIL: Cannot import CandlesFeed from candles_feed.core.candles_feed: {e}"
)
return failures
```
If more contract checks are added later that also perform imports inside `check_contracts`, they should follow the same pattern: wrap each import in `try/except ImportError`, append a structured failure message, and either `return failures` immediately or continue depending on whether a missing import should block subsequent checks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def check_contracts() -> list[str]: | ||
| """Verify expected public API classes have required methods/attributes.""" | ||
| failures = [] | ||
|
|
||
| # --- CandlesFeed contract --- | ||
| from candles_feed.core.candles_feed import CandlesFeed |
There was a problem hiding this comment.
suggestion (bug_risk): Direct imports in check_contracts will raise unhandled ImportErrors instead of producing structured failure messages.
These direct imports in check_contracts aren’t guarded, so a missing or misinstalled candles_feed will crash the script instead of being reported as a structured failure. Consider matching check_imports: wrap the imports in try/except ImportError, record a clear failure message, and return normally so the summary and exit code capture the issue without a stack trace.
Suggested implementation:
# --- CandlesFeed contract ---
try:
from candles_feed.core.candles_feed import CandlesFeed
except ImportError as e:
failures.append(
f"FAIL: Cannot import CandlesFeed from candles_feed.core.candles_feed: {e}"
)
return failuresIf more contract checks are added later that also perform imports inside check_contracts, they should follow the same pattern: wrap each import in try/except ImportError, append a structured failure message, and either return failures immediately or continue depending on whether a missing import should block subsequent checks.
9ba32cc to
0e2f802
Compare
da2fada to
0e97435
Compare
7956675 to
8684471
Compare
7dc4589 to
e317919
Compare
d44c8a2 to
357bd84
Compare
a6d5e5c to
87437fb
Compare
hb_compat moves merged (hb-market-data #19, hb-candles-feed #69, hb-market-connector #37); remove the boundary-debt crutches: - market-data-no-hummingbot: strict whole-package -> carve-out on market_data.live_market_data + allow_indirect_imports. - candles-feed-hb-isolation: drop 4 stale ignore_imports for hummingbot_network_client_adapter (now in candles_feed/hb_compat/). - add market-connector-live-market-access contract (allow_indirect) now that live_market_access routes OrderType/TradeType via hb_compat. Co-lands with the gitlink bumps on _for_bleed/modular-wiring.
e24eef7 to
a26e8eb
Compare
1c4f442 to
a1c294d
Compare
…nted DataProcessor, Phase 2) Lands the augmented-pure-python DataProcessor conversion (hb-candles-feed PR hummingbot#75, closes hummingbot#74) into the modular tier so it propagates to bleeding-edge/accelerated. The augmented module is passive (valid pure Python, zero behaviour change); the adapter->hb_compat move was already at the prior pin (a9d8743a, #69), so this bump is import-boundary-neutral. Delta also carries candles-feed CI-framework bumps (#70/#71/hummingbot#72) and a dependabot aiohttp requirement update (hummingbot#73). Prereq for Phase 3 (compile-on-accelerated needs the augmented source present). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
244e839 to
9129d8c
Compare
2b34653 to
6a8a23e
Compare
5c40d61 to
d173d7c
Compare
…iew of the ci-base->modular delta (sub-package wiring). CI state preserved so the PR surfaces gate failures. Regenerated each rebuild; not a hand-merge target.
…iew of the ci-base->modular delta (sub-package wiring). CI state preserved so the PR surfaces gate failures. Regenerated each rebuild; not a hand-merge target.
Before submitting this PR, please make sure:
A description of the changes proposed in the pull request:
Tests performed by the developer:
Tips for QA testing:
Summary by Sourcery
Introduce modular sub-package wiring and compatibility checks while updating MQTT integration and maintained external event handling behavior.
New Features:
Enhancements:
CI:
Tests:
Chores: