Dynamic retraining based on model performance ( IN DRAFT PHASE ) - #79
Dynamic retraining based on model performance ( IN DRAFT PHASE )#79modassarrana89-new wants to merge 12 commits into
Conversation
Signed-off-by: Modassar-Rana <modassar.rana@ibm.com>
Signed-off-by: Modassar-Rana <modassar.rana@ibm.com>
Signed-off-by: Modassar-Rana <modassar.rana@ibm.com>
Signed-off-by: Modassar-Rana <modassar.rana@ibm.com>
Signed-off-by: Modassar-Rana <modassar.rana@ibm.com>
| LIVE_ERROR_WINDOW_SIZE: int = int(os.getenv("LATENCY_LIVE_ERROR_WINDOW_SIZE", 200)) | ||
| MIN_LIVE_SAMPLES_FOR_DRIFT_CHECK: int = int(os.getenv("LATENCY_MIN_LIVE_SAMPLES_FOR_DRIFT_CHECK", 50)) | ||
| # Live NRMSE must exceed baseline_nrmse * this multiplier to trigger. | ||
| DRIFT_NRMSE_MULTIPLIER: float = float(os.getenv("LATENCY_DRIFT_NRMSE_MULTIPLIER", "1.5")) |
There was a problem hiding this comment.
Could we add some validation or rationale for these default thresholds? The 1.5 NRMSE multiplier, 0.15 violation-rate increase, 50-sample minimum, and 200-sample window currently look heuristic.
Since the default model predicts the p90 quantile, it would be useful to check these values against replayed data and report whether they cause false or overly frequent retraining.
There was a problem hiding this comment.
Agreed these are unvalidated heuristics right now. I'll replay recent traffic through the drift calculation and sweep DRIFT_NRMSE_MULTIPLIER, the violation-rate delta, MIN_LIVE_SAMPLES_FOR_DRIFT_CHECK, and LIVE_ERROR_WINDOW_SIZE to see how often each would fire under normal steady-state traffic vs. an actual workload shift. Before that, I want to double check one thing: since the model targets p90, actuals are expected to exceed predictions ~10% of the time by design — I want to confirm the violation-rate baseline in the code accounts for that rather than treating any exceedance as anomalous, since that changes what the threshold is actually measuring. Will follow up with replay results and either justify the current defaults or update them.
There was a problem hiding this comment.
p90 baseline — confirmed this is already handled correctly. The
violation-rate trigger compares against baseline_violation_rate + DRIFT_VIOLATION_RATE_ABS_INCREASE (training_server.py:1465), not
against zero, so it only fires on a rise above whatever the baseline
already is. Since the baseline gets snapshotted from real live
traffic, it naturally sits near the model's target quantile (~10% for
p90) rather than being treated as anomalous on its own.
Replay validation — fed synthetic calibration/steady/drift data
through the actual should_retrain_due_to_drift() (not a
reimplementation), sweeping each threshold one at a time against the
shipped defaults. Headline numbers for the default config (1.5x NRMSE
multiplier, 0.15 violation delta, 50 min samples, 200 window):
| metric | false positives (steady, ~1.1hr) | detection lag (moderate synthetic drift) |
|---|---|---|
| ttft | 0 | ~862s (~14 min) |
| tpot | 0 | ~1415s (~24 min) |
Full sweep results + script: scripts/validation/replay_drift_validation.py
(also generates the synthetic data — see generate_synthetic_replay_data.py
in the same dir).
Two caveats before I'd call this settled:
- NRMSE multiplier (tested 1.3/1.5/1.7) had zero effect on any
result — violation-rate crosses first at every value in that range,
so this lever is unexercised at this drift severity. Rerunning with
a milder/slower synthetic shift to actually validate it. - 0 false positives is from only ~1.1hr of synthetic steady
traffic (~200 effectively-independent window checks) — not
enough data to claim the FP rate is actually near zero at scale.
Extending the steady-state run length before treating this as
confirmed, and will rerun against real traffic once we have
historical logs to replay instead of synthetic data.
Will update this thread once both are addressed.
There was a problem hiding this comment.
Sweep result .csv
(.venv) [root@t313lp77 llm-d-latency-predictor]# cat sweep_results.csv
metric,nrmse_multiplier,violation_rate_delta,min_live_samples,window_size,false_positives_per_hour,detection_lag_seconds,n_triggers_total
ttft,1.5,0.15,50,200,0.0,862.0,3
tpot,1.5,0.15,50,200,0.0,1415.0,2
ttft,1.3,0.15,50,200,0.0,862.0,3
tpot,1.3,0.15,50,200,0.0,1415.0,2
ttft,1.7,0.15,50,200,0.0,862.0,3
tpot,1.7,0.15,50,200,0.0,1415.0,2
ttft,1.5,0.1,50,200,0.0,870.2,4
tpot,1.5,0.1,50,200,0.0,629.2,4
ttft,1.5,0.2,50,200,0.0,1085.2,2
tpot,1.5,0.2,50,200,0.0,2579.8,1
ttft,1.5,0.15,30,200,0.0,862.0,3
tpot,1.5,0.15,30,200,0.0,1415.0,2
ttft,1.5,0.15,50,100,0.0,512.0,4
tpot,1.5,0.15,50,100,0.0,1064.2,4
ttft,1.5,0.15,50,300,0.0,890.2,3
tpot,1.5,0.15,50,300,0.0,1519.0,1
just gate the baseline overwrite on sample count before clearing the window Signed-off-by: modassarrana89-new <modassar.rana@ibm.com>
What does this PR do?
Fixes #40
Why is this change needed?
This is feature enhancement for " Dynamic retraining on actual model performance "
Testing
Added tests/test_drift_retraining.py — unit tests for the live drift-detection / dynamic retraining trigger (issue #40). These instantiate LatencyPredictor directly in-process (no live server, no trained model needed), covering:
Known gap — not fixed in this PR: these tests aren't currently wired into CI. tests/Dockerfile's CMD and both deploy/base/test/*-job.yaml manifests hardcode pytest to run only tests/test_dual_server_client.py against the live Kind-cluster deployment; no other workflow invokes pytest. This new file needs a separate fast unit-test job (no live server dependency) added to ci-pr-checks.yaml — happy to open that as a quick follow-up PR, or fold it into this one if preferred.
Related Issues
(#40)