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 7dfb9f35..9b92d23c 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/scorer/README.md b/submitqueue/extension/speculation/scorer/README.md index d8b63693..7b9f5dce 100644 --- a/submitqueue/extension/speculation/scorer/README.md +++ b/submitqueue/extension/speculation/scorer/README.md @@ -1,17 +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 `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/scorer/evidence/BUILD.bazel b/submitqueue/extension/speculation/scorer/evidence/BUILD.bazel new file mode 100644 index 00000000..24875861 --- /dev/null +++ b/submitqueue/extension/speculation/scorer/evidence/BUILD.bazel @@ -0,0 +1,27 @@ +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/scorer/evidence", + visibility = ["//visibility:public"], + deps = [ + "//platform/metrics:go_default_library", + "//submitqueue/entity: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/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/scorer/evidence/evidence.go b/submitqueue/extension/speculation/scorer/evidence/evidence.go new file mode 100644 index 00000000..5145469f --- /dev/null +++ b/submitqueue/extension/speculation/scorer/evidence/evidence.go @@ -0,0 +1,167 @@ +// 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 base Scorer's price with factors for observed batch +// progress. 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/scorer" +) + +// 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 + // all-succeed path. + PathPassed float64 + // PathFailed applies once when the all-succeed path has failed. + 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: Score returns the base price. +func AllOnes() Factors { + return Factors{PathPassed: 1, PathFailed: 1, Merging: 1, Cancelling: 1} +} + +// epsilon keeps exact certainty revisable while remaining close to the scorer. +const epsilon = 1e-6 + +// evidence is a scorer.Scorer that revises a base scorer's price. +type evidence struct { + // 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 base price with observed evidence. + factors Factors + // scope is the tally scope for emitting metrics. + scope tally.Scope +} + +// 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 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") + } + for name, factor := range map[string]float64{ + "PathPassed": factors.PathPassed, + "PathFailed": factors.PathFailed, + "Merging": factors.Merging, + "Cancelling": factors.Cancelling, + } { + // Zero would permanently pin matching batches to 0; negatives cannot + // represent either direction in the factor contract. + 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 +} + +// 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, paths) + 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) + } + + 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 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) float64 { + if math.IsInf(factor, 1) { + return 1 - epsilon + } + revised := price * factor / (1 - price + price*factor) + return math.Min(math.Max(revised, epsilon), 1-epsilon) +} + +// 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 +} + +// 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) { + return true + } + } + return false +} diff --git a/submitqueue/extension/speculation/scorer/evidence/evidence_test.go b/submitqueue/extension/speculation/scorer/evidence/evidence_test.go new file mode 100644 index 00000000..c14466cc --- /dev/null +++ b/submitqueue/extension/speculation/scorer/evidence/evidence_test.go @@ -0,0 +1,261 @@ +// 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/scorer" +) + +// testCfg is the per-queue identity used by every case in this file. +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, _ entity.SpeculationPathSet) (float64, error) { + return f.price, nil +} + +// errorScorer always fails. +type errorScorer struct{} + +func (errorScorer) Score(_ context.Context, _ entity.Batch, _ entity.SpeculationPathSet) (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 +} + +// 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() + s, err := New(testCfg, fixedScorer{price: price}, factors, tally.NoopScope) + require.NoError(t, err) + got, err := s.Score(context.Background(), batch, paths) + require.NoError(t, err) + return got +} + +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 := scoreOnce(t, price, AllOnes(), entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed, entity.SpeculationPathStatusFailed)) + assert.Equal(t, price, got) + }) + } +} + +func TestScore_AppliesOneFactorPerEvidence(t *testing.T) { + // At scorer price 0.5, factor f revises the price to f/(1+f). + 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: "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, 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 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, 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 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, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) +} + +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, scoreOnce(t, 0.5, factors, entity.Batch{}, paths), 1e-9) +} + +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, 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} + 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 TestScore_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 := scoreOnce(t, tt.price, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed)) + assert.Greater(t, got, tt.wantAbove) + assert.Less(t, got, tt.wantBelow) + }) + } +} + +func TestScore_LargeFactorsDoNotProduceCertainty(t *testing.T) { + factors := AllOnes() + factors.PathPassed = math.MaxFloat64 + + got := scoreOnce(t, 0.5, factors, entity.Batch{}, pathSet(entity.SpeculationPathStatusPassed)) + assert.Greater(t, got, 0.99) + assert.Less(t, got, 1.0) +} + +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) { + s, err := New(testCfg, fixedScorer{price: price}, AllOnes(), tally.NoopScope) + require.NoError(t, err) + _, err = s.Score(context.Background(), entity.Batch{}, entity.SpeculationPathSet{}) + require.Error(t, err) + }) + } +} + +func TestScore_PropagatesAScorerError(t *testing.T) { + s, err := New(testCfg, errorScorer{}, AllOnes(), tally.NoopScope) + require.NoError(t, err) + _, err = s.Score(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 + infinite := AllOnes() + infinite.PathPassed = math.Inf(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: "infinite factor", base: fixedScorer{price: 0.5}, factors: infinite}, + {name: "unset factors", base: fixedScorer{price: 0.5}, factors: Factors{}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, err := New(testCfg, tt.base, tt.factors, tally.NoopScope) + require.Error(t, err) + 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 a0d75401..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 merge, 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{