fix(pmm_mister): declare candles_config and use .get() for open_order_last_update#147
Closed
gordonkoehn wants to merge 2 commits intohummingbot:mainfrom
Closed
fix(pmm_mister): declare candles_config and use .get() for open_order_last_update#147gordonkoehn wants to merge 2 commits intohummingbot:mainfrom
gordonkoehn wants to merge 2 commits intohummingbot:mainfrom
Conversation
…gine
PMMisterConfig is the only controller config under bots/controllers/generic/
that doesn't declare `candles_config: List[CandlesConfig] = []`. Every
sibling (pmm.py, pmm_adjusted.py, pmm_dynamic.py, quantum_grid_allocator.py,
etc.) declares it.
The HB backtest engine accesses `self.controller.config.candles_config`
unconditionally in BacktestingEngineBase.initialize_backtesting_data_provider
(hummingbot/strategy_v2/backtesting/backtesting_engine_base.py ~L117), so
calling POST /backtesting/run-backtesting with any pmm_mister config crashes
with:
'PMMisterConfig' object has no attribute 'candles_config'
Attempting to pass `candles_config: []` in the request body is rejected by
Pydantic's `extra="forbid"` — the model has to declare the field itself.
This patch mirrors the exact placement used by pmm.py:23.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
custom_info["open_order_last_update"] is populated lazily by the executor lifecycle; a fresh executor that hasn't yet placed its open order can enter should_effectivize_executor without the key set. The existing `if not fill_time: return False` fallback three lines below is clearly intended to handle this case, but the direct dict access raises KeyError before the fallback can run. The other two sites reading this same key (lines 623-624, 1118-1119 in this file) already use .get() defensively — this aligns line 225 with that convention and makes the function backtest-safe without changing observable live-trading behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
Closing to self-review on my fork first: gordonkoehn#1 — will reopen here once vetted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two small, independent defects in
bots/controllers/generic/pmm_mister.pythat preventPOST /backtesting/run-backtestingfrom running on anypmm_misterconfig.Bug 1 —
PMMisterConfigmissingcandles_configThe backtest engine (
backtesting_engine_base.py~L117) readsself.controller.config.candles_configunconditionally. Every other controller underbots/controllers/generic/declares the field;pmm_mister.pydoes not, so backtest fails with:Injecting it in the request payload is rejected by
extra="forbid". Fix mirrorspmm.py:23:Repro (pure Python, no network)
Save as
repro_bug1.pyin repo root:On
main:AttributeError: 'PMMisterConfig' object has no attribute 'candles_config'. On this branch:OK: candles_config = [].Bug 2 —
KeyErroronopen_order_last_updateat line 225should_effectivize_executoruses indexed access where the other three sites reading the same key in this same file (L624, L1119, L1210) use.get():def should_effectivize_executor(self, executor_info, current_time: int) -> bool: level_id = executor_info.custom_info.get("level_id", "") - fill_time = executor_info.custom_info["open_order_last_update"] + fill_time = executor_info.custom_info.get("open_order_last_update") if not level_id or not fill_time: return FalseThe
if not fill_time: return Falsefallback three lines below is clearly meant to handle the absent key, but never runs because the previous line has already raised. Live execution usually populates the key before this method is called; the simulated executor lifecycle calls it earlier, which surfaces the issue in practice.Repro (pure Python)
On
main:KeyError: 'open_order_last_update'. On this branch:OK: returned False.Test plan
mainwith the exact expected error.POST /backtesting/run-backtestingon a realpmm_misterconfig + 44h window returns HTTP 200 on this branch; crashes onmainwith the two errors above.