fix(prediction): bind settlement splits to time cohorts - #143
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR upgrades prediction missions to v2 with a required time cohort boundary, forwards that boundary through evaluator processes, applies cohort-aware walk-forward purging, and seals the boundary into binary dataset manifests and partition validation. ChangesPrediction time-boundary governance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Mission
participant ResearchLauncher
participant Evaluator
participant WalkForward
Mission->>ResearchLauncher: provide time_cohort_boundary_ms
ResearchLauncher->>Evaluator: pass boundary CLI argument
Evaluator->>Evaluator: validate boundary and event window
Evaluator->>WalkForward: provide SettlementProbabilityTimeCohort
WalkForward-->>Evaluator: return purged settlement slices
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
| #[test] | ||
| fn evaluator_process_is_the_precompiled_monday_binary() { | ||
| fn evaluator_command_forwards_the_mission_cohort_boundary() { | ||
| let root = std::env::temp_dir().join(format!( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7221a6e664
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust_hft/prediction-markets/crates/ploy-research/src/factors_v2.rs (1)
7378-7441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCohort-aware purge logic is correct; consider extracting the shared event-end lookup.
Verified against
settlement_time_cohort_purges_crossing_and_post_boundary_training_events: crossing/post-boundary events are correctly excluded from both train and test, and both filters fail closed on any missing/overflowing timestamp arithmetic. Thetest_rowsandtrain_rowsfilters each re-deriveevent_ends.get(row.event_id.as_str()).and_then(Option::as_ref), duplicating the lookup already done in theends_inclosure a few lines above — a small helper (e.g.event_end_of(row)) would remove the repetition without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/prediction-markets/crates/ploy-research/src/factors_v2.rs` around lines 7378 - 7441, Extract the repeated event-end lookup in event_disjoint_walk_forward_slices into a shared helper such as event_end_of(row), returning the referenced event end from event_ends. Reuse this helper in ends_in and both train_rows/test_rows cohort filters, preserving the current fail-closed handling and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust_hft/prediction-markets/crates/ploy-research/src/factors_v2.rs`:
- Around line 7378-7441: Extract the repeated event-end lookup in
event_disjoint_walk_forward_slices into a shared helper such as
event_end_of(row), returning the referenced event end from event_ends. Reuse
this helper in ends_in and both train_rows/test_rows cohort filters, preserving
the current fail-closed handling and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e81d7c5d-0224-4cae-ae42-a786f4a4da56
📒 Files selected for processing (13)
docs/architecture/PREDICTION_MARKETS.mdrust_hft/prediction-markets/config/research_missions/polymarket-btc-5m.example.jsonrust_hft/prediction-markets/config/research_missions/polymarket-sol-5m.example.jsonrust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-evaluator.rsrust_hft/prediction-markets/crates/ploy-research/src/bin/monday-prediction-research.rsrust_hft/prediction-markets/crates/ploy-research/src/factors_v2.rsrust_hft/prediction-markets/crates/ploy-research/src/lib.rsrust_hft/prediction-markets/crates/ploy-research/src/model/supervised/burn_binary.rsrust_hft/prediction-markets/crates/ploy-research/src/model/supervised/mod.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_loop.rsrust_hft/prediction-markets/docs/ALPHA_FACTOR_SEARCH_CICD.mdrust_hft/prediction-markets/docs/CRYPTO_LOB_ML_DEPLOY_CHECKLIST.mdrust_hft/prediction-markets/tasks/todo.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Change contract
Bind the governed BTC/SOL five-minute settlement trainer and LoopRun settlement verdict to one mission-pinned wall-clock boundary so complete event lifetimes, rather than only decision rows or event IDs, are separated across train and validation.
Out of scope
Dependency or merge order
The code contract is based directly on
mainand is independently reversible. Documentation PR #144 is stacked on this branch because it describes the new schema and cohort behavior. Merge #143 first, retarget #144 tomain, rerun its current-head checks, then merge #144.Focused validation
main.rustfmt --edition 2021 --checkon touched Rust source filesjq -eon both mission templatesgit diff --check.example.jsonmissions deliberately retain an invalid zero cohort sentinel and placeholder snapshot ID; a targeted test proves full mission validation rejects them until an operator selects real immutable inputs.RustProcessEvaluator::command(request)path and proves the mission boundary reaches the precompiled evaluator argv.Rollout / rollback impact
Research-only. Prediction mission schema moves from v1 to v2 and requires
time_cohort_boundary_ms; binary model manifest schema moves from v2 to v3 and seals that mission boundary into the dataset contract/hash and model manifest. Existing mission v1/model v2 artifacts fail closed and require a new mission/retraining; no boundary is inferred. The checked-in examples remain intentionally non-runnable until their zero boundary andREPLACE_WITH...snapshot placeholder are replaced in a reviewed immutable mission revision. The prediction policy snapshot ID changes because the governed evaluator now consumes the boundary. Roll back by reverting this PR; no data, collector, ECS, execution, or live state is mutated.Architecture note
One
symbol × horizonremains one research task, one event root remains one episode with two independent token LOBs, and settlement probability remains separate from future token execution heads. The current settlement baseline continues to use the retained one-second L2 snapshots.