Improve/backtesting storage - #205
Conversation
Covers the fix in 9d33038. A controller module imports its own base class, so the base is a member of the module namespace too, and every base except ControllerConfigBase is itself a strict subclass of it. Matching on "subclass of a base, but not that same base" therefore accepted the sibling bases, and since getmembers() sorts by name, whichever sorted first won -- resolving ema_trend_v1 to DirectionalTradingControllerConfigBase, whose extra="forbid" then rejected every controller-specific field as "Extra inputs are not permitted". Parametrised over both groups: the controllers whose config class sorts after its base (the ones that were broken) and the ones that sort before it (which worked by luck, and must not regress). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mory
A finished backtest is ~98% bulk arrays: processed_data and pnl_timeseries are 8.2 MB
of a 7.6 MB (gzipped 1.1 MB) 7-day run at 1m, while the metrics anyone reads back are
under 10 KB. Holding whole results in BacktestingService._tasks therefore rationed the
wrong resource. The flat 50-task cap was ~380 MB for 7-day runs but ~3.2 GB for 60-day
ones, and because reaping can only drop terminal tasks it did nothing during a
submission burst -- exactly when nothing has finished and memory climbs fastest.
Results are now written to bots/data/backtests/{task_id}.json.gz on completion and only
their metrics stay resident, so a task costs a few KB and retention is a count of
results rather than a memory budget. Reads rehydrate the full payload from disk, so the
HTTP response is byte-identical to before (json coerces processed_data's float keys to
strings exactly as FastAPI's encoder already did).
Three further consequences:
- Results outlive a restart. They were in-process only, so any API restart silently
destroyed every unread result; an index file now restores finished tasks on startup.
- A reaped task answers 410 rather than 404, which a caller could not distinguish from
a mistyped id.
- Reaping also runs on completion, not only on submit, so a burst submitted all at once
is still trimmed when no further submissions arrive.
Retention defaults to 100 results and is configurable via BACKTESTING_MAX_RESULTS /
BACKTESTING_RESULTS_PATH. The archive lives inside the bots volume, so it survives
container recreation. A failed archive write keeps the result in memory rather than
discarding it.
Also fixes a pre-existing E305 in config.py, surfaced now that the file is in the diff.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile SummaryThe PR moves completed backtest payloads into gzipped disk archives, restores their metadata across restarts, and adds configurable count-based retention.
Confidence Score: 4/5The PR needs fixes before merging because submission bursts can prematurely delete fresh results, storage failures can yield incomplete successful responses, and restarts lose the promised reaped-task status. The retention calculation counts running tasks as stored results, archive failures are converted into partial HTTP 200 payloads, and reaped identifiers are not included in the newly persistent state. Files Needing Attention: services/backtesting_service.py, test/test_backtest_storage.py
|
| Filename | Overview |
|---|---|
| config.py | Adds configurable backtest result-count retention and persistent archive path settings. |
| routers/backtesting.py | Switches task retrieval to archive-aware payload loading and distinguishes remembered reaped results with HTTP 410. |
| services/backtesting_service.py | Implements archive persistence, restoration, and retention, but burst retention, archive-read failures, and restart handling of reaped IDs violate the intended API contracts. |
| test/test_backtest_storage.py | Covers sequential retention and happy-path persistence but omits concurrent submission bursts, unreadable archives, and post-restart reaped-ID behavior. |
| test/test_controller_config_class_loading.py | Adds controller config-class regression coverage without changing production behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Submit backtest task] --> B[Run backtest]
B --> C{Execution succeeds?}
C -->|No| D[Persist failure metadata]
C -->|Yes| E[Write full result to gzip archive]
E --> F[Keep metrics in memory]
D --> G[Persist task index]
F --> G
G --> H[Apply retention]
I[GET task] --> J{Archived?}
J -->|Yes| K[Read gzip archive]
J -->|No| L[Use resident result]
K --> M[Return task payload]
L --> M
Reviews (1): Last reviewed commit: "(feat) archive backtest results to disk ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if len(self._tasks) <= self._max_results: | ||
| return | ||
| terminal = [(tid, t) for tid, t in self._tasks.items() if t.status in _TERMINAL] | ||
| terminal.sort(key=lambda kv: kv[1].completed_at or kv[1].created_at) | ||
| while len(self._tasks) > self._max_results and terminal: | ||
| task_id, _ = terminal.pop(0) | ||
| self._drop(task_id, reaped=True) |
There was a problem hiding this comment.
If more than max_results backtests are submitted before any finish, _reap compares the limit with all tasks, so the first completions are immediately deleted and polling clients receive 410 even though fewer than max_results completed results existed.
| if len(self._tasks) <= self._max_results: | |
| return | |
| terminal = [(tid, t) for tid, t in self._tasks.items() if t.status in _TERMINAL] | |
| terminal.sort(key=lambda kv: kv[1].completed_at or kv[1].created_at) | |
| while len(self._tasks) > self._max_results and terminal: | |
| task_id, _ = terminal.pop(0) | |
| self._drop(task_id, reaped=True) | |
| terminal = [(tid, t) for tid, t in self._tasks.items() if t.status in _TERMINAL] | |
| if len(terminal) <= self._max_results: | |
| return | |
| terminal.sort(key=lambda kv: kv[1].completed_at or kv[1].created_at) | |
| while len(terminal) > self._max_results: | |
| task_id, _ = terminal.pop(0) | |
| self._drop(task_id, reaped=True) |
| result = self._read_archive(task_id) if task.archived else None | ||
| if result is None: | ||
| result = task.result # never archived, or the archive went missing | ||
| if result is not None: | ||
| data["result"] = result |
There was a problem hiding this comment.
When a completed task's archive is missing or unreadable, get_task_payload falls back to the metrics-only resident value and returns HTTP 200 without executors, processed data, positions, or time series, causing consumers to receive an incomplete result instead of an explicit storage error.
| self._reaped[task_id] = datetime.now(timezone.utc).isoformat() | ||
| while len(self._reaped) > _REAPED_MEMORY: | ||
| self._reaped.popitem(last=False) |
There was a problem hiding this comment.
When the service restarts after a result is reaped, _reaped is initialized empty and is not restored from the persistent index, so the same task changes from 410 Gone to 404 Not Found and polling clients lose the promised distinction from an unknown ID.
1 participant
No description provided.