Skip to content

Commit ecd4305

Browse files
committed
fix(speculation): preserve predictor factor contract
## Summary ### Why? Neutral prediction changed exact scorer prices at 0 and 1, non-finite factors could create certainty, and failed-path compounding relied on duplicate logical paths that a valid path set cannot contain. ### What? Return the scorer price unchanged for a neutral combined factor, reject non-finite configured factors, keep revised outputs strictly inside the probability range, and apply failed all-succeeds evidence at most once. Extend tests for exact endpoints, large factors, and infinite-factor rejection. ## Test Plan - ✅ `./tool/bazel test //submitqueue/extension/speculation/predictor/...`
1 parent be31122 commit ecd4305

2 files changed

Lines changed: 33 additions & 22 deletions

File tree

submitqueue/extension/speculation/predictor/evidence/evidence.go

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ type Factors struct {
3535
// PathPassed applies once when a build has passed on the batch's
3636
// all-succeed path.
3737
PathPassed float64
38-
// PathFailed applies once per failed all-succeed path, compounding.
38+
// PathFailed applies once when the all-succeed path has failed.
3939
PathFailed float64
4040
// Merging applies while the batch is merging.
4141
Merging float64
@@ -66,7 +66,7 @@ type evidence struct {
6666
// New creates an evidence predictor bound to the queue named in cfg, revising
6767
// base's price by factors.
6868
//
69-
// It rejects a nil base and non-positive factors.
69+
// It rejects a nil base and factors that are non-finite or not positive.
7070
func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally.Scope) (predictor.Predictor, error) {
7171
if base == nil {
7272
return nil, fmt.Errorf("evidence.New: base must not be nil")
@@ -79,8 +79,8 @@ func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally.
7979
} {
8080
// Zero would permanently pin matching batches to 0; negatives cannot
8181
// represent either direction in the factor contract.
82-
if !(factor > 0) {
83-
return nil, fmt.Errorf("evidence.New: factor %s must be positive, got %v", name, factor)
82+
if !(factor > 0) || math.IsInf(factor, 0) {
83+
return nil, fmt.Errorf("evidence.New: factor %s must be finite and positive, got %v", name, factor)
8484
}
8585
}
8686
return &evidence{cfg: cfg, base: base, factors: factors, scope: scope}, nil
@@ -103,25 +103,32 @@ func (r *evidence) Predict(ctx context.Context, batch entity.Batch, paths entity
103103
return 0, fmt.Errorf("base scorer returned %v, which is not a probability", price)
104104
}
105105

106-
factor := math.Pow(r.factors.PathFailed, float64(countFailed(paths)))
106+
factor := 1.0
107107
if hasPassedAllSucceedPath(paths) {
108108
factor *= r.factors.PathPassed
109109
}
110+
if hasFailedAllSucceedPath(paths) {
111+
factor *= r.factors.PathFailed
112+
}
110113
switch batch.State {
111114
case entity.BatchStateMerging:
112115
factor *= r.factors.Merging
113116
case entity.BatchStateCancelling:
114117
factor *= r.factors.Cancelling
115118
}
119+
if factor == 1 {
120+
return predictor.Probability(price), nil
121+
}
116122
return revise(math.Min(math.Max(price, epsilon), 1-epsilon), factor), nil
117123
}
118124

119125
// revise applies the combined factor while keeping the result a probability.
120126
func revise(price, factor float64) predictor.Probability {
121127
if math.IsInf(factor, 1) {
122-
return 1
128+
return 1 - epsilon
123129
}
124-
return predictor.Probability(price * factor / (1 - price + price*factor))
130+
revised := price * factor / (1 - price + price*factor)
131+
return predictor.Probability(math.Min(math.Max(revised, epsilon), 1-epsilon))
125132
}
126133

127134
// hasPassedAllSucceedPath reports a passed build on the batch's all-succeed
@@ -149,15 +156,13 @@ func assumesAllSucceed(path entity.SpeculationPath) bool {
149156
return true
150157
}
151158

152-
// countFailed counts failed builds on the batch's all-succeed path; each one
153-
// compounds. Flip-subset failures are ignored: they were built under different
154-
// assumptions, the same filter PathPassed uses.
155-
func countFailed(paths entity.SpeculationPathSet) int {
156-
failed := 0
159+
// hasFailedAllSucceedPath reports a failed build on the batch's all-succeed
160+
// path. Flip-subset failures were built under different assumptions.
161+
func hasFailedAllSucceedPath(paths entity.SpeculationPathSet) bool {
157162
for _, entry := range paths.Paths {
158163
if entry.Status == entity.SpeculationPathStatusFailed && assumesAllSucceed(entry.Path) {
159-
failed++
164+
return true
160165
}
161166
}
162-
return failed
167+
return false
163168
}

submitqueue/extension/speculation/predictor/evidence/evidence_test.go

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,10 @@ func predict(t *testing.T, price float64, factors Factors, batch entity.Batch, p
7373
}
7474

7575
func TestPredict_NeutralFactorsReturnTheScorersPrice(t *testing.T) {
76-
for _, price := range []float64{0.01, 0.25, 0.5, 0.6, 0.9, 0.99} {
76+
for _, price := range []float64{0, 0.01, 0.25, 0.5, 0.6, 0.9, 0.99, 1} {
7777
t.Run(fmt.Sprintf("price %v", price), func(t *testing.T) {
7878
got := predict(t, price, AllOnes(), entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed))
79-
assert.InDelta(t, price, got, 1e-9)
79+
assert.Equal(t, price, got)
8080
})
8181
}
8282
}
@@ -108,12 +108,6 @@ func TestPredict_AppliesOneFactorPerEvidence(t *testing.T) {
108108
paths: pathSet(entity.SpeculationPathStatusFailed),
109109
want: 0.2,
110110
},
111-
{
112-
name: "failed paths compound",
113-
factors: Factors{PathPassed: 1, PathFailed: 0.5, Merging: 1, Cancelling: 1},
114-
paths: pathSet(entity.SpeculationPathStatusFailed, entity.SpeculationPathStatusFailed),
115-
want: 0.2,
116-
},
117111
{
118112
name: "merging",
119113
factors: Factors{PathPassed: 1, PathFailed: 1, Merging: 19, Cancelling: 1},
@@ -212,6 +206,15 @@ func TestPredict_CertainPricesStayInRangeAndStillMove(t *testing.T) {
212206
}
213207
}
214208

209+
func TestPredict_LargeFactorsDoNotProduceCertainty(t *testing.T) {
210+
factors := AllOnes()
211+
factors.PathPassed = math.MaxFloat64
212+
213+
got := predict(t, 0.5, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed))
214+
assert.Greater(t, got, 0.99)
215+
assert.Less(t, got, 1.0)
216+
}
217+
215218
func TestPredict_RejectsAPriceThatIsNotAProbability(t *testing.T) {
216219
for _, price := range []float64{-0.1, 1.5, math.NaN()} {
217220
t.Run(fmt.Sprintf("price %v", price), func(t *testing.T) {
@@ -235,6 +238,8 @@ func TestNew_RejectsUnusableConstruction(t *testing.T) {
235238
zeroed.Merging = 0
236239
negative := AllOnes()
237240
negative.PathFailed = -1
241+
infinite := AllOnes()
242+
infinite.PathPassed = math.Inf(1)
238243

239244
tests := []struct {
240245
name string
@@ -244,6 +249,7 @@ func TestNew_RejectsUnusableConstruction(t *testing.T) {
244249
{name: "nil base", base: nil, factors: AllOnes()},
245250
{name: "zero factor", base: fixedScorer{price: 0.5}, factors: zeroed},
246251
{name: "negative factor", base: fixedScorer{price: 0.5}, factors: negative},
252+
{name: "infinite factor", base: fixedScorer{price: 0.5}, factors: infinite},
247253
{name: "unset factors", base: fixedScorer{price: 0.5}, factors: Factors{}},
248254
}
249255
for _, tt := range tests {

0 commit comments

Comments
 (0)