Repository navigation
Conversation
✅ Action performedComments resolved and changes approved. |
|
🔎 Reviewing |
There was a problem hiding this comment.
This PR makes the liq_pnl_* refresh replay the P&L ledger once per cycle for all six windows, instead of once per window. It splits the report builder into a window-independent part (replay_pnl_rows, run by load_and_replay_pnl) and a per-window part (summarize_pnl_window, run by summarize_pnl_report). /pnl and the refresh task both use the same two steps, so each window's figures match /pnl because they share code, not only because the tests check it. The refresh task drops the per-window deadline, the rotation, the probe report and the cached range: the window dates now come from this cycle's own replay.
The panel checked the warning order, the fallible-step order, permit and catch-up handling, the dates that windows take from the shared range, and upgrade safety (nothing persisted changes). It found no new defect. The fixes from the earlier threads are present at this head, and the items moved to RAI-3195 were not raised again. One nit remains: PnlReplay.book_symbols duplicates the symbols already in replay_symbols.
claude-opus-5-5 · high · 35 min
There was a problem hiding this comment.
This PR makes the liq_pnl_* metrics refresh replay the P&L ledger once per cycle and build all six windows from that one replay, instead of one full report per window. /pnl and the refresh now share the same steps (catch-up and admission, load and replay, then one window's summary and capital), the window dates come from the same replay, the probe report and the cached range are gone, and the per-window deadline from the PR below is replaced by one shared report that the cycle waits on for up to 120 s.
Overall read: the split is sound. The replay reads no date, the warning order is kept, and the parity tests compare each window with the /pnl body. The panel found no correctness, concurrency or data defect at this commit. One minor note: the waiting admission that the PR below added has nothing left to wait for now. The two open threads (no test of the real function with one failing window, and the one-line deps() helper) still apply at this commit.
I also agree with 2 open threads and replied +1 in each.
claude-opus-5-5 · high · 2 h 21 min
|
🔎 Reviewing |
4c82401 to
eb95a7c
Compare
fd6be41 to
70c3e24
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
eb95a7c to
8410389
Compare
70c3e24 to
51b2ed8
Compare
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
There was a problem hiding this comment.
This PR changes the liq_pnl_* refresh so that it catches up and replays the P&L ledger once per cycle and then builds the six windows (1d to all) from that one replay. The P&L builder is split into a replay step and a per-window summary step, and /pnl uses the same two steps for its single window. So the parity with /pnl comes from shared code, and the tests also check it. Each window's dates now come from this cycle's replay, so the probe report and the cached range are gone.
Overall read: the split is sound. The replay does not read the window dates, the warning order is kept, a failed Alpaca fetch or capital step in one window leaves the other windows as they are, and the metrics task never takes a live /pnl permit. No lane found a blocking defect. Three small items are left: SPEC and the module doc blame the exporter for a one-cycle lag that only the bot's own cached range had, the new duration-sample rules have no test, and cycle_windows is more visible than it needs to be. I agree with the two open threads (book_symbols and the waiting admission). Reviewed at fd6be414.
I also agree with 2 open threads and replied +1 in each.
claude-opus-5-5 · high · 4 h 8 min
51b2ed8 to
ca82671
Compare
|
@rain-marvin approve |
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
|
⏳ Queued behind 57 other jobs, @JuaniRios. I'll post here when I start. |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR makes the liq_pnl_* refresh replay the position ledger once per cycle and build all six windows (1w, 1d, all, 1m, ytd, 1y) from that shared replay. Before, each window ran its own replay. /pnl and the metrics windows now share the load, catch-up/admission, replay, summary and capital code (load_and_replay_pnl, catch_up_admitted, summarize_pnl_report). The window dates come from this cycle's availableRange, so the probe report and the cached range are gone.
The head is still ca82671f, the commit of my last review, so nothing new needs review. The last change (from 51b2ed8e) only edits the SPEC.md P&L paragraph and the pnl_refresh.rs module doc. It drops the wrong claim that the exporter lagged one cycle. The new text says the windows end on the new day in the cycle that replays it, as the exporter did. That is correct: collect_pnl probed the range in the same pass (see run_pnl in scripts/liq-parity/golden.py). CI is green on this head, and CodeRabbit has approved.
Earlier findings:
cycle_windowsvisibility nit (thread 4242737288): open in the code, but deferred to RAI-3195 under the P&L refresh durations item. That is fine for a nit.
Threads resolved by hand (all by @JuaniRios, and I confirmed each one in its thread):
- Ledger clone in
replay_pnl_rows(4239633007): addressed.ordered_position_eventsborrows the rows and returns sorted references. - Windows publish together after the shared report (4239633009): dismissed with a sound reason. SPEC, docs/observability.md and the module docs now say one slow Alpaca fetch delays all six windows.
- Per-window duration sample wording (4239633012, 4240433410): addressed. Each sample covers the whole shared report plus the earlier publishes, and the docs say so.
- Hand-copied report steps (4239950857): addressed with the
load_and_replay_pnl/summarize_pnl_reportsplit. - Builder parity test could not catch a bad split (4239950859): addressed. It now asserts literal warning lists, with a fill that has no wrapper ratio, a cost row and an Alpaca FEE.
- Window dates lagged one cycle (4239950860): addressed. The window function runs on the shared replay's range, and the probe is gone.
- Copied catch-up and admission (4240933419): addressed by
catch_up_admitted. The shared Alpaca fetch helper moved to RAI-3195, which is fine for a minor. - SPEC exporter-lag wording (4242737279): addressed in
ca82671f. - Deferred to RAI-3195 as minors or nits, with sound reasons (the code is correct, only longer or less tested than it could be): repeated per-window ledger parsing and lot cloning (4240358927), the detached report task and
PNL_CYCLE_BUDGET(4240358929), no test for one failing window's Alpaca fetch (4241373883), the one-callerdeps()(4241373886), thebook_symbolsfield (4241742165), the waiting admission that never waits (4241838102) and the duration-rule test (4242737283).
No open critical or should-fix finding is left, and no new defect was found. Ready to approve.
claude-opus-5-5 · high · 5 min
agryaznov
left a comment
There was a problem hiding this comment.
LGTM, the deferred minors are tracked in the threads.
8410389 to
625f07a
Compare
ca82671 to
b51a7d5
Compare
Merge activity
|
Part of RAI-3143.
The
liq_pnl_*refresh now replays the P&L ledger once for all six windows instead of once each, one ledger replay per cycle instead of six. The per-window summaries still parse the cost ledgers and filter the matched lots each time, so the saving is the replay, not the whole report. Each window's response is byte-identical to what/pnlreturns for the same dates.Live effect: on the next bot release, less CPU per P&L refresh, same series · Risk: medium (the P&L builder is split in two; a mistake would change P&L numbers, which the parity tests guard) · Ships: next bot release · Blocks: needs #1719 below it
Decisions
/pnland the refresh share the same two steps: load and replay, then one window's summary and capital. So parity comes from shared code, not only from the tests.Proof
pnl_window_reports_match_the_route_for_every_windowseeds fills and checks, for six windows shaped like 1d to all, that/pnlreturns a body byte-identical to the shared report.a_shared_replay_summarizes_each_window_like_its_own_replaychecks the same at builder level, including warning order. Refresh tests cover a failed shared report and a report past the budget. The full suite passes locally.