feat(research): add governed Burn binary lane - #41
Conversation
|
Warning Review limit reached
Next review available in: 8 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 (4)
📝 WalkthroughWalkthroughAdds source and label availability provenance to governed research snapshots, introduces snapshot integrity verification, and implements a feature-gated Burn binary probability-model training, inference, persistence, and validation lane. ChangesGoverned Burn binary research lane
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ResearchSnapshot
participant Trainer
participant BurnBinaryModel
participant BundleStorage
ResearchSnapshot->>Trainer: provide governed snapshot and event-disjoint selectors
Trainer->>BurnBinaryModel: materialize features and train model
BurnBinaryModel->>BundleStorage: persist manifest and Burnpack
BundleStorage->>BurnBinaryModel: verify digests and load bundle
🚥 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: 189aca69fa
ℹ️ 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
products/ploy/crates/ploy-research/src/model/supervised/burn_binary.rs (2)
1068-1070: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
TrainingBackend::seedreintroduces the process-global RNG coupling the comment above says to avoid.The comment directly above (lines 826-828) explains weights are generated from a local
StdRngspecifically "so determinism does not depend on a process-global backend RNG or concurrent research jobs."TrainingBackend::seed(&device, config.seed)does exactly that: per Burn'sBackendtrait docs, seeding is "guaranteed [for] at least the specified device," and determinism from it "should ensure deterministic execution for a single-threaded program" — i.e., it's not scoped safely across concurrent invocations in the same process. Today this has no functional effect (no other randomness is consumed during training), but it's a latent contradiction of the stated design intent, and would silently break reproducibility guarantees if two missions (e.g., BTC and SOL) are ever trained concurrently in one process, or if a future feature (e.g. dropout) is added.Consider dropping this call (weights are already deterministically seeded locally, and nothing else here consumes backend randomness), or explicitly documenting/enforcing that concurrent calls to
train_event_disjoint_binarywithin one process are unsupported.🤖 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 `@products/ploy/crates/ploy-research/src/model/supervised/burn_binary.rs` around lines 1068 - 1070, Remove the TrainingBackend::seed call from train_event_disjoint_binary so training does not mutate or depend on process-global backend RNG state. Keep the local deterministic initialization through BurnBinaryLinear::<TrainingBackend>::from_seed and leave the existing device setup and training flow unchanged.
250-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid pattern-matching on float literals.
matches!(row.settlement_up, 0.0 | 1.0)matches literal floating-point values in a pattern, which trips Rust'sillegal_floating_point_literal_patternlint. It's not a correctness bug here (the value is authoritative governed evidence), but an explicit equality check is the idiomatic, lint-clean way to express this and keeps the crate's stated goal of a clean Clippy/build output.♻️ Proposed fix
- if !matches!(row.settlement_up, 0.0 | 1.0) { + if row.settlement_up != 0.0 && row.settlement_up != 1.0 { return Err(format!( "snapshot event {} lacks an official binary settlement label", row.event_id )); }🤖 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 `@products/ploy/crates/ploy-research/src/model/supervised/burn_binary.rs` around lines 250 - 255, Update the settlement validation in the surrounding supervised snapshot-processing logic to replace the float-literal matches! pattern with explicit equality checks for 0.0 and 1.0, while preserving the existing error return and event_id message for all other values.
🤖 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 `@products/ploy/crates/ploy-research/src/factors.rs`:
- Around line 117-118: Update products/ploy/crates/ploy-research/src/factors.rs
lines 117-118 so FactorSourceAvailability preserves all seven clock fields in
the serialized observation contract. Update
products/ploy/crates/ploy-research/src/research_snapshot.rs lines 2153-2158 so
observations_to_frame exports every clock, and add coverage verifying JSON and
Parquet outputs retain identical source-availability provenance.
In `@products/ploy/crates/ploy-research/src/research_snapshot.rs`:
- Around line 44-77: Update the current_resolution query to rank resolved
settlement rows per (market_slug, token_id) by the effective availability
timestamp, retaining only the latest row before the market-level aggregation and
binary-row HAVING checks. Preserve append-only settlement storage, and add a
regression test using duplicate settlement history to verify the current outcome
remains available.
---
Nitpick comments:
In `@products/ploy/crates/ploy-research/src/model/supervised/burn_binary.rs`:
- Around line 1068-1070: Remove the TrainingBackend::seed call from
train_event_disjoint_binary so training does not mutate or depend on
process-global backend RNG state. Keep the local deterministic initialization
through BurnBinaryLinear::<TrainingBackend>::from_seed and leave the existing
device setup and training flow unchanged.
- Around line 250-255: Update the settlement validation in the surrounding
supervised snapshot-processing logic to replace the float-literal matches!
pattern with explicit equality checks for 0.0 and 1.0, while preserving the
existing error return and event_id message for all other values.
🪄 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
Run ID: b914c68b-3b17-49a1-a229-d77e4a9a73fd
⛔ Files ignored due to path filters (1)
products/ploy/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
products/ploy/README.mdproducts/ploy/config/research_missions/polymarket-btc-5m.example.jsonproducts/ploy/config/research_missions/polymarket-sol-5m.example.jsonproducts/ploy/crates/ploy-research/Cargo.tomlproducts/ploy/crates/ploy-research/examples/event_dataset_rolling_windows.rsproducts/ploy/crates/ploy-research/src/backtest/engine.rsproducts/ploy/crates/ploy-research/src/dataset/builder.rsproducts/ploy/crates/ploy-research/src/dataset/export.rsproducts/ploy/crates/ploy-research/src/factors.rsproducts/ploy/crates/ploy-research/src/factors_new/scan.rsproducts/ploy/crates/ploy-research/src/factors_v2.rsproducts/ploy/crates/ploy-research/src/lib.rsproducts/ploy/crates/ploy-research/src/model/supervised/burn_binary.rsproducts/ploy/crates/ploy-research/src/model/supervised/mod.rsproducts/ploy/crates/ploy-research/src/prediction_loop.rsproducts/ploy/crates/ploy-research/src/research_snapshot.rsproducts/ploy/crates/ploy-research/src/signal/regime.rsproducts/ploy/crates/ploy-research/src/signal/rules.rsproducts/ploy/docs/BURN_BINARY_RESEARCH.mdproducts/ploy/tasks/todo.md
Summary
Authority boundary
This change adds no execution, deployment, approval, promotion, collector, CI, installer, or live-trading authority. PLOY live trading remains disabled.
Validation
Strict Clippy with -D warnings remains blocked by pre-existing warnings in untouched current-main PLOY files. Those unrelated baseline warnings are intentionally not changed in this PR.
Review
Standards review passed after binding burn_binary.rs into the policy digest and recording the task. Spec review found no remaining scope or behavior gaps.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation