Merge branch 'fix/ingestion-shape'

Reject malformed events at the ingestion boundary, add
EventBuilder::members, and record the rayon opt-in deviation.

Closes #5
Closes #37

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
This commit is contained in:
2026-09-08 15:53:13 +02:00
co-authored by Claude Opus 5
8 changed files with 462 additions and 12 deletions
+13 -7
View File
@@ -134,14 +134,20 @@ h.add_events(vec![Event {
h.converge().unwrap();
```
Like `with_prior`, the scale is **competitor configuration captured at first
appearance** — setting it on a key the history already knows has no effect. It
must be finite and non-negative; ingestion otherwise fails with
`InferenceError::InvalidParameter`.
Like `with_prior`, the scale is **competitor configuration, not a per-event
value**: it applies to the competitor for the whole history, and it applies
whenever it is supplied — including on a key the history already knows.
Configuring one late still refits the whole history rather than taking effect
only from that event onward, because `converge` refits from competitor state.
Repeating the same value is inert; supplying two *different* values for one
competitor within a single batch is `InferenceError::ConflictingCompetitorConfig`,
since events in a batch have no order. The scale must be finite and
non-negative; ingestion otherwise fails with `InferenceError::InvalidParameter`.
Note that the fluent `EventBuilder` (`h.event(t).team([...])`) sets weights but
not `drift_scale` or `prior`; those need the typed `Event` / `Team` / `Member`
shape shown above.
The fluent `EventBuilder` reaches this too: `.team([...])` is the common case
and leaves both unset, while `.members([...])` takes `Member` values directly,
so `h.event(t).members([Member::new("layout_7").with_drift_scale(0.0)])` is
equivalent to the typed shape above.
## Scored outcomes
@@ -500,6 +500,26 @@ All public traits (`Time`, `Drift`, `Observer`, `Factor`, `Schedule`) require `S
`rayon` as default-on feature; with `default-features = false`, parallel paths fall back to sequential iterators behind `cfg(feature = "rayon")`.
> **Not implemented. Deliberate deviation, decided 2026-09-08 (issue #5).**
>
> `rayon` ships **opt-in**: `Cargo.toml` has no `default = [...]` key. The
> measured speedups are 1.0x on realistic workloads and 1.3x on a pathological
> one (issue #4), because typical slices hold too few events to amortize
> rayon's task-spawn overhead. Default-on would hand every downstream user a
> thread pool and a dependency for approximately no gain.
>
> This section made the trade conditional on cross-slice dirty-bit skipping
> landing and changing the parallel story. It did not land: #4 was closed on
> 2026-08-27 by removing the inert `ConvergenceReport::slices_skipped` field
> rather than by implementing the mechanism, so the re-measurement this was
> waiting on will not arrive.
>
> The "Trade-offs" note below also cited an `unsafe` concurrent-write path
> through `SkillStore` as a cost of default-on. That cost does not exist: the
> crate is `#![forbid(unsafe_code)]`, and the compute/apply split on the
> internal `Event` is what lets a color group run in parallel without it. The
> case for opt-in rests on the measurements alone.
### Expected speedup ballpark
For 1000 players, 60 events/slice × 1000 slices, 30 convergence iterations:
@@ -521,7 +541,7 @@ These are pre-implementation estimates. Each tier validates with criterion.
- Color-group parallelism requires up-front graph coloring at ingestion. Cost: linear in events, run once per `add_events`. Cheap.
- Default = asynchronous EP (preserves current semantics). Synchronous opt-in only.
- Cross-slice sweep stays sequential; no speculative parallel sweeps.
- Rayon default-on but feature-gated.
- Rayon default-on but feature-gated. **Superseded — shipped opt-in; see the deviation note in Section 6.**
### Open question
+5 -2
View File
@@ -88,7 +88,9 @@ impl<K> Member<K> {
/// Set this competitor's starting skill estimate.
///
/// Captured at the competitor's first appearance; see the type docs.
/// Competitor configuration, not a per-event value: it applies for the
/// whole history and applies whenever it is supplied, including on a key
/// the history already knows. See the type docs.
pub fn with_prior(mut self, prior: Gaussian) -> Self {
self.prior = Some(prior);
self
@@ -104,7 +106,8 @@ impl<K> Member<K> {
/// shares a scale with moving competitors but should not itself move: a bot
/// at a known strength, a rating floor, a course difficulty.
///
/// Captured at the competitor's first appearance; see the type docs.
/// Applies for the whole history and whenever it is supplied, including on
/// a key the history already knows; see the type docs.
/// Must be finite and non-negative, or ingestion fails with
/// [`InferenceError::InvalidParameter`](crate::InferenceError::InvalidParameter).
pub fn with_drift_scale(mut self, scale: f64) -> Self {
+36
View File
@@ -50,6 +50,8 @@ where
}
/// Add a team by its member keys (weight 1.0 each, no prior overrides).
///
/// Use [`EventBuilder::members`] to set `prior` or `drift_scale`.
pub fn team<I: IntoIterator<Item = K>>(mut self, keys: I) -> Self {
let members: SmallVec<[Member<K>; 4]> = keys.into_iter().map(Member::new).collect();
self.event.teams.push(Team { members });
@@ -57,6 +59,40 @@ where
self
}
/// Add a team from fully-specified [`Member`] values.
///
/// [`EventBuilder::team`] is the common case and builds members with
/// `Member::new`, which leaves `prior` and `drift_scale` unset. This is the
/// escape hatch for when they matter:
///
/// ```
/// # use trueskill_tt::{Gaussian, History, Member};
/// # let mut h = History::builder().build();
/// h.event(0)
/// .team(["player"])
/// .members([Member::new("layout_7")
/// .with_drift_scale(0.0)
/// .with_prior(Gaussian::from_ms(0.0, 1.0))])
/// .ranking([0, 1])
/// .commit()?;
/// # Ok::<(), trueskill_tt::InferenceError>(())
/// ```
///
/// One method rather than a `priors` and a `drift_scales` setter beside
/// `weights`: those would have to grow a parallel array — and a parallel
/// length check — every time `Member` gains a field, and each one would be
/// a new way to get the lengths wrong. `Member`'s own builder already
/// expresses all of it.
///
/// `prior` and `drift_scale` are competitor configuration rather than
/// per-event values; see [`Member`] for what that means for a key the
/// history already knows.
pub fn members<I: IntoIterator<Item = Member<K>>>(mut self, members: I) -> Self {
self.event.teams.push(Team::with_members(members));
self.current_team_idx = Some(self.event.teams.len() - 1);
self
}
/// Set per-member weights for the most recently added team.
///
/// A length mismatch is recorded and returned by [`EventBuilder::commit`]
+44
View File
@@ -1505,6 +1505,50 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
});
}
// Chokepoint for event shape, for the same reason as the tie check
// below: every ingestion route lands here.
//
// `run_chain` builds one diff link per adjacent pair of teams, so a
// one-team event leaves it with an empty link vector and panics
// indexing `links[1..]` — a reachable panic from safe API, in release.
// An empty team is the quieter half: it contributes no performance,
// so a malformed event yields a finite, plausible-looking posterior
// for whoever it was matched against.
//
// Both errors already existed; they were only ever checked on the
// prediction paths, which is why ingestion could still produce them.
for teams in &composition {
if teams.len() < 2 {
return Err(InferenceError::NotEnoughTeams { got: teams.len() });
}
for (team, members) in teams.iter().enumerate() {
if members.is_empty() {
return Err(InferenceError::EmptyTeam { team });
}
}
}
// A non-finite outcome poisons the history rather than failing it:
// `converge` does report `NonFiniteResult`, but a caller who reads
// `current_skill` before converging is handed a NaN posterior with
// nothing to say it is one.
if let Some(results) = results.as_ref() {
for (event_results, kind) in results.iter().zip(kinds.iter()) {
let name = match kind {
EventKind::Ranked => "rank",
EventKind::Scored { .. } => "score",
};
for value in event_results {
if !value.is_finite() {
return Err(InferenceError::InvalidParameter {
name,
value: *value,
});
}
}
}
}
// Chokepoint for tie validation: every ingestion route lands here,
// including `record_draw`, which builds its results directly rather
// than going through `Outcome`.
+3 -2
View File
@@ -170,8 +170,9 @@ fn event_builder_rejects_a_weights_length_mismatch() {
fn event_builder_weights_mismatch_leaves_the_history_untouched() {
let mut h = History::default();
// Two teams, so ingestion would otherwise succeed — a one-team event is
// rejected for an unrelated reason and would pass this vacuously.
// Two teams, so ingestion would otherwise succeed. A one-team event is
// rejected as `NotEnoughTeams` before the weights are ever examined, so
// building this with one team would pass vacuously.
let _ = h
.event(1)
.team(["a"])
+193
View File
@@ -0,0 +1,193 @@
//! `EventBuilder::members` must reach exactly what the typed path reaches.
//!
//! Before this existed, `EventBuilder` could set weights and nothing else, so
//! `prior` and `drift_scale` were expressible only through `Event`/`Team`/
//! `Member` + `add_events`. Which ingestion route a competitor arrived through
//! decided whether it could be configured at all.
use smallvec::smallvec;
use trueskill_tt::{
ConstantDrift, ConvergenceOptions, Event, Gaussian, History, InferenceError, Member, Outcome,
Team,
};
type H = History<i64, ConstantDrift, trueskill_tt::NullObserver, &'static str>;
fn history() -> H {
History::builder()
.mu(0.0)
.sigma(6.0)
.beta(1.0)
.score_sigma(2.0)
.drift(ConstantDrift(0.5))
.convergence(ConvergenceOptions {
max_iter: 20_000,
epsilon: 1e-13,
alpha: 1.0,
})
.build()
}
const PRIOR: Gaussian = Gaussian::from_ms(3.0, 1.5);
/// The contract that makes the escape hatch worth having: same configuration,
/// same fit, bit for bit.
#[test]
fn members_matches_the_typed_path_exactly() {
let mut typed = history();
typed
.add_events(vec![Event {
time: 1,
teams: smallvec![
Team::with_members([Member::new("player")]),
Team::with_members([Member::new("layout_7")
.with_drift_scale(0.0)
.with_prior(PRIOR)]),
],
outcome: Outcome::scores([5.0, 2.0]),
}])
.unwrap();
assert!(typed.converge().unwrap().converged);
let mut fluent = history();
fluent
.event(1)
.team(["player"])
.members([Member::new("layout_7")
.with_drift_scale(0.0)
.with_prior(PRIOR)])
.scores([5.0, 2.0])
.commit()
.unwrap();
assert!(fluent.converge().unwrap().converged);
for key in ["player", "layout_7"] {
let a = typed.current_skill(&key).unwrap();
let b = fluent.current_skill(&key).unwrap();
assert_eq!(a.pi(), b.pi(), "{key} pi");
assert_eq!(a.tau(), b.tau(), "{key} tau");
}
}
/// The configuration has to actually take effect, not merely round-trip: a
/// competitor pinned with `drift_scale = 0.0` must not move across slices,
/// where an unpinned one does.
///
/// The comparison is against a control rather than against a fixed epsilon.
/// Pinned marginals are not bit-identical across slices — each slice combines
/// its own forward and backward messages, so the arithmetic order differs and
/// the last bit moves. What "pinned" promises is that no drift variance
/// accumulates, and the control is what makes that measurable.
#[test]
fn a_drift_scale_set_through_members_is_applied() {
fn spread(h: &H, key: &'static str) -> f64 {
let curve = h.learning_curve(&key);
assert!(curve.len() >= 2, "{key}: expected several appearances");
let (lo, hi) = curve.iter().fold((f64::MAX, f64::MIN), |(lo, hi), (_, g)| {
(lo.min(g.sigma()), hi.max(g.sigma()))
});
(hi - lo) / hi
}
let mut h = history();
for t in 1..=4 {
h.event(t)
.team(["player"])
.members([Member::new("pinned").with_drift_scale(0.0)])
.scores([5.0, 2.0])
.commit()
.unwrap();
// Same shape, no pinning: the control.
h.event(t)
.team(["rival"])
.team(["drifting"])
.scores([5.0, 2.0])
.commit()
.unwrap();
}
assert!(h.converge().unwrap().converged);
let pinned = spread(&h, "pinned");
let drifting = spread(&h, "drifting");
assert!(pinned < 1e-9, "pinned competitor moved: {pinned:e}");
assert!(
drifting > 1e-3,
"control did not move, so the test proves nothing: {drifting:e}"
);
}
/// `weights` still applies to a team added through `members`, and still
/// records a mismatch rather than partially applying it.
#[test]
fn weights_still_guards_a_members_team() {
let mut h = history();
let err = h
.event(1)
.team(["a"])
.members([Member::new("b"), Member::new("c")])
.weights([1.0])
.winner(0)
.commit()
.unwrap_err();
assert!(
matches!(
err,
InferenceError::MismatchedShape {
kind: "weights",
expected: 2,
got: 1
}
),
"{err:?}"
);
assert!(h.current_skill(&"b").is_none(), "nothing may reach history");
}
/// An invalid `drift_scale` surfaces from `commit`, not from a panic and not
/// silently.
#[test]
fn an_invalid_drift_scale_surfaces_from_commit() {
for bad in [-1.0, f64::NAN, f64::INFINITY] {
let mut h = history();
let err = h
.event(1)
.team(["a"])
.members([Member::new("b").with_drift_scale(bad)])
.winner(0)
.commit()
.unwrap_err();
assert!(
matches!(
err,
InferenceError::InvalidParameter {
name: "drift_scale",
..
}
),
"{bad}: {err:?}"
);
assert!(h.current_skill(&"b").is_none(), "{bad} reached the history");
}
}
/// `members` and `team` compose in either order.
#[test]
fn members_and_team_interleave() {
let mut h = history();
h.event(1)
.members([Member::new("a").with_prior(PRIOR)])
.team(["b"])
.scores([3.0, 1.0])
.commit()
.unwrap();
h.event(2)
.team(["b"])
.members([Member::new("c").with_prior(PRIOR)])
.scores([2.0, 4.0])
.commit()
.unwrap();
assert!(h.converge().unwrap().converged);
for key in ["a", "b", "c"] {
assert!(h.current_skill(&key).is_some(), "{key} missing");
}
}
+147
View File
@@ -0,0 +1,147 @@
//! Malformed events must be rejected at the ingestion boundary.
//!
//! Every case here was reachable from safe public API in a release build. Two
//! of them are the two shapes this crate's defects keep taking: a panic from
//! deep inside inference, and a finite, plausible-looking posterior computed
//! from an event that should never have been accepted.
//!
//! `InferenceError::NotEnoughTeams` and `EmptyTeam` already existed when these
//! were found — they were checked on the prediction paths and nowhere else, so
//! ingestion could still manufacture the states they describe.
use smallvec::smallvec;
use trueskill_tt::{Event, History, InferenceError, Member, Outcome, Team};
type Ev = Event<i64, &'static str>;
fn history() -> History<i64, trueskill_tt::ConstantDrift, trueskill_tt::NullObserver, &'static str>
{
History::builder().score_sigma(1.0).build()
}
fn teams(names: &[&[&'static str]]) -> smallvec::SmallVec<[Team<&'static str>; 4]> {
names
.iter()
.map(|team| Team::with_members(team.iter().map(|k| Member::new(*k))))
.collect()
}
/// The regression this file exists for: `run_chain` builds one diff link per
/// adjacent pair of teams, so a one-team event left it indexing `links[1..]`
/// on an empty vector and panicked — in release, from `History::add_events`.
#[test]
fn a_one_team_event_is_an_error_not_a_panic() {
let mut h = history();
let err = h
.add_events(vec![Ev {
time: 1,
teams: teams(&[&["a"]]),
outcome: Outcome::winner(0, 1),
}])
.unwrap_err();
assert!(
matches!(err, InferenceError::NotEnoughTeams { got: 1 }),
"{err:?}"
);
}
#[test]
fn a_zero_team_event_is_an_error() {
let mut h = history();
let err = h
.add_events(vec![Ev {
time: 1,
teams: smallvec![],
outcome: Outcome::ranking([]),
}])
.unwrap_err();
assert!(
matches!(err, InferenceError::NotEnoughTeams { got: 0 }),
"{err:?}"
);
}
/// The quiet half. An empty team contributes no performance, so before this
/// was rejected the event converged and handed back a finite posterior for its
/// opponent — a plausible constant computed from nothing.
#[test]
fn an_empty_team_is_an_error_rather_than_a_free_win() {
let mut h = history();
let err = h
.add_events(vec![Ev {
time: 1,
teams: teams(&[&[], &["b"]]),
outcome: Outcome::winner(0, 2),
}])
.unwrap_err();
assert!(
matches!(err, InferenceError::EmptyTeam { team: 0 }),
"{err:?}"
);
// Nothing was recorded, so the history is still empty.
assert!(h.current_skill(&"b").is_none());
}
#[test]
fn an_empty_team_is_reported_by_position() {
let mut h = history();
let err = h
.add_events(vec![Ev {
time: 1,
teams: teams(&[&["a"], &[]]),
outcome: Outcome::winner(0, 2),
}])
.unwrap_err();
assert!(
matches!(err, InferenceError::EmptyTeam { team: 1 }),
"{err:?}"
);
}
/// A NaN score used to ingest cleanly. `converge` reported `NonFiniteResult`,
/// but a caller who read `current_skill` first was handed `tau: NaN` with
/// nothing to say so.
#[test]
fn a_non_finite_score_is_rejected_at_ingestion() {
for bad in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] {
let mut h = history();
let err = h
.add_events(vec![Ev {
time: 1,
teams: teams(&[&["a"], &["b"]]),
outcome: Outcome::scores([bad, 0.0]),
}])
.unwrap_err();
assert!(
matches!(err, InferenceError::InvalidParameter { name: "score", .. }),
"{bad}: {err:?}"
);
assert!(h.current_skill(&"a").is_none(), "{bad} was recorded anyway");
}
}
/// The fluent builder routes through the same chokepoint, so it inherits the
/// checks rather than needing its own.
#[test]
fn the_event_builder_inherits_the_shape_checks() {
let mut h = history();
let err = h.event(1).team(["a"]).winner(0).commit().unwrap_err();
assert!(
matches!(err, InferenceError::NotEnoughTeams { got: 1 }),
"{err:?}"
);
}
/// A well-formed event is untouched by any of this.
#[test]
fn a_well_formed_event_still_ingests() {
let mut h = history();
h.add_events(vec![Ev {
time: 1,
teams: teams(&[&["a"], &["b"]]),
outcome: Outcome::scores([3.0, 1.0]),
}])
.unwrap();
assert!(h.converge().unwrap().converged);
assert!(h.current_skill(&"a").unwrap().mu() > h.current_skill(&"b").unwrap().mu());
}