Skip to content

feat: standardized offline A/B benchmark template for feature validation - #60

Open
madhugoutham wants to merge 1 commit into
llm-d:mainfrom
madhugoutham:feat/offline-ab-benchmark-template
Open

feat: standardized offline A/B benchmark template for feature validation#60
madhugoutham wants to merge 1 commit into
llm-d:mainfrom
madhugoutham:feat/offline-ab-benchmark-template

Conversation

@madhugoutham

@madhugoutham madhugoutham commented Jul 22, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Standardizes how latency predictor feature changes are validated. Every engineered feature PR (addition or removal) can now ship the same before/after evidence on a named workload calibration plots, reliability diagrams, error bars across seeds, convergence analysis, and a significance test so real signal is distinguishable from noise and regressions on the
untouched target are caught.

Includes:

  • offline_feature_ab.py contention gate, shuffled-label self-test, N-seed A/B, calibration plots, reliability diagrams, SHAP importance, regression gate (fails on TTFT regression; TPOT contamination prevented by column-level assertion), paired significance test
  • run_validation.py — single-command runner with exit code 0/1 and machine-readable verdict (IMPROVED/NEUTRAL/REGRESSION)
  • trace_recorder.py — stdlib-only recording proxy for capturing real EPP training entries to JSONL
  • test_parity.py — drift detection that validates fallback column names AND derived values against production (or column names against a committed golden file in CI)
  • golden_columns.json — committed snapshot of production feature columns, used by CI to validate fallback parity without production code
  • Three reference traces from real H200 GPUs with filled workload specs:
  • sharegpt-h200 (3,400 samples, short conversations, moderate contention)
  • chatbot-synthetic-h200 (3,060 samples, long prompts, extreme contention)
  • bimodal-h200 (3,400 samples, mixed short+long, extreme contention)
  • Trace profile report detecting degenerate distributions (warns when a trace can't exercise a feature)
  • CI workflow draft that auto-runs A/B when a PR includes feature.env (5 seeds for fast screening; full evidence standard requires ≥10 via run_validation.py)
  • XGBoost and LightGBM estimators synced with production hyperparameters
  • Forward-compatible with multimodal encoder features (encoder_matched_size, encoder_input_size)
  • Convergence analysis (--convergence) showing how feature delta evolves with sample count
  • Auto-imports production feature engineering when available, falls back to handwritten copy outside the repo

This is the benchmarking infrastructure; it starts here and can migrate to llm-d-benchmark as it matures.

Why is this change needed?

As more features get added, we need a way to systematically say it's good or bad. Previously, feature validation was ad-hoc synthetic data, no calibration plots, no error bars, no workload spec. This template standardizes the evidence standard so every feature PR ships the same proof.

Key properties:

  • Feature-agnostic: --feature is a required arg, any column works
  • Drop-in traces: --trace accepts any JSONL with 7 required fields; combine traces with cat
  • Production parity: auto-imports production feature engineering; test_parity.py validates fallback columns and values against production, or columns against golden file in CI
  • No GPU needed: offline analysis runs on CPU in seconds using a frozen trace snapshot
  • Regression-safe: fails on TTFT regression, TPOT contamination, or indistinguishable signal

How was this tested?

  • Fallback parity verified via test_parity.py — forces fallback path and compares column names AND derived values against production (inside repo) or column names against golden file (CI)
  • Sabotage test: setting effective_input_tokens = 999 in fallback correctly triggers parity failure
  • 12-point cross-validation: XGBoost params (12/12), LightGBM params (11/11), TTFT/TPOT column order, derived features, zero-fill, quantile loss formula
  • End-to-end on 3 real GPU traces (9,860 total samples):
    sharegpt-h200.jsonl (3,400 samples, 96% contention),
    chatbot-synthetic-h200.jsonl (3,060 samples, 99% contention),
    bimodal-h200.jsonl (3,400 samples, 98% contention) — all gates passed, all artifacts generated
  • Trace schema validation tested: empty file, wrong types, small sample warning
  • TTFT regression gate tested: harmful features return exit code 1; TPOT contamination prevented by column-level assertion
  • Parity test validated in both environments: production-available (compares fallback vs live production, columns + values) and production-hidden (compares fallback vs golden file, columns only)
  • Drift detection verified: tampered golden file correctly triggers failure
  • Ruff lint and format clean
  • All traces under pre-commit 1000 KB limit
  • No sensitive data (timestamps stripped, no internal names)
  • Manual testing performed

Checklist

  • Commits are signed off (git commit -s) per DCO
  • Code follows project contributing guidelines
  • Documentation updated (if applicable)

@madhugoutham
madhugoutham force-pushed the feat/offline-ab-benchmark-template branch 5 times, most recently from 8e35baa to da434ea Compare July 23, 2026 00:41
@madhugoutham
madhugoutham marked this pull request as ready for review July 28, 2026 17:52
@madhugoutham
madhugoutham requested a review from ahg-g as a code owner July 28, 2026 17:52
@kaushikmitr

kaushikmitr commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

mostly looks good. couple of points both i think fixable in this PR:
The TPOT regression gate is dead code. Both arms train TPOT on identical features, so the gate can mathematically never fire. The PR body explicitly claims "fails on TTFT or TPOT regression" and lists it as test, that claim is false as written. Merging it gives reviewers of future feature PRs false assurance that TPOT contamination is being checked.

The parity test never tests the thing it exists for. test_parity.py compares production against production. The handwritten fallback, the path CI and external users would actually run, ships completely unvalidated, while the PR checklist says "production parity verified." If the fallback drifts, every CI A/B verdict is computed on the wrong feature set with no signal that it happened.

@madhugoutham

Copy link
Copy Markdown
Member Author

Good catches, both fixed. Removed the dead TPOT check, strengthened the parity test to verify actual values, and updated the README with the correct trace counts and seed differences.

@madhugoutham
madhugoutham force-pushed the feat/offline-ab-benchmark-template branch from 593b794 to 6150523 Compare July 30, 2026 21:10
"decode_tokens_in_flight",
"prefix_cache_score",
"effective_input_tokens",
"prefill_score_bucket",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add encoding related featurs

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kaushikmitr kaushikmitr Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you test this with test_parity.py?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, tested locally parity check passes with encoder features added.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am getting this error:
$ python benchmarks/test_parity.py
2026-08-03 19:03:40,386 - INFO - Initialized LatencyPredictor with model type: xgboost, objective: quantile, quantile: 0.9
PARITY CHECK FAILED — fallback diverged from production:
TTFT column mismatch:
production: ['is_queued', 'kv_cache_percentage', 'input_token_length', 'num_request_waiting', 'num_request_running',
'prefill_tokens_in_flight', 'decode_tokens_in_flight',
'prefix_cache_score', 'effective_input_tokens', 'prefill_score_bucket', 'pod_type_cat']
fallback: ['is_queued', 'kv_cache_percentage', 'input_token_length', 'num_request_waiting', 'num_request_running',
'prefill_tokens_in_flight', 'decode_tokens_in_flight',
'encoder_matched_size', 'encoder_input_size', ← only in fallback
'prefix_cache_score', 'effective_input_tokens', 'prefill_score_bucket', 'pod_type_cat']

Update _add_derived_features_fallback() and resolve*_features() fallback lists to match production.

Fails because production omits the encoder columns when LATENCY_ENABLE_ENCODER_FEATURES is unset (default false) while the benchmark fallback always includes them. It only passes with the flag exported.

Suggested fixes:
Add an _enc_enabled() mirroring LATENCY_ENABLE_ENCODER_FEATURES in the fallback, symmetric with the existing _tif_enabled() — so fallback and production agree in every env.
In test_parity.py, pin both flags explicitly (set the env vars before importing production settings) so the test is deterministic regardless of the caller's shell.
Move the GOLDEN_FILE.write_text(...) to after the comparison succeeds, or behind an explicit --update-golden flag.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow up: i made LATENCY_ENABLE_ENCODER_FEATURES true by default so the test_parity.py shold be fien against the default setting. Would still be good to have the _enc_enabled flag

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated test_parity.py to pin both feature flags before import and to write golden_columns.json only after the parity check passes. I also kept _enc_enabled() in the fallback, and re-ran python benchmarks/test_parity.py to confirm it passes with the current production default.

Comment thread benchmarks/test_parity.py

GOLDEN_FILE = Path(__file__).resolve().parent / "golden_columns.json"

SAMPLE_DF = pd.DataFrame(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t needs a handful of rows covering edge values (waiting=0, prefix_score 0 and 1, each pod_type) to actually pin down the formulas.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done expanded SAMPLE_DF to 5 rows covering waiting=0, prefix_score 0 and 1, and all three pod_type categories ("", "prefill", "decode").

@kaushikmitr

Copy link
Copy Markdown
Collaborator

/lgtm

@github-actions github-actions Bot added the lgtm label Aug 3, 2026
@kaushikmitr

kaushikmitr commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

lgtm. leaving approval to @Gregory-Pereira to confirm if this has all the hooks for #15 eventually

@madhugoutham
madhugoutham force-pushed the feat/offline-ab-benchmark-template branch from 88e7a3e to 332d73d Compare August 3, 2026 21:19
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Unsigned commits detected! Please sign your commits.

For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation.

Signed-off-by: Madhu Goutham Reddy Ambati <mambati@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants