feat: continuous coverage evaluation + calibration-triggered retraining - #32
feat: continuous coverage evaluation + calibration-triggered retraining#32gkneighb wants to merge 1 commit into
Conversation
|
Thanks for your contribution. This feature exists to catch calibration problems, so it shouldn't ship without a test proving the detector actually fires. The drift harness the checklist mentions isn't in the diff. Two parts: Unit tests on the trigger logic, in this PR. These can run with fixed inputs, no live servers: steady drift → fires |
| f"Calibration trigger fired (max_dev={max_dev:.2f}pp > " | ||
| f"threshold={settings.CALIBRATION_TRIGGER_THRESHOLD:.2f}pp). Requesting immediate retrain." | ||
| ) | ||
| predictor._calibration_trigger.set() |
There was a problem hiding this comment.
After the trigger fires it resets consecutive_bad = 0 but leaves the EMA at its drifted value. The requested retrain takes time, during which the loop keeps evaluating the still-drifted model, so the EMA stays above threshold and re-arms within K evals, and even after the retrain lands the EMA still carries pre-retrain history. With a short COVERAGE_EVAL_INTERVAL_SEC relative to retrain duration, sustained drift can queue back-to-back retrains rather than one. Should we reset both the EMA and consecutive_bad after a triggered retrain completes, or add an explicit cooldown window?
Addresses review on llm-d#32: - Extract the drift-trigger decision logic into CalibrationTrigger (pure, unit-testable with fixed inputs); wire continuous_coverage_loop to it. - Reset the EMA and counter once a triggered retrain lands, and suppress re-firing while it is in flight, so sustained drift no longer queues back-to-back retrains during the retrain window. - Unit tests: steady drift fires; one-off/noisy do not; steady calibrated never fires; 0-100 coverage scale (not 0-1); worse-of-ttft/tpot; and no re-fire until the retrain lands, then a clean reset. - Commit the manual drift harness under tests/drift_harness/ for the e2e work in llm-d#15. Signed-off-by: Greg Neighbors <26003+gkneighb@users.noreply.github.com>
|
Thanks for the careful review -- all three addressed in the latest commit. Trigger unit tests. Extracted the decision logic into a pure Back-to-back retrains. Went with your first option -- reset after the triggered retrain lands. On fire it records the current Harness. Committed the manual drift harness under Happy to switch to an explicit cooldown window instead of tying the reset to retrain completion if you'd prefer. |
| def awaiting_retrain(self) -> bool: | ||
| return self._pending_retrain is not _UNSET | ||
|
|
||
| def update(self, ttft_cov: float | None, tpot_cov: float | None, last_retrain_time) -> bool: |
There was a problem hiding this comment.
train() returns early without updating last_retrain_time when samples are below MIN_SAMPLES_FOR_RETRAIN (training_server.py:852-854), and also skips the update when training throws or is_ready is false (line 1126 is guarded). If the trigger fires and the resulting retrain takes any of those paths, _pending_retrain never changes: the detector stays in awaiting_retrain forever, and the feature silently dies (the event was already consumed by the training loop). Realistic path: /flush clears the data buckets and test buffers → trigger had already fired → triggered train() skips → stuck.
| _, cov, _ = self._calculate_metrics_on_test(ttft_model, ttft_scaler, ttft_test, "ttft", "actual_ttft_ms") | ||
| if cov is not None: | ||
| with self.lock: | ||
| self.ttft_coverage_scores.append(cov) |
There was a problem hiding this comment.
This writes into the same 5-slot deques that train() populates, which /metrics exposes as ttft_coverage_percent{idx=...} (:1738-1741). Today those series mean "test coverage at the last 5 retrains"; with a 5s eval interval, the retrain history is evicted within ~25s and the series silently becomes "last 5 evaluations of the current model." Anything that reads these metrics — dashboards, the #15 CI work, benchmark scoring — will report different numbers when this feature is enabled even if model quality is identical, so enabled-vs-disabled runs stop being comparable on a signal that didn't actually change.
Suggest routing live evals to a new metric family instead: append to e.g. self.ttft_live_coverage_scores / tpot_live_coverage_scores and emit them as ttft_live_coverage_percent{idx=...}. The per-retrain deques keep their semantics, existing benchmark signals stay apples-to-apples, and only the real behavior change (calibration-triggered retrains) shows up in comparisons.
| crosses CALIBRATION_TRIGGER_THRESHOLD for K consecutive evaluations. | ||
| No-op when COVERAGE_EVAL_INTERVAL_SEC <= 0. | ||
| """ | ||
| if settings.COVERAGE_EVAL_INTERVAL_SEC <= 0: |
There was a problem hiding this comment.
great its 0 by default as this can change the production results in unexpected ways unless we do a full E2E benchmark like here https://github.com/kaushikmitr/llm-d/tree/guides/predicted-latency-routing/guides
Can you confirm that with this set to 0, the behavior remains as its currently
|
Great catch on the pending-state deadlock -- the /flush path you describe is exactly right, and it's now a regression test. All three addressed in the latest commit: 1. Unlanded retrains release the pending state. Rather than a timeout, 2. Live coverage split into its own family, exactly as you suggested: 3. Confirmed: at the default |
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
Rebased onto the component-directory restructure (llm-d#16): the module moved to training/, and calibration lives at training/calibration.py imported as training.calibration. Adds a continuous coverage loop that re-evaluates the loaded model between scheduled retrains and fires an early retrain when an EMA of |coverage - target| stays over threshold for k evaluations. The pure CalibrationTrigger carries the decision logic; live coverage lands in its own metric family; the detector releases its pending state on a retrain that concludes without landing a model. Opt-in via LATENCY_COVERAGE_EVAL_INTERVAL_SEC (default 0 = disabled). Squashes the prior four commits on this branch; review history is preserved in PR llm-d#32. Signed-off-by: Greg Neighbors <26003+gkneighb@users.noreply.github.com>
974e95b to
954a659
Compare
|
@kaushikmitr rebased onto the component-directory restructure (#16) -- the module now lives at This is the same set of changes addressing your earlier review (attempt-counter release of the pending state on unlanded retrains + the separate |
Rebased onto the component-directory restructure (llm-d#16) and stacked on the rebased llm-d#32: aci.py moves to training/aci.py, imported as training.aci. One-sided upper ACI (Gibbs & Candes 2021) widens the prediction interval between retrains so SLO coverage holds through detection lag. Opt-in via LATENCY_ACI_ENABLED (default false); exposes aci_alpha, aci_offset_ms, and the ACI-adjusted coverage series in /metrics. Union-merged cleanly with llm-d#32's live-coverage additions (both metric families coexist). Squashes the prior four ACI commits; review history is in PR llm-d#33. Signed-off-by: Greg Neighbors <26003+gkneighb@users.noreply.github.com>
Rebased onto the component-directory restructure (llm-d#16): the module moved to training/, and calibration lives at training/calibration.py imported as training.calibration. Adds a continuous coverage loop that re-evaluates the loaded model between scheduled retrains and fires an early retrain when an EMA of |coverage - target| stays over threshold for k evaluations. The pure CalibrationTrigger carries the decision logic; live coverage lands in its own metric family; the detector releases its pending state on a retrain that concludes without landing a model. Opt-in via LATENCY_COVERAGE_EVAL_INTERVAL_SEC (default 0 = disabled). Squashes the prior four commits on this branch; review history is preserved in PR llm-d#32. Signed-off-by: Greg Neighbors <26003+gkneighb@users.noreply.github.com>
954a659 to
492964f
Compare
Rebased onto the component-directory restructure (llm-d#16) and stacked on the rebased llm-d#32: aci.py moves to training/aci.py, imported as training.aci. One-sided upper ACI (Gibbs & Candes 2021) widens the prediction interval between retrains so SLO coverage holds through detection lag. Opt-in via LATENCY_ACI_ENABLED (default false); exposes aci_alpha, aci_offset_ms, and the ACI-adjusted coverage series in /metrics. Union-merged cleanly with llm-d#32's live-coverage additions (both metric families coexist). Squashes the prior four ACI commits; review history is in PR llm-d#33. Signed-off-by: Greg Neighbors <26003+gkneighb@users.noreply.github.com>
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
2 participants
What does this PR do?
Adds a background loop that re-evaluates p90 coverage on the current model + test buffer every
COVERAGE_EVAL_INTERVAL_SEC, without retraining. When the EMA of|coverage - target|exceedsCALIBRATION_TRIGGER_THRESHOLDforCALIBRATION_TRIGGER_Kconsecutive evaluations, it signals the training loop to retrain immediately instead of waiting out the fullRETRAINING_INTERVAL_SEC. Opt-in; disabled by default (COVERAGE_EVAL_INTERVAL_SEC=0).Why is this change needed?
Implements #19. A statically-trained p90 interval under-covers when the workload drifts between scheduled retrains; the predictor only recovers at the next retrain (a multi-minute lag). Continuously evaluating calibration lets the system detect drift early and shorten that lag.
How was this tested?
"Retraining triggered by calibration deviation")(The loop is threading/timing-based; verified by integration/manual runs rather than unit tests.)
Checklist
git commit -s) per DCOmake test)make lint)Related Issues
Implements #19 (core). The ACI enhancement discussed in that thread follows in a stacked PR.