Skip to content

Commit 4717e89

Browse files
authored
feat(speculation): rank bestfirst on evidence Scorer (#684)
## Summary ### Why? bestfirst already ranks on one probability per dependency. Wiring a sibling Predictor would reintroduce the factory the RFC dropped. YAML also had to show evidence as the outer scorer, not factors hanging off heuristic. A factors map or nested base under heuristic/composite used to load and then be ignored, so a misplaced pathPassed looked configured while ranking stayed at 1. ### What? Pass the speculate path-set snapshot into `Generate` and into `Score`. Default `scorer.type` is `evidence` wrapping a nested `base`. Named `factors` overlay; a present `base` replaces wholesale. Top-level `type: heuristic` is rejected. Content scorers reject `factors` and a nested `base` the same way. Drop the `predictor:` profile key. `bestfirst` depends only on `scorer.Scorer`. The land-stage YAML key is `landing`. ## Test Plan - ✅ `./tool/bazel test //service/submitqueue/orchestrator/server:go_default_test //submitqueue/extension/speculation/generator/bestfirst:go_default_test //submitqueue/extension/speculation/speculator/standard:go_default_test` - ✅ `go test ./service/submitqueue/orchestrator/server/ -run TestLoadProfilesConfig_RejectsBadScorers` ## Stack - #684 ⬅️ - #686
1 parent 7ba8c32 commit 4717e89

16 files changed

Lines changed: 503 additions & 123 deletions

File tree

service/submitqueue/orchestrator/server/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ go_library(
5555
"//submitqueue/extension/speculation/generator/bestfirst:go_default_library",
5656
"//submitqueue/extension/speculation/scorer:go_default_library",
5757
"//submitqueue/extension/speculation/scorer/composite:go_default_library",
58+
"//submitqueue/extension/speculation/scorer/evidence:go_default_library",
5859
"//submitqueue/extension/speculation/scorer/fake:go_default_library",
5960
"//submitqueue/extension/speculation/scorer/heuristic:go_default_library",
6061
"//submitqueue/extension/speculation/speculator:go_default_library",

service/submitqueue/orchestrator/server/config.go

Lines changed: 111 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ package main
1616

1717
import (
1818
"fmt"
19+
"maps"
20+
"math"
1921
"os"
2022
"time"
2123

@@ -53,6 +55,7 @@ const (
5355

5456
// Scorer types selectable from configuration.
5557
const (
58+
scorerTypeEvidence = "evidence"
5659
scorerTypeHeuristic = "heuristic"
5760
scorerTypeComposite = "composite"
5861
)
@@ -67,6 +70,19 @@ const (
6770
// Ways a composite scorer combines its components.
6871
const combineAvg = "avg"
6972

73+
// Evidence an evidence scorer prices, as named in configuration. The set is
74+
// closed: a factor under any other name would be applied to nothing and never
75+
// noticed.
76+
const (
77+
factorPathPassed = "pathPassed"
78+
factorPathFailed = "pathFailed"
79+
factorLanding = "landing"
80+
factorCancelling = "cancelling"
81+
)
82+
83+
// neutralFactor leaves the scorer's price untouched.
84+
const neutralFactor = 1.0
85+
7086
// defaultBuildBudget is how many builds a queue may have occupying CI at once
7187
// when it states no budget of its own. Four is enough for speculation to be
7288
// visible — a queue that can only build one path never speculates — while
@@ -208,11 +224,19 @@ type analyzerConfig struct {
208224
FailAlways bool `yaml:"failAlways"`
209225
}
210226

211-
// scorerConfig selects how a queue ranks candidate speculation paths. There is
212-
// no scoring stage: the scorer feeds the queue's speculator, which is composed
213-
// from it rather than configured separately.
227+
// scorerConfig selects how a queue ranks candidate speculation paths. The
228+
// ranking scorer is evidence wrapping a nested content base. Heuristic and
229+
// composite belong on base (and on composite components), not at the top level.
214230
type scorerConfig struct {
215231
Type string `yaml:"type"`
232+
// Factors revise the base price, one per piece of evidence (evidence only).
233+
// An omitted key keeps the inherited value, or 1 if neither defaults nor
234+
// the queue named it.
235+
Factors map[string]float64 `yaml:"factors"`
236+
// Base is the content scorer evidence revises (evidence only). An omitted
237+
// base on defaults is the default heuristic; a present base on a queue
238+
// replaces the default base wholesale.
239+
Base *scorerConfig `yaml:"base"`
216240
// Buckets map a batch's total lines changed onto a score (heuristic only).
217241
Buckets []bucketConfig `yaml:"buckets"`
218242
// Components are the scorers a composite combines, keyed by name.
@@ -291,7 +315,7 @@ func (c *profilesConfig) normalizeAndValidate() error {
291315
}
292316
}
293317
if q.Scorer != nil {
294-
if err := q.Scorer.normalizeAndValidate(where); err != nil {
318+
if err := q.Scorer.normalizeOverlay(where); err != nil {
295319
return err
296320
}
297321
}
@@ -353,14 +377,36 @@ func (c profilesConfig) resolve(q namedQueueProfileConfig) queueProfileConfig {
353377
profile.Analyzer = *q.Analyzer
354378
}
355379
if q.Scorer != nil {
356-
profile.Scorer = *q.Scorer
380+
profile.Scorer = overlayScorer(profile.Scorer, *q.Scorer)
357381
}
358382
if q.Speculator != nil {
359383
profile.Speculator = *q.Speculator
360384
}
361385
return profile
362386
}
363387

388+
// overlayScorer keeps default factors the queue did not name. A present base
389+
// replaces the default base wholesale. Type stays evidence unless the override
390+
// names one, which must still be evidence.
391+
func overlayScorer(base, override scorerConfig) scorerConfig {
392+
if override.Type != "" {
393+
base.Type = override.Type
394+
}
395+
if len(override.Factors) > 0 {
396+
merged := maps.Clone(base.Factors)
397+
if merged == nil {
398+
merged = make(map[string]float64, len(override.Factors))
399+
}
400+
maps.Copy(merged, override.Factors)
401+
base.Factors = merged
402+
}
403+
if override.Base != nil {
404+
copied := *override.Base
405+
base.Base = &copied
406+
}
407+
return base
408+
}
409+
364410
func (p *queueProfileConfig) normalizeAndValidate(where string) error {
365411
if err := p.ChangeProvider.normalizeAndValidate(where); err != nil {
366412
return err
@@ -374,7 +420,10 @@ func (p *queueProfileConfig) normalizeAndValidate(where string) error {
374420
if err := p.Scorer.normalizeAndValidate(where); err != nil {
375421
return err
376422
}
377-
return p.Speculator.normalizeAndValidate(where)
423+
if err := p.Speculator.normalizeAndValidate(where); err != nil {
424+
return err
425+
}
426+
return nil
378427
}
379428

380429
func (c *changeProviderConfig) normalizeAndValidate(where string) error {
@@ -514,10 +563,62 @@ func (a *analyzerConfig) normalizeAndValidate(where string) error {
514563
}
515564
}
516565

517-
// normalizeAndValidate applies defaults and rejects a scorer that could not be
518-
// built. An empty block is a flat heuristic: every batch scores the same, which
519-
// is the neutral choice for a queue with no opinion about ordering.
566+
// normalizeAndValidate applies defaults and rejects a ranking scorer that
567+
// could not be built. An empty block is evidence wrapping the default
568+
// heuristic, with every factor neutral.
520569
func (s *scorerConfig) normalizeAndValidate(where string) error {
570+
return s.normalizeRanking(where, true)
571+
}
572+
573+
// normalizeOverlay validates a queue's scorer override without inventing a
574+
// base: omitted base means inherit the default base.
575+
func (s *scorerConfig) normalizeOverlay(where string) error {
576+
return s.normalizeRanking(where, false)
577+
}
578+
579+
func (s *scorerConfig) normalizeRanking(where string, fillBase bool) error {
580+
if s.Type == "" {
581+
s.Type = scorerTypeEvidence
582+
}
583+
if s.Type != scorerTypeEvidence {
584+
return fmt.Errorf("%s: scorer type %q belongs under base, not at the ranking layer", where, s.Type)
585+
}
586+
if len(s.Buckets) > 0 || len(s.Components) > 0 || s.Combine != "" {
587+
return fmt.Errorf("%s: buckets, components, and combine belong under base", where)
588+
}
589+
if err := validateFactors(where, s.Factors); err != nil {
590+
return err
591+
}
592+
if s.Base != nil {
593+
return s.Base.normalizeContent(where + " base")
594+
}
595+
if fillBase {
596+
s.Base = &scorerConfig{}
597+
return s.Base.normalizeContent(where + " base")
598+
}
599+
return nil
600+
}
601+
602+
func validateFactors(where string, factors map[string]float64) error {
603+
for name, factor := range factors {
604+
switch name {
605+
case factorPathPassed, factorPathFailed, factorLanding, factorCancelling:
606+
default:
607+
return fmt.Errorf("%s: unknown scorer factor %q", where, name)
608+
}
609+
// Zero would permanently pin matching batches to 0; negatives cannot
610+
// represent either direction in the factor contract.
611+
if !(factor > 0) || math.IsInf(factor, 0) {
612+
return fmt.Errorf("%s: scorer factor %q is %v, must be finite and positive", where, name, factor)
613+
}
614+
}
615+
return nil
616+
}
617+
618+
func (s *scorerConfig) normalizeContent(where string) error {
619+
if len(s.Factors) > 0 || s.Base != nil {
620+
return fmt.Errorf("%s: factors and base belong on the ranking scorer, not under base", where)
621+
}
521622
if s.Type == "" {
522623
s.Type = scorerTypeHeuristic
523624
}
@@ -539,7 +640,7 @@ func (s *scorerConfig) normalizeAndValidate(where string) error {
539640
return fmt.Errorf("%s: composite scorer needs at least one component", where)
540641
}
541642
for name, component := range s.Components {
542-
if err := component.normalizeAndValidate(fmt.Sprintf("%s component %q", where, name)); err != nil {
643+
if err := component.normalizeContent(fmt.Sprintf("%s component %q", where, name)); err != nil {
543644
return err
544645
}
545646
s.Components[name] = component

service/submitqueue/orchestrator/server/config_test.go

Lines changed: 111 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -350,20 +350,24 @@ func TestDefaultProfilesConfig_KeepsPerQueueScorers(t *testing.T) {
350350
byName[q.Name] = q
351351
}
352352

353-
assert.Equal(t, scorerTypeHeuristic, cfg.Defaults.Scorer.Type)
354-
assert.Len(t, cfg.Defaults.Scorer.Buckets, 1, "the baseline scores every batch alike")
353+
assert.Equal(t, scorerTypeEvidence, cfg.Defaults.Scorer.Type)
354+
require.NotNil(t, cfg.Defaults.Scorer.Base)
355+
assert.Equal(t, scorerTypeHeuristic, cfg.Defaults.Scorer.Base.Type)
356+
assert.Len(t, cfg.Defaults.Scorer.Base.Buckets, 1, "the baseline scores every batch alike")
355357

356358
bucketed, ok := byName["test-queue"]
357359
require.True(t, ok)
358360
require.NotNil(t, bucketed.Scorer)
359-
assert.Equal(t, scorerTypeHeuristic, bucketed.Scorer.Type)
360-
assert.Len(t, bucketed.Scorer.Buckets, 4, "smaller batches must rank ahead of larger ones")
361+
require.NotNil(t, bucketed.Scorer.Base)
362+
assert.Equal(t, scorerTypeHeuristic, bucketed.Scorer.Base.Type)
363+
assert.Len(t, bucketed.Scorer.Base.Buckets, 4, "smaller batches must rank ahead of larger ones")
361364

362365
comp, ok := byName["e2e-test-queue"]
363366
require.True(t, ok)
364367
require.NotNil(t, comp.Scorer)
365-
assert.Equal(t, scorerTypeComposite, comp.Scorer.Type)
366-
assert.ElementsMatch(t, []string{"size", "flat"}, keysOf(comp.Scorer.Components))
368+
require.NotNil(t, comp.Scorer.Base)
369+
assert.Equal(t, scorerTypeComposite, comp.Scorer.Base.Type)
370+
assert.ElementsMatch(t, []string{"size", "flat"}, keysOf(comp.Scorer.Base.Components))
367371
}
368372

369373
func keysOf(m map[string]scorerConfig) []string {
@@ -660,10 +664,19 @@ func TestLoadProfilesConfig_RejectsBadScorers(t *testing.T) {
660664
contents string
661665
}{
662666
{name: "unknown scorer type", contents: "defaults:\n scorer: {type: vibes}\n"},
663-
{name: "composite with no components", contents: "defaults:\n scorer: {type: composite}\n"},
664-
{name: "unknown combine", contents: "defaults:\n scorer:\n type: composite\n combine: median\n components: {a: {type: heuristic}}\n"},
665-
{name: "score out of range", contents: "defaults:\n scorer:\n type: heuristic\n buckets: [{min: 0, max: 10, score: 2.0}]\n"},
666-
{name: "inverted bucket", contents: "defaults:\n scorer:\n type: heuristic\n buckets: [{min: 10, max: 1, score: 0.5}]\n"},
667+
{name: "top-level heuristic", contents: "defaults:\n scorer: {type: heuristic}\n"},
668+
{name: "top-level buckets without type", contents: "defaults:\n scorer:\n buckets: [{min: 0, max: 10, score: 0.9}]\n"},
669+
{name: "buckets next to evidence", contents: "defaults:\n scorer:\n type: evidence\n buckets: [{min: 0, max: 10, score: 0.9}]\n"},
670+
{name: "top-level combine", contents: "defaults:\n scorer:\n combine: avg\n"},
671+
{name: "queue overlay buckets without type", contents: "defaults: {}\nqueues:\n - name: q\n scorer:\n buckets: [{min: 0, max: 10, score: 0.9}]\n"},
672+
{name: "composite with no components", contents: "defaults:\n scorer:\n base: {type: composite}\n"},
673+
{name: "unknown combine", contents: "defaults:\n scorer:\n base:\n type: composite\n combine: median\n components: {a: {type: heuristic}}\n"},
674+
{name: "score out of range", contents: "defaults:\n scorer:\n base:\n type: heuristic\n buckets: [{min: 0, max: 10, score: 2.0}]\n"},
675+
{name: "inverted bucket", contents: "defaults:\n scorer:\n base:\n type: heuristic\n buckets: [{min: 10, max: 1, score: 0.5}]\n"},
676+
{name: "factors under heuristic base", contents: "defaults:\n scorer:\n base:\n type: heuristic\n factors: {pathPassed: 10}\n"},
677+
{name: "nested base under heuristic", contents: "defaults:\n scorer:\n base:\n type: heuristic\n base: {type: heuristic}\n"},
678+
{name: "factors on a composite component", contents: "defaults:\n scorer:\n base:\n type: composite\n components:\n a:\n type: heuristic\n factors: {pathPassed: 10}\n"},
679+
{name: "factors under queue overlay base", contents: "defaults: {}\nqueues:\n - name: q\n scorer:\n base:\n type: heuristic\n factors: {pathPassed: 10}\n"},
667680
}
668681
for _, tt := range tests {
669682
t.Run(tt.name, func(t *testing.T) {
@@ -672,3 +685,91 @@ func TestLoadProfilesConfig_RejectsBadScorers(t *testing.T) {
672685
})
673686
}
674687
}
688+
689+
func TestLoadProfilesConfig_RejectsBadScorerFactors(t *testing.T) {
690+
tests := []struct {
691+
name string
692+
contents string
693+
}{
694+
{name: "unknown factor", contents: "defaults:\n scorer:\n factors: {pathPased: 2}\n"},
695+
{name: "zero factor", contents: "defaults:\n scorer:\n factors: {landing: 0}\n"},
696+
{name: "negative factor", contents: "defaults:\n scorer:\n factors: {pathFailed: -1}\n"},
697+
{name: "infinite factor", contents: "defaults:\n scorer:\n factors: {pathPassed: .inf}\n"},
698+
{name: "bad factor on a queue override", contents: "defaults: {}\nqueues:\n - name: q\n scorer:\n factors: {landing: 0}\n"},
699+
}
700+
for _, tt := range tests {
701+
t.Run(tt.name, func(t *testing.T) {
702+
_, err := loadProfilesConfig(writeProfiles(t, tt.contents))
703+
require.Error(t, err)
704+
})
705+
}
706+
}
707+
708+
// An omitted factors map leaves the queue ranking on its base price
709+
// alone, which is what every queue does until someone states a factor.
710+
func TestLoadProfilesConfig_DefaultsTheScorerToEvidence(t *testing.T) {
711+
cfg, err := loadProfilesConfig(writeProfiles(t, "defaults: {}\nqueues:\n - name: q\n"))
712+
require.NoError(t, err)
713+
714+
assert.Equal(t, scorerTypeEvidence, cfg.Defaults.Scorer.Type)
715+
require.NotNil(t, cfg.Defaults.Scorer.Base)
716+
assert.Equal(t, scorerTypeHeuristic, cfg.Defaults.Scorer.Base.Type)
717+
718+
factors := factorsFrom(cfg.resolve(cfg.Queues[0]).Scorer)
719+
assert.Equal(t, neutralFactor, factors.PathPassed)
720+
assert.Equal(t, neutralFactor, factors.PathFailed)
721+
assert.Equal(t, neutralFactor, factors.Landing)
722+
assert.Equal(t, neutralFactor, factors.Cancelling)
723+
}
724+
725+
func TestLoadProfilesConfig_ReadsScorerFactors(t *testing.T) {
726+
cfg, err := loadProfilesConfig(writeProfiles(t,
727+
"defaults:\n scorer:\n factors: {pathPassed: 10, pathFailed: 0.3, landing: 12, cancelling: 0.1}\n"))
728+
require.NoError(t, err)
729+
730+
factors := factorsFrom(cfg.Defaults.Scorer)
731+
assert.Equal(t, 10.0, factors.PathPassed)
732+
assert.Equal(t, 0.3, factors.PathFailed)
733+
assert.Equal(t, 12.0, factors.Landing)
734+
assert.Equal(t, 0.1, factors.Cancelling)
735+
}
736+
737+
func TestLoadProfilesConfig_QueueScorerFactorsOverlayDefaults(t *testing.T) {
738+
cfg, err := loadProfilesConfig(writeProfiles(t,
739+
"defaults:\n scorer:\n factors: {pathPassed: 10, pathFailed: 0.3, landing: 12, cancelling: 0.1}\nqueues:\n - name: q\n scorer:\n factors: {pathPassed: 4}\n"))
740+
require.NoError(t, err)
741+
742+
factors := factorsFrom(cfg.resolve(cfg.Queues[0]).Scorer)
743+
assert.Equal(t, 4.0, factors.PathPassed)
744+
assert.Equal(t, 0.3, factors.PathFailed)
745+
assert.Equal(t, 12.0, factors.Landing)
746+
assert.Equal(t, 0.1, factors.Cancelling)
747+
748+
defaults := factorsFrom(cfg.Defaults.Scorer)
749+
assert.Equal(t, 10.0, defaults.PathPassed)
750+
}
751+
752+
func TestLoadProfilesConfig_QueueScorerBaseReplacesDefaultBase(t *testing.T) {
753+
cfg, err := loadProfilesConfig(writeProfiles(t, ""+
754+
"defaults:\n"+
755+
" scorer:\n"+
756+
" factors: {pathPassed: 10}\n"+
757+
" base:\n"+
758+
" type: heuristic\n"+
759+
" buckets: [{min: 0, max: 1000, score: 0.4}]\n"+
760+
"queues:\n"+
761+
" - name: q\n"+
762+
" scorer:\n"+
763+
" base:\n"+
764+
" type: heuristic\n"+
765+
" buckets: [{min: 0, max: 1000, score: 0.9}]\n"))
766+
require.NoError(t, err)
767+
768+
resolved := cfg.resolve(cfg.Queues[0]).Scorer
769+
require.NotNil(t, resolved.Base)
770+
assert.Equal(t, 0.9, resolved.Base.Buckets[0].Score)
771+
assert.Equal(t, 10.0, factorsFrom(resolved).PathPassed)
772+
773+
require.NotNil(t, cfg.Defaults.Scorer.Base)
774+
assert.Equal(t, 0.4, cfg.Defaults.Scorer.Base.Buckets[0].Score)
775+
}

service/submitqueue/orchestrator/server/main.go

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -389,24 +389,28 @@ func defaultProfilesConfig() profilesConfig {
389389
// Bucketed scoring: smaller batches are likelier to land, so they
390390
// rank ahead of larger ones. Conflicts stay conservative.
391391
{Name: "test-queue", Scorer: &scorerConfig{
392-
Type: scorerTypeHeuristic,
393-
Buckets: []bucketConfig{
394-
{Min: 0, Max: 1, Score: 0.95},
395-
{Min: 2, Max: 5, Score: 0.80},
396-
{Min: 6, Max: 20, Score: 0.60},
397-
{Min: 21, Max: maxBucket, Score: 0.40},
392+
Base: &scorerConfig{
393+
Type: scorerTypeHeuristic,
394+
Buckets: []bucketConfig{
395+
{Min: 0, Max: 1, Score: 0.95},
396+
{Min: 2, Max: 5, Score: 0.80},
397+
{Min: 6, Max: 20, Score: 0.60},
398+
{Min: 21, Max: maxBucket, Score: 0.40},
399+
},
398400
},
399401
}},
400402
// Maximum parallelism: nothing ever conflicts. Scored by a
401403
// composite, which exercises the combining path.
402404
{Name: "e2e-test-queue",
403405
Analyzer: &analyzerConfig{Type: analyzerTypeNone},
404406
Scorer: &scorerConfig{
405-
Type: scorerTypeComposite,
406-
Combine: combineAvg,
407-
Components: map[string]scorerConfig{
408-
"size": {Type: scorerTypeHeuristic, Buckets: []bucketConfig{{Min: 0, Max: maxBucket, Score: 0.8}}},
409-
"flat": {Type: scorerTypeHeuristic, Buckets: []bucketConfig{{Min: 0, Max: maxBucket, Score: 0.6}}},
407+
Base: &scorerConfig{
408+
Type: scorerTypeComposite,
409+
Combine: combineAvg,
410+
Components: map[string]scorerConfig{
411+
"size": {Type: scorerTypeHeuristic, Buckets: []bucketConfig{{Min: 0, Max: maxBucket, Score: 0.8}}},
412+
"flat": {Type: scorerTypeHeuristic, Buckets: []bucketConfig{{Min: 0, Max: maxBucket, Score: 0.6}}},
413+
},
410414
},
411415
},
412416
},

0 commit comments

Comments
 (0)