From df6db8a46c545148623155a374e9b0d42ca6041b Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 7 Sep 2026 16:53:52 -0700 Subject: [PATCH 1/8] feat(speculation): add outcome predictor implementation ## Summary ### Why? A scorer prices change content, but speculation also needs a separate contract for revising that price with evidence observed during a run. Keeping the concerns separate avoids adding path data that every content scorer would discard. ### What? Add the `predictor.Predictor` contract and an evidence implementation that converts the scorer probability to odds, applies factors for passed and failed all-succeed paths plus merging and cancelling states, and converts the result back to a probability. Include generated mocks and unit coverage for neutral factors, compounding evidence, path filtering, bounds, validation, and scorer failures. ## Test Plan - `bazel test //submitqueue/extension/speculation/predictor/...` - `make check-gazelle` --- .../speculation/predictor/BUILD.bazel | 9 + .../predictor/evidence/BUILD.bazel | 29 ++ .../predictor/evidence/evidence.go | 183 +++++++++++++ .../predictor/evidence/evidence_test.go | 257 ++++++++++++++++++ .../speculation/predictor/mock/BUILD.bazel | 13 + .../predictor/mock/predictor_mock.go | 97 +++++++ .../speculation/predictor/predictor.go | 56 ++++ 7 files changed, 644 insertions(+) create mode 100644 submitqueue/extension/speculation/predictor/BUILD.bazel create mode 100644 submitqueue/extension/speculation/predictor/evidence/BUILD.bazel create mode 100644 submitqueue/extension/speculation/predictor/evidence/evidence.go create mode 100644 submitqueue/extension/speculation/predictor/evidence/evidence_test.go create mode 100644 submitqueue/extension/speculation/predictor/mock/BUILD.bazel create mode 100644 submitqueue/extension/speculation/predictor/mock/predictor_mock.go create mode 100644 submitqueue/extension/speculation/predictor/predictor.go diff --git a/submitqueue/extension/speculation/predictor/BUILD.bazel b/submitqueue/extension/speculation/predictor/BUILD.bazel new file mode 100644 index 00000000..fc689f24 --- /dev/null +++ b/submitqueue/extension/speculation/predictor/BUILD.bazel @@ -0,0 +1,9 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["predictor.go"], + importpath = "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor", + visibility = ["//visibility:public"], + deps = ["//submitqueue/entity:go_default_library"], +) diff --git a/submitqueue/extension/speculation/predictor/evidence/BUILD.bazel b/submitqueue/extension/speculation/predictor/evidence/BUILD.bazel new file mode 100644 index 00000000..f0e2eca9 --- /dev/null +++ b/submitqueue/extension/speculation/predictor/evidence/BUILD.bazel @@ -0,0 +1,29 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["evidence.go"], + importpath = "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor/evidence", + visibility = ["//visibility:public"], + deps = [ + "//platform/metrics:go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/speculation/predictor:go_default_library", + "//submitqueue/extension/speculation/scorer:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["evidence_test.go"], + embed = [":go_default_library"], + deps = [ + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/speculation/predictor:go_default_library", + "//submitqueue/extension/speculation/scorer:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + ], +) diff --git a/submitqueue/extension/speculation/predictor/evidence/evidence.go b/submitqueue/extension/speculation/predictor/evidence/evidence.go new file mode 100644 index 00000000..75927a65 --- /dev/null +++ b/submitqueue/extension/speculation/predictor/evidence/evidence.go @@ -0,0 +1,183 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package evidence revises a Scorer's price by multiplying its odds by one +// factor per piece of evidence about the batch's progress. +// +// Odds rather than the probability itself, because a factor then means the same +// thing wherever it applies and the result cannot leave [0, 1]. Written as logs +// and summed, the same arithmetic is a logistic regression, which is what lets +// hand-written factors later be replaced by fitted ones without changing the +// form. See doc/rfc/submitqueue/outcome-predictor.md. +package evidence + +import ( + "fmt" + "math" + + "context" + + "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/metrics" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor" + "github.com/uber/submitqueue/submitqueue/extension/speculation/scorer" +) + +// Factors are the odds multipliers, one per piece of evidence. A factor of 1 +// leaves the price alone. Named fields rather than a keyed map, so an evidence +// name that does not exist fails to compile instead of being ignored. +type Factors struct { + // PathPassed applies once when a build has passed on the batch's + // all-succeed path. + PathPassed float64 + // PathFailed applies once per failed all-succeed path, compounding. + PathFailed float64 + // Merging applies while the batch is merging. + Merging float64 + // Cancelling applies while the batch is cancelling. + Cancelling float64 +} + +// AllOnes is the neutral set: the prediction is the scorer's price. +func AllOnes() Factors { + return Factors{PathPassed: 1, PathFailed: 1, Merging: 1, Cancelling: 1} +} + +// epsilon bounds the price away from 0 and 1, which have no finite odds. +// Without it a certain scorer could never be revised by any evidence — and +// certainty about an unfinished batch is the scorer overstating what it sees. +const epsilon = 1e-6 + +// evidence is a predictor.Predictor that revises a scorer's price. +type evidence struct { + // cfg is the per-queue identity this predictor was built for. + cfg predictor.Config + // base prices the batch's change; its price is what the factors revise. + base scorer.Scorer + // factors are the odds multipliers applied to that price. + factors Factors + // scope is the tally scope for emitting metrics. + scope tally.Scope +} + +// New creates an evidence predictor bound to the queue named in cfg, revising +// base's price by factors. +// +// It returns an error rather than panic on a nil base or a non-positive factor: +// configuration rejects those today, but the fitted-factor file loader planned +// in doc/rfc/submitqueue/outcome-predictor.md bypasses configuration entirely, +// and on that path this check is the only guard. +func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally.Scope) (predictor.Predictor, error) { + if base == nil { + return nil, fmt.Errorf("evidence.New: base must not be nil") + } + for name, factor := range map[string]float64{ + "PathPassed": factors.PathPassed, + "PathFailed": factors.PathFailed, + "Merging": factors.Merging, + "Cancelling": factors.Cancelling, + } { + // Zero would pin the prediction to 0 and negative has no meaning as a + // multiplier on odds. + if !(factor > 0) { + return nil, fmt.Errorf("evidence.New: factor %s must be positive, got %v", name, factor) + } + } + return &evidence{cfg: cfg, base: base, factors: factors, scope: scope}, nil +} + +// Predict prices the batch's change through the base scorer, then multiplies +// the odds of that price by one factor per piece of evidence. +func (r *evidence) Predict(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (ret predictor.Probability, retErr error) { + op := metrics.Begin(r.scope, "predict", metrics.FastLatencyBuckets) + defer func() { op.Complete(retErr) }() + + price, err := r.base.Score(ctx, batch) + if err != nil { + return 0, err + } + // A price that is not a probability is a broken scorer, not a low opinion of + // the batch. Saying so leaves the caller to fall back on its own default, + // where clamping would hand back a number that looks deliberate. + if !(price >= 0 && price <= 1) { + return 0, fmt.Errorf("base scorer returned %v, which is not a probability", price) + } + + odds := oddsOf(math.Min(math.Max(price, epsilon), 1-epsilon)) + if hasPassedAllSucceedPath(paths) { + odds *= r.factors.PathPassed + } + odds *= math.Pow(r.factors.PathFailed, float64(countFailed(paths))) + switch batch.State { + case entity.BatchStateMerging: + odds *= r.factors.Merging + case entity.BatchStateCancelling: + odds *= r.factors.Cancelling + } + return probabilityOf(odds), nil +} + +// oddsOf converts a probability to odds. p is bounded away from 1, so this is +// finite. +func oddsOf(p float64) float64 { + return p / (1 - p) +} + +// probabilityOf converts odds back to a probability. Overflowed odds read as +// certainty rather than the NaN the division would produce. +func probabilityOf(odds float64) predictor.Probability { + if math.IsInf(odds, 1) { + return 1 + } + return predictor.Probability(odds / (1 + odds)) +} + +// hasPassedAllSucceedPath reports a passed build on the batch's all-succeed +// path. Only that path counts: one built without a dependency's changes says +// nothing about a candidate that assumes the dependency lands. +func hasPassedAllSucceedPath(paths entity.SpeculationPathSet) bool { + for _, entry := range paths.Paths { + if entry.Status != entity.SpeculationPathStatusPassed { + continue + } + if assumesAllSucceed(entry.Path) { + return true + } + } + return false +} + +// assumesAllSucceed reports whether every dependency is assumed to succeed. +func assumesAllSucceed(path entity.SpeculationPath) bool { + for _, dep := range path.Dependencies { + if dep.Assumption != entity.DependencyAssumptionSucceeds { + return false + } + } + return true +} + +// countFailed counts failed builds on the batch's all-succeed path; each one +// compounds. Flip-subset failures are ignored: they were built under different +// assumptions, the same filter PathPassed uses. +func countFailed(paths entity.SpeculationPathSet) int { + failed := 0 + for _, entry := range paths.Paths { + if entry.Status == entity.SpeculationPathStatusFailed && assumesAllSucceed(entry.Path) { + failed++ + } + } + return failed +} diff --git a/submitqueue/extension/speculation/predictor/evidence/evidence_test.go b/submitqueue/extension/speculation/predictor/evidence/evidence_test.go new file mode 100644 index 00000000..25033cba --- /dev/null +++ b/submitqueue/extension/speculation/predictor/evidence/evidence_test.go @@ -0,0 +1,257 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package evidence + +import ( + "context" + "fmt" + "math" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor" + "github.com/uber/submitqueue/submitqueue/extension/speculation/scorer" +) + +// testCfg is the per-queue identity used by every case in this file. +var testCfg = predictor.Config{QueueName: "test-queue"} + +// fixedScorer always returns the same price. +type fixedScorer struct{ price float64 } + +func (f fixedScorer) Score(_ context.Context, _ entity.Batch) (float64, error) { + return f.price, nil +} + +// errorScorer always fails. +type errorScorer struct{} + +func (errorScorer) Score(_ context.Context, _ entity.Batch) (float64, error) { + return 0, fmt.Errorf("scorer failed") +} + +// pathSet builds a set whose entries carry the given statuses, every path +// assuming all of its dependencies succeed. +func pathSet(statuses ...entity.SpeculationPathStatus) entity.SpeculationPathSet { + set := entity.SpeculationPathSet{Queue: "q", Head: "q/batch/1"} + for i, status := range statuses { + set.Paths = append(set.Paths, entity.SpeculationPathEntry{ + ID: fmt.Sprintf("path-%d", i), + Status: status, + Path: entity.SpeculationPath{ + Head: "q/batch/1", + Dependencies: []entity.PathDependency{{Batch: "q/batch/0", Assumption: entity.DependencyAssumptionSucceeds}}, + }, + }) + } + return set +} + +// predict runs one prediction with all-neutral factors except those overridden. +func predict(t *testing.T, price float64, factors Factors, batch entity.Batch, paths entity.SpeculationPathSet) float64 { + t.Helper() + p, err := New(testCfg, fixedScorer{price: price}, factors, tally.NoopScope) + require.NoError(t, err) + got, err := p.Predict(context.Background(), batch, paths) + require.NoError(t, err) + return float64(got) +} + +func TestPredict_NeutralFactorsReturnTheScorersPrice(t *testing.T) { + for _, price := range []float64{0.01, 0.25, 0.5, 0.6, 0.9, 0.99} { + t.Run(fmt.Sprintf("price %v", price), func(t *testing.T) { + got := predict(t, price, AllOnes(), entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed)) + assert.InDelta(t, price, got, 1e-9) + }) + } +} + +func TestPredict_AppliesOneFactorPerEvidence(t *testing.T) { + // 0.5 has odds of exactly 1, so the resulting odds are the factor itself and + // the expected probability is factor/(1+factor). + tests := []struct { + name string + factors Factors + batch entity.Batch + paths entity.SpeculationPathSet + want float64 + }{ + { + name: "a passed path", + factors: Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1}, + paths: pathSet(entity.SpeculationPathStatusPassed), + want: 0.9, + }, + { + name: "no passed path leaves the price alone", + factors: Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1}, + paths: pathSet(entity.SpeculationPathStatusBuilding), + want: 0.5, + }, + { + name: "one failed path", + factors: Factors{PathPassed: 1, PathFailed: 0.25, Merging: 1, Cancelling: 1}, + paths: pathSet(entity.SpeculationPathStatusFailed), + want: 0.2, + }, + { + name: "failed paths compound", + factors: Factors{PathPassed: 1, PathFailed: 0.5, Merging: 1, Cancelling: 1}, + paths: pathSet(entity.SpeculationPathStatusFailed, entity.SpeculationPathStatusFailed), + want: 0.2, + }, + { + name: "merging", + factors: Factors{PathPassed: 1, PathFailed: 1, Merging: 19, Cancelling: 1}, + batch: entity.Batch{State: entity.BatchStateMerging}, + want: 0.95, + }, + { + name: "cancelling", + factors: Factors{PathPassed: 1, PathFailed: 1, Merging: 1, Cancelling: 0.25}, + batch: entity.Batch{State: entity.BatchStateCancelling}, + want: 0.2, + }, + { + name: "a state with no factor leaves the price alone", + factors: Factors{PathPassed: 1, PathFailed: 1, Merging: 19, Cancelling: 0.25}, + batch: entity.Batch{State: entity.BatchStateSpeculating}, + want: 0.5, + }, + { + name: "evidence compounds across kinds", + factors: Factors{PathPassed: 4, PathFailed: 1, Merging: 3, Cancelling: 1}, + batch: entity.Batch{State: entity.BatchStateMerging}, + paths: pathSet(entity.SpeculationPathStatusPassed), + want: 0.923076923, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.InDelta(t, tt.want, predict(t, 0.5, tt.factors, tt.batch, tt.paths), 1e-9) + }) + } +} + +// A path built without one of its dependencies proves nothing about a candidate +// that assumes the dependency lands, which is what stacking on this batch means. +func TestPredict_IgnoresAPassedPathThatAssumesAFailure(t *testing.T) { + paths := pathSet(entity.SpeculationPathStatusPassed) + paths.Paths[0].Path.Dependencies[0].Assumption = entity.DependencyAssumptionFails + + factors := Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1} + assert.InDelta(t, 0.5, predict(t, 0.5, factors, entity.Batch{}, paths), 1e-9) +} + +// A failed flip-subset must not drag down a green all-succeed build: it was +// built under different assumptions, the same filter PathPassed uses. +func TestPredict_IgnoresAFailedPathThatAssumesAFailure(t *testing.T) { + paths := pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed) + paths.Paths[1].Path.Dependencies[0].Assumption = entity.DependencyAssumptionFails + + factors := Factors{PathPassed: 9, PathFailed: 0.25, Merging: 1, Cancelling: 1} + assert.InDelta(t, 0.9, predict(t, 0.5, factors, entity.Batch{}, paths), 1e-9) +} + +func TestPredict_APathWithNoDependenciesCounts(t *testing.T) { + paths := pathSet(entity.SpeculationPathStatusPassed) + paths.Paths[0].Path.Dependencies = nil + + factors := Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1} + assert.InDelta(t, 0.9, predict(t, 0.5, factors, entity.Batch{}, paths), 1e-9) +} + +func TestPredict_AFailedPathWithNoDependenciesCounts(t *testing.T) { + paths := pathSet(entity.SpeculationPathStatusFailed) + paths.Paths[0].Path.Dependencies = nil + + factors := Factors{PathPassed: 1, PathFailed: 0.25, Merging: 1, Cancelling: 1} + assert.InDelta(t, 0.2, predict(t, 0.5, factors, entity.Batch{}, paths), 1e-9) +} + +func TestPredict_AnEmptyPathSetIsNoEvidence(t *testing.T) { + factors := Factors{PathPassed: 9, PathFailed: 0.1, Merging: 1, Cancelling: 1} + assert.InDelta(t, 0.5, predict(t, 0.5, factors, entity.Batch{}, entity.SpeculationPathSet{}), 1e-9) +} + +// A scorer certain either way still has to be movable, or no evidence could ever +// revise a price the scorer had no business being certain about. +func TestPredict_CertainPricesStayInRangeAndStillMove(t *testing.T) { + tests := []struct { + name string + price float64 + factor float64 + wantAbove float64 + wantBelow float64 + }{ + {name: "certain success, evidence against", price: 1, factor: 0.5, wantAbove: 0.99, wantBelow: 1}, + {name: "certain failure, evidence for", price: 0, factor: 2, wantAbove: 0, wantBelow: 0.01}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + factors := AllOnes() + factors.PathPassed = tt.factor + got := predict(t, tt.price, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed)) + assert.Greater(t, got, tt.wantAbove) + assert.Less(t, got, tt.wantBelow) + }) + } +} + +func TestPredict_RejectsAPriceThatIsNotAProbability(t *testing.T) { + for _, price := range []float64{-0.1, 1.5, math.NaN()} { + t.Run(fmt.Sprintf("price %v", price), func(t *testing.T) { + p, err := New(testCfg, fixedScorer{price: price}, AllOnes(), tally.NoopScope) + require.NoError(t, err) + _, err = p.Predict(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) + require.Error(t, err) + }) + } +} + +func TestPredict_PropagatesAScorerError(t *testing.T) { + p, err := New(testCfg, errorScorer{}, AllOnes(), tally.NoopScope) + require.NoError(t, err) + _, err = p.Predict(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) + require.Error(t, err) +} + +func TestNew_RejectsUnusableConstruction(t *testing.T) { + zeroed := AllOnes() + zeroed.Merging = 0 + negative := AllOnes() + negative.PathFailed = -1 + + tests := []struct { + name string + base scorer.Scorer + factors Factors + }{ + {name: "nil base", base: nil, factors: AllOnes()}, + {name: "zero factor", base: fixedScorer{price: 0.5}, factors: zeroed}, + {name: "negative factor", base: fixedScorer{price: 0.5}, factors: negative}, + {name: "unset factors", base: fixedScorer{price: 0.5}, factors: Factors{}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p, err := New(testCfg, tt.base, tt.factors, tally.NoopScope) + require.Error(t, err) + assert.Nil(t, p) + }) + } +} diff --git a/submitqueue/extension/speculation/predictor/mock/BUILD.bazel b/submitqueue/extension/speculation/predictor/mock/BUILD.bazel new file mode 100644 index 00000000..fd84517d --- /dev/null +++ b/submitqueue/extension/speculation/predictor/mock/BUILD.bazel @@ -0,0 +1,13 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["predictor_mock.go"], + importpath = "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor/mock", + visibility = ["//visibility:public"], + deps = [ + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/speculation/predictor:go_default_library", + "@org_uber_go_mock//gomock:go_default_library", + ], +) diff --git a/submitqueue/extension/speculation/predictor/mock/predictor_mock.go b/submitqueue/extension/speculation/predictor/mock/predictor_mock.go new file mode 100644 index 00000000..01088820 --- /dev/null +++ b/submitqueue/extension/speculation/predictor/mock/predictor_mock.go @@ -0,0 +1,97 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: predictor.go +// +// Generated by this command: +// +// mockgen -source=predictor.go -destination=mock/predictor_mock.go -package=mock +// + +// Package mock is a generated GoMock package. +package mock + +import ( + context "context" + reflect "reflect" + + entity "github.com/uber/submitqueue/submitqueue/entity" + predictor "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor" + gomock "go.uber.org/mock/gomock" +) + +// MockPredictor is a mock of Predictor interface. +type MockPredictor struct { + ctrl *gomock.Controller + recorder *MockPredictorMockRecorder + isgomock struct{} +} + +// MockPredictorMockRecorder is the mock recorder for MockPredictor. +type MockPredictorMockRecorder struct { + mock *MockPredictor +} + +// NewMockPredictor creates a new mock instance. +func NewMockPredictor(ctrl *gomock.Controller) *MockPredictor { + mock := &MockPredictor{ctrl: ctrl} + mock.recorder = &MockPredictorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockPredictor) EXPECT() *MockPredictorMockRecorder { + return m.recorder +} + +// Predict mocks base method. +func (m *MockPredictor) Predict(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (predictor.Probability, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Predict", ctx, batch, paths) + ret0, _ := ret[0].(predictor.Probability) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Predict indicates an expected call of Predict. +func (mr *MockPredictorMockRecorder) Predict(ctx, batch, paths any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Predict", reflect.TypeOf((*MockPredictor)(nil).Predict), ctx, batch, paths) +} + +// MockFactory is a mock of Factory interface. +type MockFactory struct { + ctrl *gomock.Controller + recorder *MockFactoryMockRecorder + isgomock struct{} +} + +// MockFactoryMockRecorder is the mock recorder for MockFactory. +type MockFactoryMockRecorder struct { + mock *MockFactory +} + +// NewMockFactory creates a new mock instance. +func NewMockFactory(ctrl *gomock.Controller) *MockFactory { + mock := &MockFactory{ctrl: ctrl} + mock.recorder = &MockFactoryMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockFactory) EXPECT() *MockFactoryMockRecorder { + return m.recorder +} + +// For mocks base method. +func (m *MockFactory) For(cfg predictor.Config) (predictor.Predictor, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "For", cfg) + ret0, _ := ret[0].(predictor.Predictor) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// For indicates an expected call of For. +func (mr *MockFactoryMockRecorder) For(cfg any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "For", reflect.TypeOf((*MockFactory)(nil).For), cfg) +} diff --git a/submitqueue/extension/speculation/predictor/predictor.go b/submitqueue/extension/speculation/predictor/predictor.go new file mode 100644 index 00000000..66b57c01 --- /dev/null +++ b/submitqueue/extension/speculation/predictor/predictor.go @@ -0,0 +1,56 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package predictor defines how likely a batch is to succeed, given both what +// it changes and what has happened to it so far. A Scorer prices the change; a +// Predictor is built over one and revises its price with the batch's observed +// progress. +package predictor + +//go:generate mockgen -source=predictor.go -destination=mock/predictor_mock.go -package=mock + +import ( + "context" + + "github.com/uber/submitqueue/submitqueue/entity" +) + +// Probability is how likely an outcome is, from 0.0 to 1.0. +type Probability float64 + +// Predictor estimates a batch's final outcome. +type Predictor interface { + // Predict returns how likely the batch is to reach Succeeded with its + // changes landed. A passing build is necessary but not sufficient. + // + // paths is the batch's own build progress, zero-valued for a batch nothing + // has speculated on. Callers may predict every batch a queue waits on, so + // anything expensive belongs behind the implementation's own cache. + Predict(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (Probability, error) +} + +// Config carries the per-queue identity handed to a Factory. The system knows +// only the queue name; everything an implementation needs is injected at +// construction by the integrator. +type Config struct { + // QueueName identifies the queue this Predictor serves. + QueueName string +} + +// Factory builds the Predictor for a queue. Implementations inject what they +// need at construction, including the Scorer whose price they revise. +type Factory interface { + // For returns the Predictor for the given queue. + For(cfg Config) (Predictor, error) +} From 5407234fc02c6dc45ae41319258fc5f91e0f0c76 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 7 Sep 2026 17:12:09 -0700 Subject: [PATCH 2/8] refactor(speculation): express prediction in factor terms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The evidence implementation still described the model as logistic regression over odds and referenced fitting work that the RFC no longer proposes. That made the code's vocabulary diverge from the factor contract exposed to operators. ### What? Combine the applicable evidence factors first and revise the scorer price with the equivalent bounded formula. Remove stale fitting rationale and keep implementation and tests in the RFC's scorer-price and factor terminology without changing behavior. ## Test Plan - ✅ `./tool/bazel test //submitqueue/extension/speculation/predictor/...` --- .../predictor/evidence/evidence.go | 60 +++++++------------ .../predictor/evidence/evidence_test.go | 3 +- 2 files changed, 21 insertions(+), 42 deletions(-) diff --git a/submitqueue/extension/speculation/predictor/evidence/evidence.go b/submitqueue/extension/speculation/predictor/evidence/evidence.go index 75927a65..6b4e38c2 100644 --- a/submitqueue/extension/speculation/predictor/evidence/evidence.go +++ b/submitqueue/extension/speculation/predictor/evidence/evidence.go @@ -12,14 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package evidence revises a Scorer's price by multiplying its odds by one -// factor per piece of evidence about the batch's progress. -// -// Odds rather than the probability itself, because a factor then means the same -// thing wherever it applies and the result cannot leave [0, 1]. Written as logs -// and summed, the same arithmetic is a logistic regression, which is what lets -// hand-written factors later be replaced by fitted ones without changing the -// form. See doc/rfc/submitqueue/outcome-predictor.md. +// Package evidence revises a Scorer's price with factors for observed batch +// progress. See doc/rfc/submitqueue/outcome-predictor.md. package evidence import ( @@ -35,9 +29,8 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/speculation/scorer" ) -// Factors are the odds multipliers, one per piece of evidence. A factor of 1 -// leaves the price alone. Named fields rather than a keyed map, so an evidence -// name that does not exist fails to compile instead of being ignored. +// Factors revise the scorer's price, one per piece of evidence. A factor of 1 +// leaves the price alone. Named fields make unknown evidence fail to compile. type Factors struct { // PathPassed applies once when a build has passed on the batch's // all-succeed path. @@ -55,9 +48,7 @@ func AllOnes() Factors { return Factors{PathPassed: 1, PathFailed: 1, Merging: 1, Cancelling: 1} } -// epsilon bounds the price away from 0 and 1, which have no finite odds. -// Without it a certain scorer could never be revised by any evidence — and -// certainty about an unfinished batch is the scorer overstating what it sees. +// epsilon keeps exact certainty revisable while remaining close to the scorer. const epsilon = 1e-6 // evidence is a predictor.Predictor that revises a scorer's price. @@ -66,7 +57,7 @@ type evidence struct { cfg predictor.Config // base prices the batch's change; its price is what the factors revise. base scorer.Scorer - // factors are the odds multipliers applied to that price. + // factors revise the scorer's price with observed evidence. factors Factors // scope is the tally scope for emitting metrics. scope tally.Scope @@ -75,10 +66,7 @@ type evidence struct { // New creates an evidence predictor bound to the queue named in cfg, revising // base's price by factors. // -// It returns an error rather than panic on a nil base or a non-positive factor: -// configuration rejects those today, but the fitted-factor file loader planned -// in doc/rfc/submitqueue/outcome-predictor.md bypasses configuration entirely, -// and on that path this check is the only guard. +// It rejects a nil base and non-positive factors. func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally.Scope) (predictor.Predictor, error) { if base == nil { return nil, fmt.Errorf("evidence.New: base must not be nil") @@ -89,8 +77,8 @@ func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally. "Merging": factors.Merging, "Cancelling": factors.Cancelling, } { - // Zero would pin the prediction to 0 and negative has no meaning as a - // multiplier on odds. + // Zero would permanently pin matching batches to 0; negatives cannot + // represent either direction in the factor contract. if !(factor > 0) { return nil, fmt.Errorf("evidence.New: factor %s must be positive, got %v", name, factor) } @@ -98,8 +86,8 @@ func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally. return &evidence{cfg: cfg, base: base, factors: factors, scope: scope}, nil } -// Predict prices the batch's change through the base scorer, then multiplies -// the odds of that price by one factor per piece of evidence. +// Predict prices the batch's change, combines its evidence factors, and revises +// the scorer's price with the result. func (r *evidence) Predict(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (ret predictor.Probability, retErr error) { op := metrics.Begin(r.scope, "predict", metrics.FastLatencyBuckets) defer func() { op.Complete(retErr) }() @@ -115,33 +103,25 @@ func (r *evidence) Predict(ctx context.Context, batch entity.Batch, paths entity return 0, fmt.Errorf("base scorer returned %v, which is not a probability", price) } - odds := oddsOf(math.Min(math.Max(price, epsilon), 1-epsilon)) + factor := math.Pow(r.factors.PathFailed, float64(countFailed(paths))) if hasPassedAllSucceedPath(paths) { - odds *= r.factors.PathPassed + factor *= r.factors.PathPassed } - odds *= math.Pow(r.factors.PathFailed, float64(countFailed(paths))) switch batch.State { case entity.BatchStateMerging: - odds *= r.factors.Merging + factor *= r.factors.Merging case entity.BatchStateCancelling: - odds *= r.factors.Cancelling + factor *= r.factors.Cancelling } - return probabilityOf(odds), nil -} - -// oddsOf converts a probability to odds. p is bounded away from 1, so this is -// finite. -func oddsOf(p float64) float64 { - return p / (1 - p) + return revise(math.Min(math.Max(price, epsilon), 1-epsilon), factor), nil } -// probabilityOf converts odds back to a probability. Overflowed odds read as -// certainty rather than the NaN the division would produce. -func probabilityOf(odds float64) predictor.Probability { - if math.IsInf(odds, 1) { +// revise applies the combined factor while keeping the result a probability. +func revise(price, factor float64) predictor.Probability { + if math.IsInf(factor, 1) { return 1 } - return predictor.Probability(odds / (1 + odds)) + return predictor.Probability(price * factor / (1 - price + price*factor)) } // hasPassedAllSucceedPath reports a passed build on the batch's all-succeed diff --git a/submitqueue/extension/speculation/predictor/evidence/evidence_test.go b/submitqueue/extension/speculation/predictor/evidence/evidence_test.go index 25033cba..18c16ae5 100644 --- a/submitqueue/extension/speculation/predictor/evidence/evidence_test.go +++ b/submitqueue/extension/speculation/predictor/evidence/evidence_test.go @@ -82,8 +82,7 @@ func TestPredict_NeutralFactorsReturnTheScorersPrice(t *testing.T) { } func TestPredict_AppliesOneFactorPerEvidence(t *testing.T) { - // 0.5 has odds of exactly 1, so the resulting odds are the factor itself and - // the expected probability is factor/(1+factor). + // At scorer price 0.5, factor f revises the price to f/(1+f). tests := []struct { name string factors Factors From 1eb718ac67c20e52579d4c79fde3433a82502c65 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 7 Sep 2026 17:28:02 -0700 Subject: [PATCH 3/8] fix(speculation): preserve predictor factor contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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/...` --- .../predictor/evidence/evidence.go | 33 +++++++++++-------- .../predictor/evidence/evidence_test.go | 22 ++++++++----- 2 files changed, 33 insertions(+), 22 deletions(-) diff --git a/submitqueue/extension/speculation/predictor/evidence/evidence.go b/submitqueue/extension/speculation/predictor/evidence/evidence.go index 6b4e38c2..52eac912 100644 --- a/submitqueue/extension/speculation/predictor/evidence/evidence.go +++ b/submitqueue/extension/speculation/predictor/evidence/evidence.go @@ -35,7 +35,7 @@ type Factors struct { // PathPassed applies once when a build has passed on the batch's // all-succeed path. PathPassed float64 - // PathFailed applies once per failed all-succeed path, compounding. + // PathFailed applies once when the all-succeed path has failed. PathFailed float64 // Merging applies while the batch is merging. Merging float64 @@ -66,7 +66,7 @@ type evidence struct { // New creates an evidence predictor bound to the queue named in cfg, revising // base's price by factors. // -// It rejects a nil base and non-positive factors. +// It rejects a nil base and factors that are non-finite or not positive. func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally.Scope) (predictor.Predictor, error) { if base == nil { 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. } { // Zero would permanently pin matching batches to 0; negatives cannot // represent either direction in the factor contract. - if !(factor > 0) { - return nil, fmt.Errorf("evidence.New: factor %s must be positive, got %v", name, factor) + if !(factor > 0) || math.IsInf(factor, 0) { + return nil, fmt.Errorf("evidence.New: factor %s must be finite and positive, got %v", name, factor) } } 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 return 0, fmt.Errorf("base scorer returned %v, which is not a probability", price) } - factor := math.Pow(r.factors.PathFailed, float64(countFailed(paths))) + factor := 1.0 if hasPassedAllSucceedPath(paths) { factor *= r.factors.PathPassed } + if hasFailedAllSucceedPath(paths) { + factor *= r.factors.PathFailed + } switch batch.State { case entity.BatchStateMerging: factor *= r.factors.Merging case entity.BatchStateCancelling: factor *= r.factors.Cancelling } + if factor == 1 { + return predictor.Probability(price), nil + } return revise(math.Min(math.Max(price, epsilon), 1-epsilon), factor), nil } // revise applies the combined factor while keeping the result a probability. func revise(price, factor float64) predictor.Probability { if math.IsInf(factor, 1) { - return 1 + return 1 - epsilon } - return predictor.Probability(price * factor / (1 - price + price*factor)) + revised := price * factor / (1 - price + price*factor) + return predictor.Probability(math.Min(math.Max(revised, epsilon), 1-epsilon)) } // hasPassedAllSucceedPath reports a passed build on the batch's all-succeed @@ -149,15 +156,13 @@ func assumesAllSucceed(path entity.SpeculationPath) bool { return true } -// countFailed counts failed builds on the batch's all-succeed path; each one -// compounds. Flip-subset failures are ignored: they were built under different -// assumptions, the same filter PathPassed uses. -func countFailed(paths entity.SpeculationPathSet) int { - failed := 0 +// hasFailedAllSucceedPath reports a failed build on the batch's all-succeed +// path. Flip-subset failures were built under different assumptions. +func hasFailedAllSucceedPath(paths entity.SpeculationPathSet) bool { for _, entry := range paths.Paths { if entry.Status == entity.SpeculationPathStatusFailed && assumesAllSucceed(entry.Path) { - failed++ + return true } } - return failed + return false } diff --git a/submitqueue/extension/speculation/predictor/evidence/evidence_test.go b/submitqueue/extension/speculation/predictor/evidence/evidence_test.go index 18c16ae5..a1222fdc 100644 --- a/submitqueue/extension/speculation/predictor/evidence/evidence_test.go +++ b/submitqueue/extension/speculation/predictor/evidence/evidence_test.go @@ -73,10 +73,10 @@ func predict(t *testing.T, price float64, factors Factors, batch entity.Batch, p } func TestPredict_NeutralFactorsReturnTheScorersPrice(t *testing.T) { - for _, price := range []float64{0.01, 0.25, 0.5, 0.6, 0.9, 0.99} { + for _, price := range []float64{0, 0.01, 0.25, 0.5, 0.6, 0.9, 0.99, 1} { t.Run(fmt.Sprintf("price %v", price), func(t *testing.T) { got := predict(t, price, AllOnes(), entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed)) - assert.InDelta(t, price, got, 1e-9) + assert.Equal(t, price, got) }) } } @@ -108,12 +108,6 @@ func TestPredict_AppliesOneFactorPerEvidence(t *testing.T) { paths: pathSet(entity.SpeculationPathStatusFailed), want: 0.2, }, - { - name: "failed paths compound", - factors: Factors{PathPassed: 1, PathFailed: 0.5, Merging: 1, Cancelling: 1}, - paths: pathSet(entity.SpeculationPathStatusFailed, entity.SpeculationPathStatusFailed), - want: 0.2, - }, { name: "merging", factors: Factors{PathPassed: 1, PathFailed: 1, Merging: 19, Cancelling: 1}, @@ -212,6 +206,15 @@ func TestPredict_CertainPricesStayInRangeAndStillMove(t *testing.T) { } } +func TestPredict_LargeFactorsDoNotProduceCertainty(t *testing.T) { + factors := AllOnes() + factors.PathPassed = math.MaxFloat64 + + got := predict(t, 0.5, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed)) + assert.Greater(t, got, 0.99) + assert.Less(t, got, 1.0) +} + func TestPredict_RejectsAPriceThatIsNotAProbability(t *testing.T) { for _, price := range []float64{-0.1, 1.5, math.NaN()} { t.Run(fmt.Sprintf("price %v", price), func(t *testing.T) { @@ -235,6 +238,8 @@ func TestNew_RejectsUnusableConstruction(t *testing.T) { zeroed.Merging = 0 negative := AllOnes() negative.PathFailed = -1 + infinite := AllOnes() + infinite.PathPassed = math.Inf(1) tests := []struct { name string @@ -244,6 +249,7 @@ func TestNew_RejectsUnusableConstruction(t *testing.T) { {name: "nil base", base: nil, factors: AllOnes()}, {name: "zero factor", base: fixedScorer{price: 0.5}, factors: zeroed}, {name: "negative factor", base: fixedScorer{price: 0.5}, factors: negative}, + {name: "infinite factor", base: fixedScorer{price: 0.5}, factors: infinite}, {name: "unset factors", base: fixedScorer{price: 0.5}, factors: Factors{}}, } for _, tt := range tests { From cc893627c8850858fe68b664eb0818c671f01283 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 7 Sep 2026 17:56:27 -0700 Subject: [PATCH 4/8] docs(speculation): document predictor extension Add the predictor package guide and clarify how it composes over the scorer without adding path evidence to the scorer contract. --- .../extension/speculation/predictor/README.md | 17 +++++++++++++++++ .../extension/speculation/scorer/README.md | 2 ++ 2 files changed, 19 insertions(+) create mode 100644 submitqueue/extension/speculation/predictor/README.md diff --git a/submitqueue/extension/speculation/predictor/README.md b/submitqueue/extension/speculation/predictor/README.md new file mode 100644 index 00000000..b32c0851 --- /dev/null +++ b/submitqueue/extension/speculation/predictor/README.md @@ -0,0 +1,17 @@ +# predictor + +A `Predictor` returns how likely a batch is to reach `Succeeded` with its changes landed, given both what it changes and what this speculate run has already observed. It is built over the queue's `Scorer`, which prices the change from content signals; the predictor revises that price with path-set evidence and batch state. + +`Predict` is handed the batch identity and that batch's own `SpeculationPathSet` — zero-valued when nothing has speculated on it yet. Callers may predict every unresolved dependency a queue waits on, so anything expensive belongs behind the implementation's own cache. + +Like the other extensions, a `Predictor` is selected **per queue** by the wiring layer through the `Config` (queue name) and `Factory` interface. The default `standard` `Speculator` composes its `Generator` over the queue's predictor, which in turn composes over the queue's scorer. + +See [doc/rfc/submitqueue/outcome-predictor.md](../../../../doc/rfc/submitqueue/outcome-predictor.md) for the factor contract, evidence rules, and configuration shape. + +## Implementations + +**`evidence`** revises the scorer's price with YAML-configured factors for `pathPassed`, `pathFailed`, `merging`, and `cancelling`. A factor of `1` leaves the scorer's price alone; every factor defaults to `1` until someone sets one. Only paths that assume every dependency succeeds count as path evidence. + +## Adding a backend + +Create a package under `predictor//` whose `New(...)` returns a `predictor.Predictor`, injecting whatever it needs at construction — typically the queue's `Scorer`, factor configuration, and a metrics scope. Do not add a `Config` or `Factory` implementation here; per-queue routing and the factory adapter live in the wiring layer. diff --git a/submitqueue/extension/speculation/scorer/README.md b/submitqueue/extension/speculation/scorer/README.md index d8b63693..4d70ed15 100644 --- a/submitqueue/extension/speculation/scorer/README.md +++ b/submitqueue/extension/speculation/scorer/README.md @@ -4,6 +4,8 @@ A `Scorer` returns the probability that a batch ultimately succeeds — reaches Callers may score every batch a queue is waiting on, so implementations should be cheap. A speculation run scores each batch at most once, but it does not carry results across runs; anything expensive belongs behind the implementation's own cache. +The default speculation pipeline does not rank on the scorer directly. The queue's `Predictor` is built over its `Scorer` and revises the scorer's price with path-set evidence before `bestfirst` ranks paths. The scorer still prices only the change; it does not see path sets. + Like the other extensions, a `Scorer` is selected **per queue** by the wiring layer through the `Config` (queue name) and `Factory` interface. ## Implementations From feeb03f7369bf9dffb580972cd8400a71f26828a Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Fri, 11 Sep 2026 09:39:30 -0700 Subject: [PATCH 5/8] feat(speculation): fold predictor into Scorer Score takes the path-set snapshot. Evidence is a scorer wrapping a base; heuristic and composite ignore paths. Delete the sibling Predictor factory. # Conflicts: # submitqueue/extension/speculation/scorer/scorer.go # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto 2de54998 # Last commands done (5 commands done): # pick ff043d9f # docs(speculation): document predictor extension # pick 0ff6ea72 # feat(speculation): fold predictor into Scorer # No commands remaining. # You are currently rebasing branch 'preetam/outcome-predictor' on '2de54998'. # # Changes to be committed: # modified: submitqueue/extension/speculation/generator/bestfirst/bestfirst.go # modified: submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go # deleted: submitqueue/extension/speculation/predictor/BUILD.bazel # deleted: submitqueue/extension/speculation/predictor/README.md # deleted: submitqueue/extension/speculation/predictor/mock/BUILD.bazel # deleted: submitqueue/extension/speculation/predictor/mock/predictor_mock.go # deleted: submitqueue/extension/speculation/predictor/predictor.go # modified: submitqueue/extension/speculation/scorer/README.md # modified: submitqueue/extension/speculation/scorer/composite/scorer.go # modified: submitqueue/extension/speculation/scorer/composite/scorer_test.go # renamed: submitqueue/extension/speculation/predictor/evidence/BUILD.bazel -> submitqueue/extension/speculation/scorer/evidence/BUILD.bazel # renamed: submitqueue/extension/speculation/predictor/evidence/evidence.go -> submitqueue/extension/speculation/scorer/evidence/evidence.go # renamed: submitqueue/extension/speculation/predictor/evidence/evidence_test.go -> submitqueue/extension/speculation/scorer/evidence/evidence_test.go # modified: submitqueue/extension/speculation/scorer/fake/fake.go # modified: submitqueue/extension/speculation/scorer/fake/fake_test.go # modified: submitqueue/extension/speculation/scorer/heuristic/scorer.go # modified: submitqueue/extension/speculation/scorer/heuristic/scorer_test.go # modified: submitqueue/extension/speculation/scorer/mock/scorer_mock.go # modified: submitqueue/extension/speculation/scorer/scorer.go # modified: submitqueue/extension/speculation/speculator/standard/standard_test.go # --- .../generator/bestfirst/bestfirst.go | 2 +- .../generator/bestfirst/bestfirst_test.go | 10 +- .../speculation/predictor/BUILD.bazel | 9 -- .../extension/speculation/predictor/README.md | 17 ---- .../speculation/predictor/mock/BUILD.bazel | 13 --- .../predictor/mock/predictor_mock.go | 97 ------------------- .../speculation/predictor/predictor.go | 56 ----------- .../extension/speculation/scorer/README.md | 14 ++- .../speculation/scorer/composite/scorer.go | 4 +- .../scorer/composite/scorer_test.go | 10 +- .../evidence/BUILD.bazel | 4 +- .../evidence/evidence.go | 35 ++++--- .../evidence/evidence_test.go | 69 +++++++------ .../extension/speculation/scorer/fake/fake.go | 4 +- .../speculation/scorer/fake/fake_test.go | 4 +- .../speculation/scorer/heuristic/scorer.go | 2 +- .../scorer/heuristic/scorer_test.go | 4 +- .../speculation/scorer/mock/scorer_mock.go | 8 +- .../extension/speculation/scorer/scorer.go | 21 ++-- .../speculator/standard/standard_test.go | 2 +- 20 files changed, 92 insertions(+), 293 deletions(-) delete mode 100644 submitqueue/extension/speculation/predictor/BUILD.bazel delete mode 100644 submitqueue/extension/speculation/predictor/README.md delete mode 100644 submitqueue/extension/speculation/predictor/mock/BUILD.bazel delete mode 100644 submitqueue/extension/speculation/predictor/mock/predictor_mock.go delete mode 100644 submitqueue/extension/speculation/predictor/predictor.go rename submitqueue/extension/speculation/{predictor => scorer}/evidence/BUILD.bazel (83%) rename submitqueue/extension/speculation/{predictor => scorer}/evidence/evidence.go (78%) rename submitqueue/extension/speculation/{predictor => scorer}/evidence/evidence_test.go (73%) diff --git a/submitqueue/extension/speculation/generator/bestfirst/bestfirst.go b/submitqueue/extension/speculation/generator/bestfirst/bestfirst.go index 97a70275..43965d81 100644 --- a/submitqueue/extension/speculation/generator/bestfirst/bestfirst.go +++ b/submitqueue/extension/speculation/generator/bestfirst/bestfirst.go @@ -121,7 +121,7 @@ func (g *bestFirst) score(ctx context.Context, ids []string, batchByID map[strin probabilityByID[id] = defaultProbability continue } - probability, err := g.scorer.Score(ctx, batch) + probability, err := g.scorer.Score(ctx, batch, entity.SpeculationPathSet{}) if err != nil { // A scorer that failed because the caller went away has not found // an unpriceable dependency — it has found a dead ctx, which ends diff --git a/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go b/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go index 5f0a5d56..4942dee7 100644 --- a/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go +++ b/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go @@ -38,7 +38,7 @@ type stubScorer struct { scores map[string]float64 } -func (s stubScorer) Score(_ context.Context, b entity.Batch) (float64, error) { +func (s stubScorer) Score(_ context.Context, b entity.Batch, _ entity.SpeculationPathSet) (float64, error) { if v, ok := s.scores[b.ID]; ok { return v, nil } @@ -136,7 +136,7 @@ func newCountingScorer(scores map[string]float64) *countingScorer { return &countingScorer{scores: scores, calls: map[string]int{}} } -func (c *countingScorer) Score(_ context.Context, b entity.Batch) (float64, error) { +func (c *countingScorer) Score(_ context.Context, b entity.Batch, _ entity.SpeculationPathSet) (float64, error) { c.calls[b.ID]++ c.total++ if v, ok := c.scores[b.ID]; ok { @@ -148,14 +148,14 @@ func (c *countingScorer) Score(_ context.Context, b entity.Batch) (float64, erro // errScorer always fails, to exercise error propagation from scoring. type errScorer struct{} -func (errScorer) Score(context.Context, entity.Batch) (float64, error) { +func (errScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) (float64, error) { return 0, assert.AnError } // constScorer scores every batch identically, regardless of ID. type constScorer struct{ v float64 } -func (c constScorer) Score(context.Context, entity.Batch) (float64, error) { return c.v, nil } +func (c constScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) (float64, error) { return c.v, nil } // wideHead builds one Speculating head over n unresolved dependencies, each at a // distinct score so no two combinations tie. @@ -839,7 +839,7 @@ func TestBestFirst_HonorsCancelledContext(t *testing.T) { // own call was cancelled would. type cancellingScorer struct{ cancel context.CancelFunc } -func (s cancellingScorer) Score(context.Context, entity.Batch) (float64, error) { +func (s cancellingScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) (float64, error) { s.cancel() return 0, context.Canceled } diff --git a/submitqueue/extension/speculation/predictor/BUILD.bazel b/submitqueue/extension/speculation/predictor/BUILD.bazel deleted file mode 100644 index fc689f24..00000000 --- a/submitqueue/extension/speculation/predictor/BUILD.bazel +++ /dev/null @@ -1,9 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library") - -go_library( - name = "go_default_library", - srcs = ["predictor.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor", - visibility = ["//visibility:public"], - deps = ["//submitqueue/entity:go_default_library"], -) diff --git a/submitqueue/extension/speculation/predictor/README.md b/submitqueue/extension/speculation/predictor/README.md deleted file mode 100644 index b32c0851..00000000 --- a/submitqueue/extension/speculation/predictor/README.md +++ /dev/null @@ -1,17 +0,0 @@ -# predictor - -A `Predictor` returns how likely a batch is to reach `Succeeded` with its changes landed, given both what it changes and what this speculate run has already observed. It is built over the queue's `Scorer`, which prices the change from content signals; the predictor revises that price with path-set evidence and batch state. - -`Predict` is handed the batch identity and that batch's own `SpeculationPathSet` — zero-valued when nothing has speculated on it yet. Callers may predict every unresolved dependency a queue waits on, so anything expensive belongs behind the implementation's own cache. - -Like the other extensions, a `Predictor` is selected **per queue** by the wiring layer through the `Config` (queue name) and `Factory` interface. The default `standard` `Speculator` composes its `Generator` over the queue's predictor, which in turn composes over the queue's scorer. - -See [doc/rfc/submitqueue/outcome-predictor.md](../../../../doc/rfc/submitqueue/outcome-predictor.md) for the factor contract, evidence rules, and configuration shape. - -## Implementations - -**`evidence`** revises the scorer's price with YAML-configured factors for `pathPassed`, `pathFailed`, `merging`, and `cancelling`. A factor of `1` leaves the scorer's price alone; every factor defaults to `1` until someone sets one. Only paths that assume every dependency succeeds count as path evidence. - -## Adding a backend - -Create a package under `predictor//` whose `New(...)` returns a `predictor.Predictor`, injecting whatever it needs at construction — typically the queue's `Scorer`, factor configuration, and a metrics scope. Do not add a `Config` or `Factory` implementation here; per-queue routing and the factory adapter live in the wiring layer. diff --git a/submitqueue/extension/speculation/predictor/mock/BUILD.bazel b/submitqueue/extension/speculation/predictor/mock/BUILD.bazel deleted file mode 100644 index fd84517d..00000000 --- a/submitqueue/extension/speculation/predictor/mock/BUILD.bazel +++ /dev/null @@ -1,13 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library") - -go_library( - name = "go_default_library", - srcs = ["predictor_mock.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor/mock", - visibility = ["//visibility:public"], - deps = [ - "//submitqueue/entity:go_default_library", - "//submitqueue/extension/speculation/predictor:go_default_library", - "@org_uber_go_mock//gomock:go_default_library", - ], -) diff --git a/submitqueue/extension/speculation/predictor/mock/predictor_mock.go b/submitqueue/extension/speculation/predictor/mock/predictor_mock.go deleted file mode 100644 index 01088820..00000000 --- a/submitqueue/extension/speculation/predictor/mock/predictor_mock.go +++ /dev/null @@ -1,97 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: predictor.go -// -// Generated by this command: -// -// mockgen -source=predictor.go -destination=mock/predictor_mock.go -package=mock -// - -// Package mock is a generated GoMock package. -package mock - -import ( - context "context" - reflect "reflect" - - entity "github.com/uber/submitqueue/submitqueue/entity" - predictor "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor" - gomock "go.uber.org/mock/gomock" -) - -// MockPredictor is a mock of Predictor interface. -type MockPredictor struct { - ctrl *gomock.Controller - recorder *MockPredictorMockRecorder - isgomock struct{} -} - -// MockPredictorMockRecorder is the mock recorder for MockPredictor. -type MockPredictorMockRecorder struct { - mock *MockPredictor -} - -// NewMockPredictor creates a new mock instance. -func NewMockPredictor(ctrl *gomock.Controller) *MockPredictor { - mock := &MockPredictor{ctrl: ctrl} - mock.recorder = &MockPredictorMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockPredictor) EXPECT() *MockPredictorMockRecorder { - return m.recorder -} - -// Predict mocks base method. -func (m *MockPredictor) Predict(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (predictor.Probability, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Predict", ctx, batch, paths) - ret0, _ := ret[0].(predictor.Probability) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Predict indicates an expected call of Predict. -func (mr *MockPredictorMockRecorder) Predict(ctx, batch, paths any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Predict", reflect.TypeOf((*MockPredictor)(nil).Predict), ctx, batch, paths) -} - -// MockFactory is a mock of Factory interface. -type MockFactory struct { - ctrl *gomock.Controller - recorder *MockFactoryMockRecorder - isgomock struct{} -} - -// MockFactoryMockRecorder is the mock recorder for MockFactory. -type MockFactoryMockRecorder struct { - mock *MockFactory -} - -// NewMockFactory creates a new mock instance. -func NewMockFactory(ctrl *gomock.Controller) *MockFactory { - mock := &MockFactory{ctrl: ctrl} - mock.recorder = &MockFactoryMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockFactory) EXPECT() *MockFactoryMockRecorder { - return m.recorder -} - -// For mocks base method. -func (m *MockFactory) For(cfg predictor.Config) (predictor.Predictor, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "For", cfg) - ret0, _ := ret[0].(predictor.Predictor) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// For indicates an expected call of For. -func (mr *MockFactoryMockRecorder) For(cfg any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "For", reflect.TypeOf((*MockFactory)(nil).For), cfg) -} diff --git a/submitqueue/extension/speculation/predictor/predictor.go b/submitqueue/extension/speculation/predictor/predictor.go deleted file mode 100644 index 66b57c01..00000000 --- a/submitqueue/extension/speculation/predictor/predictor.go +++ /dev/null @@ -1,56 +0,0 @@ -// Copyright (c) 2025 Uber Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package predictor defines how likely a batch is to succeed, given both what -// it changes and what has happened to it so far. A Scorer prices the change; a -// Predictor is built over one and revises its price with the batch's observed -// progress. -package predictor - -//go:generate mockgen -source=predictor.go -destination=mock/predictor_mock.go -package=mock - -import ( - "context" - - "github.com/uber/submitqueue/submitqueue/entity" -) - -// Probability is how likely an outcome is, from 0.0 to 1.0. -type Probability float64 - -// Predictor estimates a batch's final outcome. -type Predictor interface { - // Predict returns how likely the batch is to reach Succeeded with its - // changes landed. A passing build is necessary but not sufficient. - // - // paths is the batch's own build progress, zero-valued for a batch nothing - // has speculated on. Callers may predict every batch a queue waits on, so - // anything expensive belongs behind the implementation's own cache. - Predict(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (Probability, error) -} - -// Config carries the per-queue identity handed to a Factory. The system knows -// only the queue name; everything an implementation needs is injected at -// construction by the integrator. -type Config struct { - // QueueName identifies the queue this Predictor serves. - QueueName string -} - -// Factory builds the Predictor for a queue. Implementations inject what they -// need at construction, including the Scorer whose price they revise. -type Factory interface { - // For returns the Predictor for the given queue. - For(cfg Config) (Predictor, error) -} diff --git a/submitqueue/extension/speculation/scorer/README.md b/submitqueue/extension/speculation/scorer/README.md index 4d70ed15..7b9f5dce 100644 --- a/submitqueue/extension/speculation/scorer/README.md +++ b/submitqueue/extension/speculation/scorer/README.md @@ -1,19 +1,23 @@ # scorer -A `Scorer` returns the probability that a batch ultimately succeeds — reaches its terminal `Succeeded` state with its changes landed, not merely a passing build — as a number between 0.0 and 1.0. It is handed the batch identity and resolves the batch's changes itself through an injected `changeset.Resolver`, so callers pass an `entity.Batch` and nothing more. +A `Scorer` returns how likely a batch is to reach `Succeeded` with its changes landed, as a number between 0.0 and 1.0. `Score(ctx, batch, paths)` is handed the batch identity and that batch's own `SpeculationPathSet` — zero-valued when nothing has speculated on it yet. Callers pass a snapshot they already hold; a scorer must not load the path-set store. Callers may score every batch a queue is waiting on, so implementations should be cheap. A speculation run scores each batch at most once, but it does not carry results across runs; anything expensive belongs behind the implementation's own cache. -The default speculation pipeline does not rank on the scorer directly. The queue's `Predictor` is built over its `Scorer` and revises the scorer's price with path-set evidence before `bestfirst` ranks paths. The scorer still prices only the change; it does not see path sets. +The default `bestfirst` generator ranks on this number. The default scorer is **evidence** wrapping a **base**: heuristic or composite prices the change and ignores `paths`; evidence revises that price from the path set and batch state. Like the other extensions, a `Scorer` is selected **per queue** by the wiring layer through the `Config` (queue name) and `Factory` interface. +See [doc/rfc/submitqueue/outcome-predictor.md](../../../../doc/rfc/submitqueue/outcome-predictor.md) for the GLM, factor contract, evidence rules, and configuration shape. + ## Implementations -**`heuristic`** scores a batch by extracting one number from its changes and matching that against ordered buckets, each mapping a `[Min, Max]` range to a probability. The extraction is a caller-supplied `ValueFunc` over the resolved `entity.BatchChanges`, so the same bucketing works for files touched, lines changed, or any other metric. +**`evidence`** revises a nested base scorer with YAML-configured factors for `pathPassed`, `pathFailed`, `merging`, and `cancelling`. A factor of `1` leaves the base price alone; every factor defaults to `1` until someone sets one. Only paths that assume every dependency succeeds count as path evidence. + +**`heuristic`** scores a batch by extracting one number from its changes and matching that against ordered buckets, each mapping a `[Min, Max]` range to a probability. The extraction is a caller-supplied `ValueFunc` over the resolved `entity.BatchChanges`, so the same bucketing works for files touched, lines changed, or any other metric. It ignores `paths`. -**`composite`** runs several named scorers and reduces their scores to one. The reduce function receives the scores keyed by scorer name, so it can weigh sources differently rather than treating them as interchangeable; `Min`, `Max`, and `Avg` are provided. +**`composite`** runs several named scorers and reduces their scores to one. The reduce function receives the scores keyed by scorer name, so it can weigh sources differently rather than treating them as interchangeable; `Min`, `Max`, and `Avg` are provided. It ignores `paths` except to forward them to children. ## Adding a backend -Create a package under `scorer//` whose `New(...)` returns a `scorer.Scorer`, injecting whatever it needs at construction — a `changeset.Resolver` to reach the batch's changes, a metrics scope, any client. Do not add a `Config` or `Factory` implementation here; per-queue routing and the factory adapter live in the wiring layer. +Create a package under `scorer//` whose `New(...)` returns a `scorer.Scorer`, injecting whatever it needs at construction — a nested `Scorer` for evidence, a `changeset.Resolver` to reach the batch's changes, a metrics scope, any client. Do not add a `Config` or `Factory` implementation here; per-queue routing and the factory adapter live in the wiring layer. diff --git a/submitqueue/extension/speculation/scorer/composite/scorer.go b/submitqueue/extension/speculation/scorer/composite/scorer.go index 92ef31fb..db544103 100644 --- a/submitqueue/extension/speculation/scorer/composite/scorer.go +++ b/submitqueue/extension/speculation/scorer/composite/scorer.go @@ -94,13 +94,13 @@ func New(cfg scorer.Config, scorers map[string]scorer.Scorer, reduce ReduceFunc, // Score evaluates all child scorers on the batch and combines their results using the // reduce function. If any child scorer returns an error, that error is returned immediately. -func (c *compositeScorer) Score(ctx context.Context, batch entity.Batch) (ret float64, retErr error) { +func (c *compositeScorer) Score(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (ret float64, retErr error) { op := metrics.Begin(c.scope, "score", metrics.FastLatencyBuckets) defer func() { op.Complete(retErr) }() scores := make(map[string]float64, len(c.scorers)) for name, s := range c.scorers { - score, err := s.Score(ctx, batch) + score, err := s.Score(ctx, batch, paths) if err != nil { return 0, err } diff --git a/submitqueue/extension/speculation/scorer/composite/scorer_test.go b/submitqueue/extension/speculation/scorer/composite/scorer_test.go index f210fcd5..c2ae5874 100644 --- a/submitqueue/extension/speculation/scorer/composite/scorer_test.go +++ b/submitqueue/extension/speculation/scorer/composite/scorer_test.go @@ -34,14 +34,14 @@ type fixedScorer struct { score float64 } -func (f *fixedScorer) Score(_ context.Context, _ entity.Batch) (float64, error) { +func (f *fixedScorer) Score(_ context.Context, _ entity.Batch, _ entity.SpeculationPathSet) (float64, error) { return f.score, nil } // errorScorer always returns an error. type errorScorer struct{} -func (e *errorScorer) Score(_ context.Context, _ entity.Batch) (float64, error) { +func (e *errorScorer) Score(_ context.Context, _ entity.Batch, _ entity.SpeculationPathSet) (float64, error) { return 0, fmt.Errorf("scorer failed") } @@ -102,7 +102,7 @@ func TestScorer_Score(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := New(testCfg, tt.scorers, tt.reduce, tally.NoopScope) - got, err := s.Score(context.Background(), entity.Batch{}) + got, err := s.Score(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) require.NoError(t, err) assert.InDelta(t, tt.want, got, 1e-9) }) @@ -114,7 +114,7 @@ func TestScorer_Score_ChildError(t *testing.T) { "error": &errorScorer{}, "files": &fixedScorer{0.9}, }, Min, tally.NoopScope) - _, err := s.Score(context.Background(), entity.Batch{}) + _, err := s.Score(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) require.Error(t, err) } @@ -143,7 +143,7 @@ func TestReduceFunc_ReceivesNames(t *testing.T) { "files": &fixedScorer{0.9}, "deps": &fixedScorer{0.95}, }, custom, tally.NoopScope) - got, err := s.Score(context.Background(), entity.Batch{}) + got, err := s.Score(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) require.NoError(t, err) assert.Equal(t, 0.9, got) assert.ElementsMatch(t, []string{"files", "deps"}, receivedNames) diff --git a/submitqueue/extension/speculation/predictor/evidence/BUILD.bazel b/submitqueue/extension/speculation/scorer/evidence/BUILD.bazel similarity index 83% rename from submitqueue/extension/speculation/predictor/evidence/BUILD.bazel rename to submitqueue/extension/speculation/scorer/evidence/BUILD.bazel index f0e2eca9..24875861 100644 --- a/submitqueue/extension/speculation/predictor/evidence/BUILD.bazel +++ b/submitqueue/extension/speculation/scorer/evidence/BUILD.bazel @@ -3,12 +3,11 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = ["evidence.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor/evidence", + importpath = "github.com/uber/submitqueue/submitqueue/extension/speculation/scorer/evidence", visibility = ["//visibility:public"], deps = [ "//platform/metrics:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/speculation/predictor:go_default_library", "//submitqueue/extension/speculation/scorer:go_default_library", "@com_github_uber_go_tally//:go_default_library", ], @@ -20,7 +19,6 @@ go_test( embed = [":go_default_library"], deps = [ "//submitqueue/entity:go_default_library", - "//submitqueue/extension/speculation/predictor:go_default_library", "//submitqueue/extension/speculation/scorer:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/extension/speculation/predictor/evidence/evidence.go b/submitqueue/extension/speculation/scorer/evidence/evidence.go similarity index 78% rename from submitqueue/extension/speculation/predictor/evidence/evidence.go rename to submitqueue/extension/speculation/scorer/evidence/evidence.go index 52eac912..5145469f 100644 --- a/submitqueue/extension/speculation/predictor/evidence/evidence.go +++ b/submitqueue/extension/speculation/scorer/evidence/evidence.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package evidence revises a Scorer's price with factors for observed batch +// Package evidence revises a base Scorer's price with factors for observed batch // progress. See doc/rfc/submitqueue/outcome-predictor.md. package evidence @@ -25,11 +25,10 @@ import ( "github.com/uber-go/tally" "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor" "github.com/uber/submitqueue/submitqueue/extension/speculation/scorer" ) -// Factors revise the scorer's price, one per piece of evidence. A factor of 1 +// Factors revise the base price, one per piece of evidence. A factor of 1 // leaves the price alone. Named fields make unknown evidence fail to compile. type Factors struct { // PathPassed applies once when a build has passed on the batch's @@ -43,7 +42,7 @@ type Factors struct { Cancelling float64 } -// AllOnes is the neutral set: the prediction is the scorer's price. +// AllOnes is the neutral set: Score returns the base price. func AllOnes() Factors { return Factors{PathPassed: 1, PathFailed: 1, Merging: 1, Cancelling: 1} } @@ -51,23 +50,23 @@ func AllOnes() Factors { // epsilon keeps exact certainty revisable while remaining close to the scorer. const epsilon = 1e-6 -// evidence is a predictor.Predictor that revises a scorer's price. +// evidence is a scorer.Scorer that revises a base scorer's price. type evidence struct { - // cfg is the per-queue identity this predictor was built for. - cfg predictor.Config + // cfg is the per-queue identity this scorer was built for. + cfg scorer.Config // base prices the batch's change; its price is what the factors revise. base scorer.Scorer - // factors revise the scorer's price with observed evidence. + // factors revise the base price with observed evidence. factors Factors // scope is the tally scope for emitting metrics. scope tally.Scope } -// New creates an evidence predictor bound to the queue named in cfg, revising +// New creates an evidence scorer bound to the queue named in cfg, revising // base's price by factors. // // It rejects a nil base and factors that are non-finite or not positive. -func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally.Scope) (predictor.Predictor, error) { +func New(cfg scorer.Config, base scorer.Scorer, factors Factors, scope tally.Scope) (scorer.Scorer, error) { if base == nil { return nil, fmt.Errorf("evidence.New: base must not be nil") } @@ -86,13 +85,13 @@ func New(cfg predictor.Config, base scorer.Scorer, factors Factors, scope tally. return &evidence{cfg: cfg, base: base, factors: factors, scope: scope}, nil } -// Predict prices the batch's change, combines its evidence factors, and revises -// the scorer's price with the result. -func (r *evidence) Predict(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (ret predictor.Probability, retErr error) { - op := metrics.Begin(r.scope, "predict", metrics.FastLatencyBuckets) +// Score prices the batch's change, combines its evidence factors, and revises +// the base price with the result. +func (r *evidence) Score(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (ret float64, retErr error) { + op := metrics.Begin(r.scope, "score", metrics.FastLatencyBuckets) defer func() { op.Complete(retErr) }() - price, err := r.base.Score(ctx, batch) + price, err := r.base.Score(ctx, batch, paths) if err != nil { return 0, err } @@ -117,18 +116,18 @@ func (r *evidence) Predict(ctx context.Context, batch entity.Batch, paths entity factor *= r.factors.Cancelling } if factor == 1 { - return predictor.Probability(price), nil + return price, nil } return revise(math.Min(math.Max(price, epsilon), 1-epsilon), factor), nil } // revise applies the combined factor while keeping the result a probability. -func revise(price, factor float64) predictor.Probability { +func revise(price, factor float64) float64 { if math.IsInf(factor, 1) { return 1 - epsilon } revised := price * factor / (1 - price + price*factor) - return predictor.Probability(math.Min(math.Max(revised, epsilon), 1-epsilon)) + return math.Min(math.Max(revised, epsilon), 1-epsilon) } // hasPassedAllSucceedPath reports a passed build on the batch's all-succeed diff --git a/submitqueue/extension/speculation/predictor/evidence/evidence_test.go b/submitqueue/extension/speculation/scorer/evidence/evidence_test.go similarity index 73% rename from submitqueue/extension/speculation/predictor/evidence/evidence_test.go rename to submitqueue/extension/speculation/scorer/evidence/evidence_test.go index a1222fdc..c14466cc 100644 --- a/submitqueue/extension/speculation/predictor/evidence/evidence_test.go +++ b/submitqueue/extension/speculation/scorer/evidence/evidence_test.go @@ -24,24 +24,23 @@ import ( "github.com/stretchr/testify/require" "github.com/uber-go/tally" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/speculation/predictor" "github.com/uber/submitqueue/submitqueue/extension/speculation/scorer" ) // testCfg is the per-queue identity used by every case in this file. -var testCfg = predictor.Config{QueueName: "test-queue"} +var testCfg = scorer.Config{QueueName: "test-queue"} // fixedScorer always returns the same price. type fixedScorer struct{ price float64 } -func (f fixedScorer) Score(_ context.Context, _ entity.Batch) (float64, error) { +func (f fixedScorer) Score(_ context.Context, _ entity.Batch, _ entity.SpeculationPathSet) (float64, error) { return f.price, nil } // errorScorer always fails. type errorScorer struct{} -func (errorScorer) Score(_ context.Context, _ entity.Batch) (float64, error) { +func (errorScorer) Score(_ context.Context, _ entity.Batch, _ entity.SpeculationPathSet) (float64, error) { return 0, fmt.Errorf("scorer failed") } @@ -62,26 +61,26 @@ func pathSet(statuses ...entity.SpeculationPathStatus) entity.SpeculationPathSet return set } -// predict runs one prediction with all-neutral factors except those overridden. -func predict(t *testing.T, price float64, factors Factors, batch entity.Batch, paths entity.SpeculationPathSet) float64 { +// scoreOnce runs one Score with all-neutral factors except those overridden. +func scoreOnce(t *testing.T, price float64, factors Factors, batch entity.Batch, paths entity.SpeculationPathSet) float64 { t.Helper() - p, err := New(testCfg, fixedScorer{price: price}, factors, tally.NoopScope) + s, err := New(testCfg, fixedScorer{price: price}, factors, tally.NoopScope) require.NoError(t, err) - got, err := p.Predict(context.Background(), batch, paths) + got, err := s.Score(context.Background(), batch, paths) require.NoError(t, err) - return float64(got) + return got } -func TestPredict_NeutralFactorsReturnTheScorersPrice(t *testing.T) { +func TestScore_NeutralFactorsReturnTheScorersPrice(t *testing.T) { for _, price := range []float64{0, 0.01, 0.25, 0.5, 0.6, 0.9, 0.99, 1} { t.Run(fmt.Sprintf("price %v", price), func(t *testing.T) { - got := predict(t, price, AllOnes(), entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed)) + got := scoreOnce(t, price, AllOnes(), entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed)) assert.Equal(t, price, got) }) } } -func TestPredict_AppliesOneFactorPerEvidence(t *testing.T) { +func TestScore_AppliesOneFactorPerEvidence(t *testing.T) { // At scorer price 0.5, factor f revises the price to f/(1+f). tests := []struct { name string @@ -136,55 +135,55 @@ func TestPredict_AppliesOneFactorPerEvidence(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.InDelta(t, tt.want, predict(t, 0.5, tt.factors, tt.batch, tt.paths), 1e-9) + assert.InDelta(t, tt.want, scoreOnce(t, 0.5, tt.factors, tt.batch, tt.paths), 1e-9) }) } } // A path built without one of its dependencies proves nothing about a candidate // that assumes the dependency lands, which is what stacking on this batch means. -func TestPredict_IgnoresAPassedPathThatAssumesAFailure(t *testing.T) { +func TestScore_IgnoresAPassedPathThatAssumesAFailure(t *testing.T) { paths := pathSet(entity.SpeculationPathStatusPassed) paths.Paths[0].Path.Dependencies[0].Assumption = entity.DependencyAssumptionFails factors := Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1} - assert.InDelta(t, 0.5, predict(t, 0.5, factors, entity.Batch{}, paths), 1e-9) + assert.InDelta(t, 0.5, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) } // A failed flip-subset must not drag down a green all-succeed build: it was // built under different assumptions, the same filter PathPassed uses. -func TestPredict_IgnoresAFailedPathThatAssumesAFailure(t *testing.T) { +func TestScore_IgnoresAFailedPathThatAssumesAFailure(t *testing.T) { paths := pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed) paths.Paths[1].Path.Dependencies[0].Assumption = entity.DependencyAssumptionFails factors := Factors{PathPassed: 9, PathFailed: 0.25, Merging: 1, Cancelling: 1} - assert.InDelta(t, 0.9, predict(t, 0.5, factors, entity.Batch{}, paths), 1e-9) + assert.InDelta(t, 0.9, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) } -func TestPredict_APathWithNoDependenciesCounts(t *testing.T) { +func TestScore_APathWithNoDependenciesCounts(t *testing.T) { paths := pathSet(entity.SpeculationPathStatusPassed) paths.Paths[0].Path.Dependencies = nil factors := Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1} - assert.InDelta(t, 0.9, predict(t, 0.5, factors, entity.Batch{}, paths), 1e-9) + assert.InDelta(t, 0.9, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) } -func TestPredict_AFailedPathWithNoDependenciesCounts(t *testing.T) { +func TestScore_AFailedPathWithNoDependenciesCounts(t *testing.T) { paths := pathSet(entity.SpeculationPathStatusFailed) paths.Paths[0].Path.Dependencies = nil factors := Factors{PathPassed: 1, PathFailed: 0.25, Merging: 1, Cancelling: 1} - assert.InDelta(t, 0.2, predict(t, 0.5, factors, entity.Batch{}, paths), 1e-9) + assert.InDelta(t, 0.2, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) } -func TestPredict_AnEmptyPathSetIsNoEvidence(t *testing.T) { +func TestScore_AnEmptyPathSetIsNoEvidence(t *testing.T) { factors := Factors{PathPassed: 9, PathFailed: 0.1, Merging: 1, Cancelling: 1} - assert.InDelta(t, 0.5, predict(t, 0.5, factors, entity.Batch{}, entity.SpeculationPathSet{}), 1e-9) + assert.InDelta(t, 0.5, scoreOnce(t, 0.5, factors, entity.Batch{}, entity.SpeculationPathSet{}), 1e-9) } // A scorer certain either way still has to be movable, or no evidence could ever // revise a price the scorer had no business being certain about. -func TestPredict_CertainPricesStayInRangeAndStillMove(t *testing.T) { +func TestScore_CertainPricesStayInRangeAndStillMove(t *testing.T) { tests := []struct { name string price float64 @@ -199,37 +198,37 @@ func TestPredict_CertainPricesStayInRangeAndStillMove(t *testing.T) { t.Run(tt.name, func(t *testing.T) { factors := AllOnes() factors.PathPassed = tt.factor - got := predict(t, tt.price, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed)) + got := scoreOnce(t, tt.price, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed)) assert.Greater(t, got, tt.wantAbove) assert.Less(t, got, tt.wantBelow) }) } } -func TestPredict_LargeFactorsDoNotProduceCertainty(t *testing.T) { +func TestScore_LargeFactorsDoNotProduceCertainty(t *testing.T) { factors := AllOnes() factors.PathPassed = math.MaxFloat64 - got := predict(t, 0.5, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed)) + got := scoreOnce(t, 0.5, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed)) assert.Greater(t, got, 0.99) assert.Less(t, got, 1.0) } -func TestPredict_RejectsAPriceThatIsNotAProbability(t *testing.T) { +func TestScore_RejectsAPriceThatIsNotAProbability(t *testing.T) { for _, price := range []float64{-0.1, 1.5, math.NaN()} { t.Run(fmt.Sprintf("price %v", price), func(t *testing.T) { - p, err := New(testCfg, fixedScorer{price: price}, AllOnes(), tally.NoopScope) + s, err := New(testCfg, fixedScorer{price: price}, AllOnes(), tally.NoopScope) require.NoError(t, err) - _, err = p.Predict(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) + _, err = s.Score(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) require.Error(t, err) }) } } -func TestPredict_PropagatesAScorerError(t *testing.T) { - p, err := New(testCfg, errorScorer{}, AllOnes(), tally.NoopScope) +func TestScore_PropagatesAScorerError(t *testing.T) { + s, err := New(testCfg, errorScorer{}, AllOnes(), tally.NoopScope) require.NoError(t, err) - _, err = p.Predict(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) + _, err = s.Score(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) require.Error(t, err) } @@ -254,9 +253,9 @@ func TestNew_RejectsUnusableConstruction(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - p, err := New(testCfg, tt.base, tt.factors, tally.NoopScope) + s, err := New(testCfg, tt.base, tt.factors, tally.NoopScope) require.Error(t, err) - assert.Nil(t, p) + assert.Nil(t, s) }) } } diff --git a/submitqueue/extension/speculation/scorer/fake/fake.go b/submitqueue/extension/speculation/scorer/fake/fake.go index 61168cf2..02a6c442 100644 --- a/submitqueue/extension/speculation/scorer/fake/fake.go +++ b/submitqueue/extension/speculation/scorer/fake/fake.go @@ -56,7 +56,7 @@ func New(cfg scorer.Config, resolver changeset.Resolver, delegate scorer.Scorer) // Score returns an error when a change URI carries the failure marker; otherwise // it delegates to the wrapped scorer. -func (s scorerFake) Score(ctx context.Context, batch entity.Batch) (float64, error) { +func (s scorerFake) Score(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (float64, error) { changes, err := s.resolver.DetailedForBatch(ctx, batch) if err != nil { return 0, err @@ -64,7 +64,7 @@ func (s scorerFake) Score(ctx context.Context, batch entity.Batch) (float64, err if markerToken(changes) == tokenError { return 0, fmt.Errorf("fake: marked score error") } - return s.delegate.Score(ctx, batch) + return s.delegate.Score(ctx, batch, paths) } // markerToken returns the marker token embedded in the first change URI that diff --git a/submitqueue/extension/speculation/scorer/fake/fake_test.go b/submitqueue/extension/speculation/scorer/fake/fake_test.go index a4f94607..f018cf18 100644 --- a/submitqueue/extension/speculation/scorer/fake/fake_test.go +++ b/submitqueue/extension/speculation/scorer/fake/fake_test.go @@ -61,7 +61,7 @@ func delegate(resolver changeset.Resolver, want float64) scorer.Scorer { func TestScore_DelegatesWhenUnmarked(t *testing.T) { r := resolverFor("github://github.example.com/o/r/pull/1/a") s := New(testCfg, r, delegate(r, 0.7)) - got, err := s.Score(context.Background(), entity.Batch{ID: batchID}) + got, err := s.Score(context.Background(), entity.Batch{ID: batchID}, entity.SpeculationPathSet{}) require.NoError(t, err) assert.Equal(t, 0.7, got) } @@ -69,6 +69,6 @@ func TestScore_DelegatesWhenUnmarked(t *testing.T) { func TestScore_ErrorMarker(t *testing.T) { r := resolverFor("github://github.example.com/o/r/pull/1/a?sq-fake=score-error") s := New(testCfg, r, delegate(r, 0.7)) - _, err := s.Score(context.Background(), entity.Batch{ID: batchID}) + _, err := s.Score(context.Background(), entity.Batch{ID: batchID}, entity.SpeculationPathSet{}) require.Error(t, err) } diff --git a/submitqueue/extension/speculation/scorer/heuristic/scorer.go b/submitqueue/extension/speculation/scorer/heuristic/scorer.go index 55de3fde..ff04f95f 100644 --- a/submitqueue/extension/speculation/scorer/heuristic/scorer.go +++ b/submitqueue/extension/speculation/scorer/heuristic/scorer.go @@ -72,7 +72,7 @@ func New(cfg scorer.Config, resolver changeset.Resolver, buckets []Bucket, value // Score resolves the batch's changes, extracts the metric, then returns the probability // score for the first bucket whose range [Min, Max] contains the value. Returns an error // if no bucket matches. -func (s *heuristicScorer) Score(ctx context.Context, batch entity.Batch) (ret float64, retErr error) { +func (s *heuristicScorer) Score(ctx context.Context, batch entity.Batch, _ entity.SpeculationPathSet) (ret float64, retErr error) { op := metrics.Begin(s.scope, "score", metrics.FastLatencyBuckets) defer func() { op.Complete(retErr) }() changes, err := s.resolver.DetailedForBatch(ctx, batch) diff --git a/submitqueue/extension/speculation/scorer/heuristic/scorer_test.go b/submitqueue/extension/speculation/scorer/heuristic/scorer_test.go index 3274fade..0b483562 100644 --- a/submitqueue/extension/speculation/scorer/heuristic/scorer_test.go +++ b/submitqueue/extension/speculation/scorer/heuristic/scorer_test.go @@ -112,7 +112,7 @@ func TestScorer_Score(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := New(testCfg, changesetfake.New(), tt.buckets, tt.valueFunc, tally.NoopScope) - got, err := s.Score(context.Background(), entity.Batch{}) + got, err := s.Score(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) if tt.wantErr { require.Error(t, err) return @@ -128,7 +128,7 @@ func TestScorer_Score_ValueFuncError(t *testing.T) { return 0, assert.AnError } s := New(testCfg, changesetfake.New(), []Bucket{{Min: 0, Max: 10, Score: 0.9}}, failing, tally.NoopScope) - _, err := s.Score(context.Background(), entity.Batch{}) + _, err := s.Score(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) require.Error(t, err) } diff --git a/submitqueue/extension/speculation/scorer/mock/scorer_mock.go b/submitqueue/extension/speculation/scorer/mock/scorer_mock.go index af42e826..322d4bc5 100644 --- a/submitqueue/extension/speculation/scorer/mock/scorer_mock.go +++ b/submitqueue/extension/speculation/scorer/mock/scorer_mock.go @@ -43,18 +43,18 @@ func (m *MockScorer) EXPECT() *MockScorerMockRecorder { } // Score mocks base method. -func (m *MockScorer) Score(ctx context.Context, batch entity.Batch) (float64, error) { +func (m *MockScorer) Score(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (float64, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Score", ctx, batch) + ret := m.ctrl.Call(m, "Score", ctx, batch, paths) ret0, _ := ret[0].(float64) ret1, _ := ret[1].(error) return ret0, ret1 } // Score indicates an expected call of Score. -func (mr *MockScorerMockRecorder) Score(ctx, batch any) *gomock.Call { +func (mr *MockScorerMockRecorder) Score(ctx, batch, paths any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Score", reflect.TypeOf((*MockScorer)(nil).Score), ctx, batch) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Score", reflect.TypeOf((*MockScorer)(nil).Score), ctx, batch, paths) } // MockFactory is a mock of Factory interface. diff --git a/submitqueue/extension/speculation/scorer/scorer.go b/submitqueue/extension/speculation/scorer/scorer.go index c654cd82..4d8cb385 100644 --- a/submitqueue/extension/speculation/scorer/scorer.go +++ b/submitqueue/extension/speculation/scorer/scorer.go @@ -22,22 +22,13 @@ import ( "github.com/uber/submitqueue/submitqueue/entity" ) -// Scorer computes the probability that a batch ultimately succeeds, based on -// its changes. +// Scorer computes the probability that a batch ultimately succeeds. type Scorer interface { - // Score returns a probability between 0.0 and 1.0 that the given batch - // ultimately succeeds — reaches its terminal Succeeded state with its - // changes landed, rather than Failed or Cancelled. A passing build is - // necessary but not sufficient: a batch whose build already passed can - // still fail to land, so this is the probability of the final outcome, - // not of the build alone. It is handed the batch identity and resolves the - // batch's changes itself through an injected changeset.Resolver. - // - // Callers may score every batch a queue is waiting on, so implementations - // should be cheap: a speculation run scores each batch at most once, but it - // does not carry results over to the next run, so anything expensive to - // compute belongs behind the implementation's own cache. - Score(ctx context.Context, batch entity.Batch) (float64, error) + // Score returns a probability in [0, 1] that the batch reaches Succeeded. + // paths is that batch's build progress (zero-valued if none); content + // backends ignore it. Callers pass a snapshot they already hold — a Scorer + // must not load the path-set store. Implementations should be cheap. + Score(ctx context.Context, batch entity.Batch, paths entity.SpeculationPathSet) (float64, error) } // Config carries the per-queue identity handed to a Factory. The system knows diff --git a/submitqueue/extension/speculation/speculator/standard/standard_test.go b/submitqueue/extension/speculation/speculator/standard/standard_test.go index 78aea9b2..1b756013 100644 --- a/submitqueue/extension/speculation/speculator/standard/standard_test.go +++ b/submitqueue/extension/speculation/speculator/standard/standard_test.go @@ -47,7 +47,7 @@ func assumptionFor(p entity.SpeculationPath, dep string) entity.DependencyAssump // constScorer is a minimal scorer.Scorer that scores every batch identically. type constScorer struct{ v float64 } -func (c constScorer) Score(context.Context, entity.Batch) (float64, error) { return c.v, nil } +func (c constScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) (float64, error) { return c.v, nil } func TestComposed_EndToEnd_NaivePair(t *testing.T) { batches := []entity.Batch{ From 24b33d3ae8468d05363375b5fa1cc8e5240ae3d2 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 14 Sep 2026 12:28:32 -0700 Subject: [PATCH 6/8] fix(speculation): rename merging evidence to landing BatchStateLanding replaced Merging on main. Keep the YAML/factor name aligned with that state, and point the package at outcome-scorer.md. --- .../extension/speculation/scorer/README.md | 2 +- .../speculation/scorer/evidence/evidence.go | 14 ++++---- .../scorer/evidence/evidence_test.go | 32 +++++++++---------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/submitqueue/extension/speculation/scorer/README.md b/submitqueue/extension/speculation/scorer/README.md index 7b9f5dce..7ec50869 100644 --- a/submitqueue/extension/speculation/scorer/README.md +++ b/submitqueue/extension/speculation/scorer/README.md @@ -8,7 +8,7 @@ The default `bestfirst` generator ranks on this number. The default scorer is ** Like the other extensions, a `Scorer` is selected **per queue** by the wiring layer through the `Config` (queue name) and `Factory` interface. -See [doc/rfc/submitqueue/outcome-predictor.md](../../../../doc/rfc/submitqueue/outcome-predictor.md) for the GLM, factor contract, evidence rules, and configuration shape. +See [doc/rfc/submitqueue/outcome-scorer.md](../../../../doc/rfc/submitqueue/outcome-scorer.md) for the GLM, factor contract, evidence rules, and configuration shape. ## Implementations diff --git a/submitqueue/extension/speculation/scorer/evidence/evidence.go b/submitqueue/extension/speculation/scorer/evidence/evidence.go index 5145469f..c3cd486a 100644 --- a/submitqueue/extension/speculation/scorer/evidence/evidence.go +++ b/submitqueue/extension/speculation/scorer/evidence/evidence.go @@ -13,7 +13,7 @@ // limitations under the License. // Package evidence revises a base Scorer's price with factors for observed batch -// progress. See doc/rfc/submitqueue/outcome-predictor.md. +// progress. See doc/rfc/submitqueue/outcome-scorer.md. package evidence import ( @@ -36,15 +36,15 @@ type Factors struct { PathPassed float64 // PathFailed applies once when the all-succeed path has failed. PathFailed float64 - // Merging applies while the batch is merging. - Merging float64 + // Landing applies while the batch is landing. + Landing float64 // Cancelling applies while the batch is cancelling. Cancelling float64 } // AllOnes is the neutral set: Score returns the base price. func AllOnes() Factors { - return Factors{PathPassed: 1, PathFailed: 1, Merging: 1, Cancelling: 1} + return Factors{PathPassed: 1, PathFailed: 1, Landing: 1, Cancelling: 1} } // epsilon keeps exact certainty revisable while remaining close to the scorer. @@ -73,7 +73,7 @@ func New(cfg scorer.Config, base scorer.Scorer, factors Factors, scope tally.Sco for name, factor := range map[string]float64{ "PathPassed": factors.PathPassed, "PathFailed": factors.PathFailed, - "Merging": factors.Merging, + "Landing": factors.Landing, "Cancelling": factors.Cancelling, } { // Zero would permanently pin matching batches to 0; negatives cannot @@ -110,8 +110,8 @@ func (r *evidence) Score(ctx context.Context, batch entity.Batch, paths entity.S factor *= r.factors.PathFailed } switch batch.State { - case entity.BatchStateMerging: - factor *= r.factors.Merging + case entity.BatchStateLanding: + factor *= r.factors.Landing case entity.BatchStateCancelling: factor *= r.factors.Cancelling } diff --git a/submitqueue/extension/speculation/scorer/evidence/evidence_test.go b/submitqueue/extension/speculation/scorer/evidence/evidence_test.go index c14466cc..c6882e60 100644 --- a/submitqueue/extension/speculation/scorer/evidence/evidence_test.go +++ b/submitqueue/extension/speculation/scorer/evidence/evidence_test.go @@ -91,44 +91,44 @@ func TestScore_AppliesOneFactorPerEvidence(t *testing.T) { }{ { name: "a passed path", - factors: Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1}, + factors: Factors{PathPassed: 9, PathFailed: 1, Landing: 1, Cancelling: 1}, paths: pathSet(entity.SpeculationPathStatusPassed), want: 0.9, }, { name: "no passed path leaves the price alone", - factors: Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1}, + factors: Factors{PathPassed: 9, PathFailed: 1, Landing: 1, Cancelling: 1}, paths: pathSet(entity.SpeculationPathStatusBuilding), want: 0.5, }, { name: "one failed path", - factors: Factors{PathPassed: 1, PathFailed: 0.25, Merging: 1, Cancelling: 1}, + factors: Factors{PathPassed: 1, PathFailed: 0.25, Landing: 1, Cancelling: 1}, paths: pathSet(entity.SpeculationPathStatusFailed), want: 0.2, }, { - name: "merging", - factors: Factors{PathPassed: 1, PathFailed: 1, Merging: 19, Cancelling: 1}, - batch: entity.Batch{State: entity.BatchStateMerging}, + name: "landing", + factors: Factors{PathPassed: 1, PathFailed: 1, Landing: 19, Cancelling: 1}, + batch: entity.Batch{State: entity.BatchStateLanding}, want: 0.95, }, { name: "cancelling", - factors: Factors{PathPassed: 1, PathFailed: 1, Merging: 1, Cancelling: 0.25}, + factors: Factors{PathPassed: 1, PathFailed: 1, Landing: 1, Cancelling: 0.25}, batch: entity.Batch{State: entity.BatchStateCancelling}, want: 0.2, }, { name: "a state with no factor leaves the price alone", - factors: Factors{PathPassed: 1, PathFailed: 1, Merging: 19, Cancelling: 0.25}, + factors: Factors{PathPassed: 1, PathFailed: 1, Landing: 19, Cancelling: 0.25}, batch: entity.Batch{State: entity.BatchStateSpeculating}, want: 0.5, }, { name: "evidence compounds across kinds", - factors: Factors{PathPassed: 4, PathFailed: 1, Merging: 3, Cancelling: 1}, - batch: entity.Batch{State: entity.BatchStateMerging}, + factors: Factors{PathPassed: 4, PathFailed: 1, Landing: 3, Cancelling: 1}, + batch: entity.Batch{State: entity.BatchStateLanding}, paths: pathSet(entity.SpeculationPathStatusPassed), want: 0.923076923, }, @@ -146,7 +146,7 @@ func TestScore_IgnoresAPassedPathThatAssumesAFailure(t *testing.T) { paths := pathSet(entity.SpeculationPathStatusPassed) paths.Paths[0].Path.Dependencies[0].Assumption = entity.DependencyAssumptionFails - factors := Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1} + factors := Factors{PathPassed: 9, PathFailed: 1, Landing: 1, Cancelling: 1} assert.InDelta(t, 0.5, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) } @@ -156,7 +156,7 @@ func TestScore_IgnoresAFailedPathThatAssumesAFailure(t *testing.T) { paths := pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed) paths.Paths[1].Path.Dependencies[0].Assumption = entity.DependencyAssumptionFails - factors := Factors{PathPassed: 9, PathFailed: 0.25, Merging: 1, Cancelling: 1} + factors := Factors{PathPassed: 9, PathFailed: 0.25, Landing: 1, Cancelling: 1} assert.InDelta(t, 0.9, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) } @@ -164,7 +164,7 @@ func TestScore_APathWithNoDependenciesCounts(t *testing.T) { paths := pathSet(entity.SpeculationPathStatusPassed) paths.Paths[0].Path.Dependencies = nil - factors := Factors{PathPassed: 9, PathFailed: 1, Merging: 1, Cancelling: 1} + factors := Factors{PathPassed: 9, PathFailed: 1, Landing: 1, Cancelling: 1} assert.InDelta(t, 0.9, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) } @@ -172,12 +172,12 @@ func TestScore_AFailedPathWithNoDependenciesCounts(t *testing.T) { paths := pathSet(entity.SpeculationPathStatusFailed) paths.Paths[0].Path.Dependencies = nil - factors := Factors{PathPassed: 1, PathFailed: 0.25, Merging: 1, Cancelling: 1} + factors := Factors{PathPassed: 1, PathFailed: 0.25, Landing: 1, Cancelling: 1} assert.InDelta(t, 0.2, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) } func TestScore_AnEmptyPathSetIsNoEvidence(t *testing.T) { - factors := Factors{PathPassed: 9, PathFailed: 0.1, Merging: 1, Cancelling: 1} + factors := Factors{PathPassed: 9, PathFailed: 0.1, Landing: 1, Cancelling: 1} assert.InDelta(t, 0.5, scoreOnce(t, 0.5, factors, entity.Batch{}, entity.SpeculationPathSet{}), 1e-9) } @@ -234,7 +234,7 @@ func TestScore_PropagatesAScorerError(t *testing.T) { func TestNew_RejectsUnusableConstruction(t *testing.T) { zeroed := AllOnes() - zeroed.Merging = 0 + zeroed.Landing = 0 negative := AllOnes() negative.PathFailed = -1 infinite := AllOnes() From e69436489dfb6fa73c3cd59b138db866a1d9771a Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 14 Sep 2026 12:36:59 -0700 Subject: [PATCH 7/8] style(speculation): gofmt widened Score test stubs --- .../speculation/generator/bestfirst/bestfirst_test.go | 4 +++- .../speculation/speculator/standard/standard_test.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go b/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go index 4942dee7..41738dd9 100644 --- a/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go +++ b/submitqueue/extension/speculation/generator/bestfirst/bestfirst_test.go @@ -155,7 +155,9 @@ func (errScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) // constScorer scores every batch identically, regardless of ID. type constScorer struct{ v float64 } -func (c constScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) (float64, error) { return c.v, nil } +func (c constScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) (float64, error) { + return c.v, nil +} // wideHead builds one Speculating head over n unresolved dependencies, each at a // distinct score so no two combinations tie. diff --git a/submitqueue/extension/speculation/speculator/standard/standard_test.go b/submitqueue/extension/speculation/speculator/standard/standard_test.go index 1b756013..4537f5ec 100644 --- a/submitqueue/extension/speculation/speculator/standard/standard_test.go +++ b/submitqueue/extension/speculation/speculator/standard/standard_test.go @@ -47,7 +47,9 @@ func assumptionFor(p entity.SpeculationPath, dep string) entity.DependencyAssump // constScorer is a minimal scorer.Scorer that scores every batch identically. type constScorer struct{ v float64 } -func (c constScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) (float64, error) { return c.v, nil } +func (c constScorer) Score(context.Context, entity.Batch, entity.SpeculationPathSet) (float64, error) { + return c.v, nil +} func TestComposed_EndToEnd_NaivePair(t *testing.T) { batches := []entity.Batch{ From 1cf44eb4d6b77b83b5b6565ce5b1bff7dfa6a87b Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Tue, 15 Sep 2026 12:25:44 -0700 Subject: [PATCH 8/8] docs(speculation): align landing evidence terminology Use the landing factor name consistently with the implementation and YAML contract. --- submitqueue/extension/speculation/scorer/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/submitqueue/extension/speculation/scorer/README.md b/submitqueue/extension/speculation/scorer/README.md index 7ec50869..ccdddb65 100644 --- a/submitqueue/extension/speculation/scorer/README.md +++ b/submitqueue/extension/speculation/scorer/README.md @@ -12,7 +12,7 @@ See [doc/rfc/submitqueue/outcome-scorer.md](../../../../doc/rfc/submitqueue/outc ## Implementations -**`evidence`** revises a nested base scorer with YAML-configured factors for `pathPassed`, `pathFailed`, `merging`, and `cancelling`. A factor of `1` leaves the base price alone; every factor defaults to `1` until someone sets one. Only paths that assume every dependency succeeds count as path evidence. +**`evidence`** revises a nested base scorer with YAML-configured factors for `pathPassed`, `pathFailed`, `landing`, and `cancelling`. A factor of `1` leaves the base price alone; every factor defaults to `1` until someone sets one. Only paths that assume every dependency succeeds count as path evidence. **`heuristic`** scores a batch by extracting one number from its changes and matching that against ordered buckets, each mapping a `[Min, Max]` range to a probability. The extraction is a caller-supplied `ValueFunc` over the resolved `entity.BatchChanges`, so the same bucketing works for files touched, lines changed, or any other metric. It ignores `paths`.