feat: add per-topology-class residual correction (behind default-off flag) - #3
feat: add per-topology-class residual correction (behind default-off flag)#3madhugoutham wants to merge 2 commits into
Conversation
…flag) Adds TopologyCorrectionTable (common/topology_correction.py): a per-class TTFT correction computed out-of-fold on a frozen base model, rather than feeding topology_distance directly into the model as a raw feature. Why not a raw feature: measured on real P/D traffic (predictor issue llm-d#30), feeding a low-cardinality topology label into XGBoost let it steal decision-splitting capacity from higher-value continuous features, improving one topology class (zone: -42.8ms MAE) while regressing another (region: +20.3ms MAE, CI excludes zero). The residual-correction approach cannot cause that kind of cross-class regression, because no class's correction is estimated from or affects another class's data - confirmed by two independent test datasets in this session (real Kermit data: neutral, no regression; synthetic data: no regression on either gap size tested). Wiring: - TrainingEntry / PredictionRequest (both servers): new optional topology_distance field, default None - LatencyPredictor.train(): fits the correction table after each retrain, scoped to XGBoost only (the only model type this was validated against), gated behind settings.ENABLE_TOPOLOGY_CORRECTION (default false) - Persisted via the existing _get_model_paths()/joblib pattern; excluded from the /model/export seed bundle (a fresh replica should re-derive its own cluster's corrections, not inherit a source cluster's) - prediction_server.py: syncs and applies the table the same way as every other model artifact; correction_for() is an unconditional 0.0 no-op when topology_distance is absent or the table is empty Backward compatibility: the flag defaults to false, and correction_for() returns exactly 0.0 for any absent/unrecognized class regardless of the flag, so every existing caller that doesn't send topology_distance is completely unaffected. Explicitly deferred: enabling this in any real deployment. Measured neutral (no improvement, no harm) on Kermit's single-region uniform-InfiniBand fabric - the real gap there (~37ms) is too small relative to this model's own ~750-800ms p90 margin to show a measurable difference either way. Do not enable without validating on a cluster with a larger real topology gap first. Tests: tests/test_topology_correction.py (8 cases - no-op guarantees, bias recovery direction, min-sample skipping, serialization round-trip). Signed-off-by: Madhu Goutham Reddy Ambati <mambati@redhat.com>
Real regression caught via live integration testing (not the unit test suite, which doesn't exercise the raw DataFrame path): training_server.py's train() runs `pd.DataFrame(clean_ttft).dropna()` on the FULL raw training row - every column, not just the ones actually used as features - before any feature selection happens. pandas treats None as NaN once it's in a DataFrame, and .dropna() defaults to dropping a row if ANY column is NaN. Defaulting topology_distance to None therefore silently discarded every row from every caller that doesn't send it - which, for a brand new optional field, was every existing caller, 100% of the time. Confirmed via a live training/prediction server pair: 3,000 real training samples submitted, zero survived to actually train on. Fix: default to "" instead, matching the existing pod_type field's already- safe convention exactly (empty string survives .dropna(); None does not). Also updated TopologyCorrectionTable.fit()/correction_for() and the retrain trigger condition to treat "" as "no topology known", consistent with None, so an empty-string absence never gets fit as a spurious real class or looked up as one. Verified against a real, live training+prediction server pair (matching the actual Dockerfile invocation, not just unit tests): the existing tests/test_dual_server_client.py::test_dual_server_quantile_regression_learns_distribution_stress integration test, which exercises this exact path with real HTTP traffic, now passes. It did not before this fix, even with training/training_server.py's unrelated pre-existing _drop_timestamp bug fixed locally to isolate this specific issue. Added tests/test_topology_correction.py::TestTopologyDistanceWireDefaultIsDropnaSafe as a permanent regression guard, plus two more cases confirming "" is never fit or looked up as a real topology class. Signed-off-by: Madhu Goutham Reddy Ambati <mambati@redhat.com>
|
Update: found and fixed a real bug in this PR itself via live integration testing (not caught by the unit test suite alone). The bug: Fix: default to How I caught it: ran the actual Also found, while doing this: a separate, pre-existing, unrelated bug ( Also found a second, separate, unrelated pre-existing calibration issue in |
|
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 |
1 participant
Summary
Adds
TopologyCorrectionTable(common/topology_correction.py): a per-class TTFT correction computed out-of-fold on a frozen base model, applied after prediction rather than fed into the model as a raw training feature.Why not a raw feature
Measured on real P/D traffic (predictor issue llm-d#30): feeding
topology_distancedirectly into XGBoost let it steal decision-splitting capacity from higher-value continuous features, improving one class (zone: -42.8ms MAE) while regressing another (region: +20.3ms MAE, CI excludes zero - a real regression, not noise). Residual correction cannot cause that kind of cross-class regression, since no class's correction is estimated from or affects another class's data.What changed
common/topology_correction.py- new, self-containedTopologyCorrectionTableclass (fit/correction_for/serialize)TrainingEntry/PredictionRequest(both servers) - new optionaltopology_distancefield, defaultNoneLatencyPredictor.train()- fits the table after each retrain, scoped to XGBoost only (the only model type this was validated against), gated behindsettings.ENABLE_TOPOLOGY_CORRECTION(default false)_get_model_paths()/joblib pattern; explicitly excluded from the/model/exportseed bundle (a fresh replica should re-derive its own cluster's corrections, not inherit a source cluster's)prediction_server.py- syncs and applies the table the same way as every other model artifactBackward compatibility
The flag defaults to
false.correction_for()returns exactly0.0for any absent/unrecognized class regardless of the flag, so every existing caller that doesn't sendtopology_distanceis completely unaffected. Verified: existing test suite (tests/, 16 cases) passes unchanged; both servers import and construct cleanly with the feature present but unconfigured.Validation this session
tests/test_topology_correction.py): no-op guarantees (None, unknown class, empty table), correct bias-recovery direction on synthetic data with a known injected gap, min-sample-per-class skipping, joblib serialization round-trip, input validationoffline_feature_ab.pyself-test): this approach is neutral (no regression) on real production-shaped dataExplicitly deferred - do not enable yet
This is not validated for any real deployment. Measured neutral (no improvement, no harm) on Kermit's single-region, uniform-InfiniBand fabric - the real topology gap there (~37ms) is too small relative to this model's own ~750-800ms p90 margin to move the needle either way. Do not flip
LATENCY_ENABLE_TOPOLOGY_CORRECTIONon in any real deployment without first validating on a cluster with a larger real topology gap (heterogeneous fabric / cross-region).Not covered in this PR (follow-up)
predict_batch()does not yet apply the correction (onlypredict()does) - same wiring needed, kept out to keep this diff reviewabletests/test_dual_server_client.pyintegration suite (live server processes) was not re-run locally in this session (requires infra beyond what's available here) - please confirm it stays green in CIScope note
Sandboxed on my fork. Not proposing this upstream, and not closing predictor issue llm-d#30 - the issue is a design investigation, and this PR's own honest conclusion is "safe but not yet proven beneficial," not "solved."