fix: don't ignore SHA operations from promotion of exited trials
Previously, any operations that came about due to the promotion of a
trial that had already exited early were dropped, which could lead the
searcher to get to a state where it stopped emitting workloads while
trials were still active.
@@ -175,21 +175,14 @@ func (s *syncHalvingSearch) promoteSync(
ops = append(ops, NewTrain(promotionID, model.NewLength(s.Unit(), unitsNeeded)))
ops = append(ops, NewValidate(promotionID))
} else {
- // We can make a recursive call (and discard the results)
- // because of the following invariants:
- // 1) There are other trials executing that will receive any
- // extra operations when they complete workloads. We know
- // this is true since otherwise we would have received
- // TrialClosed responses already and the searcher would have
- // closed.
- //
- // 2) We are bounded on the depth of this recursive stack by
- // the number of rungs. We default this to max out at 5.
- _, err := s.promoteSync(ctx, promotionID, shaExitedMetricValue)
- return nil, err
+ // Since the trial being promoted has already exited and will never finish any more workloads,
+ // we should treat it as immediately completing the next rung with the worst possible result.
+ // The recursive call is safe because the rung being considered goes up by one each time and
+ // there are a finite number of rungs.
+ return s.promoteSync(ctx, promotionID, shaExitedMetricValue)
}
}
- // Closes the unpromoted trials in the rung once all trials in the rung finish.
+ // Close the unpromoted trials in the rung once all trials in the rung finish.
if rung.startTrials < len(rung.metrics) {
return nil, errors.Errorf("number of trials exceeded initial trials for rung: %d < %d",
rung.startTrials, len(rung.metrics))
