feat(research): admit reduced Polymarket baseline - #237
Conversation
|
Warning Review limit reached
Next review available in: 2 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 Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR introduces source-specific settlement probability component profiles, validates reduced-authority Polymarket+Chainlink snapshots, rejects unsupported supervised training, and threads the selected profile through evaluator, reporting, and MCTS execution. Mission configs now reference updated search-policy snapshots. ChangesBaseline eligibility and execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ResearchMission
participant PredictionResearch
participant PredictionEvaluator
participant Snapshot
participant MCTS
ResearchMission->>PredictionResearch: load mission and snapshot
PredictionResearch->>Snapshot: read source_kind
PredictionResearch->>MCTS: run with selected component profile
PredictionResearch->>PredictionEvaluator: launch with baseline flag when required
PredictionEvaluator->>Snapshot: validate source, coverage, and prior eligibility
PredictionEvaluator->>MCTS: generate profile-aware reports
Possibly related issues
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11db48d932
ℹ️ 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".
| if !requirements.contains("all") { | ||
| let baseline = manifest.source_kind == crate::POLYMARKET_CHAINLINK_BASELINE_SOURCE_KIND; | ||
| if baseline { | ||
| if requirements != BTreeSet::from([crate::POLYMARKET_CHAINLINK_BASELINE_REQUIREMENT]) |
There was a problem hiding this comment.
Every snapshot produced by build_research_snapshot_from_polymarket_chainlink_baseline declares the profile plus chainlink_reference, polymarket_orderbook, and polymarket_official_settlement in research_snapshot.rs:976-983, so this singleton equality always rejects the production baseline manifest. Because validate_prediction_run_inputs invokes this validator before research begins, no snapshot created by the baseline constructor can reach evaluator/MCTS; the new test passes only because its helper replaces the emitted requirements with a singleton. Validate the actual four-item contract rather than requiring only the profile marker.
Useful? React with 👍 / 👎.
| let advice = if component_profile | ||
| == SettlementProbabilityComponentProfile::MarketMidpointOnly | ||
| { | ||
| state.advisor_failure = Some( | ||
| "reduced-authority baseline has no LLM-expandable probability components" | ||
| .to_string(), | ||
| ); | ||
| Vec::new() |
There was a problem hiding this comment.
For MarketMidpointOnly, this branch removes every advisor action, leaving one deterministic midpoint mutation per level; with MCTS_MAX_DEPTH fixed at 3, the engine can therefore produce only three candidates. Both updated baseline mission examples request six candidates, while the runner continues calling engine.propose() until all six are trained, so the fourth iteration returns prediction MCTS tree has no expandable node instead of completing held-out evaluation. Cap or terminate this profile's search when its finite tree is exhausted, or require a compatible candidate budget.
Useful? React with 👍 / 👎.
| let mut by_model: BTreeMap<String, Vec<SettlementProbabilitySample>> = BTreeMap::new(); | ||
| for (row, win, pnl, conservative_pnl) in eligible_rows { | ||
| for (model, q) in settlement_probability_models(row) { | ||
| for (model, q) in settlement_probability_models(row, options.component_profile) { |
There was a problem hiding this comment.
The profile-aware model call excludes the registered event-surface component, but immediately afterward q_event_surface_empirical is still added unconditionally; EventVolSurface::predict returns its leave-one-event-out global mean even when baseline distance features are NaN, so a normal multi-event MarketMidpointOnly snapshot still reports this non-midpoint model. Promotion-gate selection treats every non-naive model as a candidate, allowing baseline gates and artifacts to be driven by the supposedly ineligible empirical surface. Gate both event-surface fitting and emission on FullSurface in the probability and verdict report paths.
Useful? React with 👍 / 👎.
| row.depth_imbalance, | ||
| row.depth_far_ratio, | ||
| row.depth_acceleration, | ||
| row.obi_10, |
There was a problem hiding this comment.
For baseline rows this check correctly requires raw obi_10 to be non-finite, but build_factor_observations_v2 subsequently calls flip_count, where signum(NaN) becomes 0, the sample is filtered out, and the empty window's count is returned as finite 0.0. Because obi_flip_count_60s is registered as a CexLob factor and the core evaluator still runs the factor walk-forward report, the reduced-authority lane silently reintroduces a finite CEX placeholder after validation. Return NaN when the flip window contains no finite observations, or profile-gate this derived factor before reporting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs (1)
329-367: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the baseline masking/validation lists aligned. The CEX-derived feature lists currently match each other, but they omit the Binance-quote-derived snapshot fields
fair_prob_up,fair_prob_up_clean,prob_disagreement,reward_risk_up, andreward_risk_down; add these same fields toclear_polymarket_chainlink_baseline_unavailable_featuresandunavailable_cex_featuresso Polymarket+Chainlink baselines cannot retain values from these Binance-fed inputs.🤖 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/research_snapshot.rs` around lines 329 - 367, Add fair_prob_up, fair_prob_up_clean, prob_disagreement, reward_risk_up, and reward_risk_down to clear_polymarket_chainlink_baseline_unavailable_features in rust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs:329-367 and to unavailable_cex_features in rust_hft/prediction-markets/crates/ploy-research/src/prediction_loop.rs:858-907, keeping both masking and validation lists aligned so these Binance-fed fields are unavailable for the Polymarket+Chainlink baseline.
🤖 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.
Inline comments:
In `@rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rs`:
- Around line 228-247: Update PredictionMctsRunState and
run_or_resume_prediction_mcts_with_component_profile to persist the supplied
component_profile in durable state, compare it when loading existing state
alongside version and mission, and return the existing incompatibility error
before any checkpointing or evaluator invocation when profiles differ. Ensure
newly created state records the current profile.
In `@rust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts.rs`:
- Around line 687-698: Update blend_allowed to use the shared
factors_v2::probability_blend_allowed predicate or its exposed epsilon rule
instead of literal zero comparisons for inactive weights, while preserving the
required positive market midpoint weight and FullSurface behavior. Ensure MCTS
accepts the same reduced-profile blends as the evaluator.
---
Outside diff comments:
In `@rust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs`:
- Around line 329-367: Add fair_prob_up, fair_prob_up_clean, prob_disagreement,
reward_risk_up, and reward_risk_down to
clear_polymarket_chainlink_baseline_unavailable_features in
rust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs:329-367
and to unavailable_cex_features in
rust_hft/prediction-markets/crates/ploy-research/src/prediction_loop.rs:858-907,
keeping both masking and validation lists aligned so these Binance-fed fields
are unavailable for the Polymarket+Chainlink baseline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6ec67d0-6547-4123-a359-513234b6c455
📒 Files selected for processing (10)
rust_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/model/supervised/burn_binary.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_loop.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts.rsrust_hft/prediction-markets/crates/ploy-research/src/prediction_mcts_run.rsrust_hft/prediction-markets/crates/ploy-research/src/research_snapshot.rs
| pub fn run_or_resume_prediction_mcts_with_component_profile< | ||
| C: ProposalClient, | ||
| E: PredictionMctsRunEvaluator, | ||
| >( | ||
| mission: PredictionResearchMission, | ||
| snapshot_dir: &Path, | ||
| output_dir: &Path, | ||
| client: &mut C, | ||
| evaluator: &mut E, | ||
| component_profile: SettlementProbabilityComponentProfile, | ||
| ) -> Result<LoopRunSummary, String> { | ||
| validate_prediction_mission(&mission, ¤t_prediction_policy_snapshot_id())?; | ||
| let _lock = OutputLock::acquire(output_dir)?; | ||
| let state_path = output_dir.join("prediction-mcts-state.json"); | ||
| let mut state = if state_path.exists() { | ||
| let state: PredictionMctsRunState = read_json(&state_path)?; | ||
| if state.version != RUN_STATE_VERSION || state.mission != mission { | ||
| return Err("prediction MCTS output belongs to a different mission".to_string()); | ||
| return Err( | ||
| "prediction MCTS output uses an incompatible state version or mission".to_string(), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Persist the component profile in PredictionMctsRunState.
Compatibility currently checks only state version and mission. Before an optional checkpoint exists, the same output can resume with a different profile and run evaluator/advisor steps under changed authority. Store component_profile in the durable state and reject a mismatch before checkpointing or invoking the evaluator.
🤖 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/prediction_mcts_run.rs`
around lines 228 - 247, Update PredictionMctsRunState and
run_or_resume_prediction_mcts_with_component_profile to persist the supplied
component_profile in durable state, compare it when loading existing state
alongside version and mission, and return the existing incompatibility error
before any checkpointing or evaluator invocation when profiles differ. Ensure
newly created state records the current profile.
| fn blend_allowed( | ||
| profile: SettlementProbabilityComponentProfile, | ||
| blend: &LlmProbabilityBlendSpec, | ||
| ) -> bool { | ||
| profile == SettlementProbabilityComponentProfile::FullSurface | ||
| || (blend.market_midpoint_weight > 0.0 | ||
| && blend.chainlink_digital_weight == 0.0 | ||
| && blend.distance_lob_vol_weight == 0.0 | ||
| && blend.event_surface_weight == 0.0 | ||
| && blend.existing_model_weight == 0.0) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align reduced-profile blend eligibility with the evaluator.
This requires literal 0.0 for inactive weights, while factors_v2::probability_blend_allowed accepts weights <= EPS. A blend accepted by reporting can therefore be rejected by MCTS. Reuse or expose the shared predicate/epsilon rule rather than duplicating it.
🤖 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/prediction_mcts.rs`
around lines 687 - 698, Update blend_allowed to use the shared
factors_v2::probability_blend_allowed predicate or its exposed epsilon rule
instead of literal zero comparisons for inactive weights, while preserving the
required positive market midpoint weight and FullSurface behavior. Ensure MCTS
accepts the same reduced-profile blends as the evaluator.
11db48d to
675424a
Compare
|
Exact-head Matt Spec review raised whether |
Change contract
Allow evaluator and shared MCTS research to consume the explicit verified Polymarket + Chainlink baseline only when every Binance-derived component and training feature is provably ineligible, while leaving the full-surface profile unchanged.
Closes #227.
Out of scope
Snapshot construction; collector code or deployment; Binance backfill; cloud orchestration or result publication; Paper or Live; OMS/RiskGate; profitability claims; 15-minute or 1-hour products; #189 retirement.
Dependency or merge order
Depends on merged #228. After this PR merges: #232 snapshot construction, then #233 evaluator/MCTS, then #234 immutable completion proof.
Focused validation
cargo test -p ploy-research --lib— 285 passedcargo test -p ploy-research --bin monday-prediction-research— 2 passedcargo test -p ploy-research --features db --bin monday-prediction-evaluator— 12 passedcargo test -p ploy-research --features ml --lib model::supervised::burn_binary::tests::rejects_unregistered_label_features_and_caller_supplied_values -- --exact— passedgit diff --check— passedTargeted counterexamples reject finite CEX placeholders, unsupported component weights, malformed omission markers, baseline legacy-loop entry, and incompatible pre-profile durable run state.
Clippy remains blocked by unchanged pre-existing lints in
ploy-market-contractsandploy-feed-loaders; this PR does not modify those crates.Rollout/rollback impact
Rollout is opt-in only for the explicit reduced-authority source kind and exact evaluator flag/profile match. Rollback removes baseline consumer admission; immutable snapshots remain readable, while evaluator and MCTS fail closed again. No collector or runtime deployment changes.
Summary by CodeRabbit
New Features
Bug Fixes
Compatibility