(#3) test: shared E2E lib + configurable matrix - #58
Conversation
5ceb2be to
017720d
Compare
5e2cbef to
0fd4692
Compare
017720d to
6c8f3a6
Compare
0fd4692 to
c2950b2
Compare
Phases 2-3 of the test-suite refactor (plan §7, §8 / PR3). Additive only: the matrix runs alongside the existing tests and nothing is deleted. tests/lib/ — shared E2E library (§7), consolidating logic that vllm_identical_comparator / compare_disk_vs_ch / vllm_rowcnt_comparator / hf_comparator each reimplemented: - compare.py: Check dataclass + the four standards (bitwise, allclose, row_count, transport_bitwise) as one interface; records max/mean abs + first-diff position even on a pass so "barely passing" stays visible. - align.py: left-pad strip, EOS trim, request_id "<gid>:<row>" parsing, vLLM UUID-suffix normalization, logits head-skip alignment. - report.py: CellResult dataclass -> JSON record / JSONL artifact + human table; legacy comparator result.json adapter. - clickhouse_io.py: canonical dtype map + short-hook->CH act_name map + row decode / per-hook counts / request grouping (clickhouse_driver lazy). - segments.py: thin re-export of monitoring.segment_merger + a sorted per-request chunk merge helper. - disk_ref.py: parse RefDiskWorker .pt filenames; load HF structured refs. - hf_reference.py: stable-named re-export of the ROL/GEN HF rollouts (the 700-line module relocation is deferred to the legacy-removal PR). tests/e2e_matrix.py — configurable matrix entry point (§8) replacing the hardcoded shell sweeps: - Multi-value axes (backend/model/mode/standard/hooks/tp/ring/dtype/ prompt-set) -> Cartesian product of cells. - Dispatches each cell to the existing runners + comparators as subprocesses (no inference logic reimplemented): vLLM bitwise/transport -> ref+monitored+vllm_identical_comparator; vLLM row_count/allclose -> monitored+vllm_rowcnt_comparator; HF -> ref+monitored+hf_comparator. - Translates the public E2E_HOOK_SELECTION input to internal DMX_HOOK_SELECTION per subprocess (plan §2). - One JSONL record per cell; per-cell isolation so one bad cell can't abort the matrix; per-subprocess --cell-timeout so a hung runner fails the cell. - --dry-run prints planned cells + dispatch commands with no CUDA/CH, so expansion and env translation are CPU-testable. tests/test_e2e_lib.py — 37 cpu-marked unit tests covering the four standards, align helpers, report serialization/round-trip, and matrix cell expansion / env translation / dispatch planning / dry-run. Acceptance: `pytest -m "not gpu and not e2e and not manual"` — new suite green (37 passed; 108 total passing on this checkout). Pre-existing failures stem from the unbuilt monitoring native layer (HOOK_TYPE_ROUTER_LOGITS) and are unrelated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6c8f3a6 to
2d75054
Compare
| "--ref-config", config_file, "--mon-dir", mon_dir, | ||
| "--result-file", result_file))) | ||
| return steps, "tests.vllm_identical_comparator", result_file | ||
| # row_count / allclose -> monitored-only + rowcnt comparator |
There was a problem hiding this comment.
allclose is advertised as a matrix standard, but this branch routes it to the row-count comparator. In run_cell(), the same non-identical vLLM path also writes ref/meta.json with {"skipped": true}, so vllm_rowcnt_comparator skips value comparison and vllm + allclose behaves like row-count-only. Please either reject/document this unsupported combination or wire an actual allclose comparison path.
There was a problem hiding this comment.
Fixed. allclose is no longer a valid vLLM standard in plan_cell(), so now the code now raises ValueError at plan time with a message listing the supported standards (bitwise, transport_bitwise, row_count). The matrix cannot produce a vLLM+allclose cell; it will fail before any subprocess is launched. The module docstring documents the supported combinations.
| "--result-file", result_file))) | ||
| return steps, "tests.vllm_rowcnt_comparator", result_file | ||
|
|
||
| if cell.backend == "hf": |
There was a problem hiding this comment.
HF ignores cell.standard here: bitwise, row_count, and allclose all run the same hf_comparator, whose behavior is the existing max_abs <= E2E_TOLERANCE tolerance check. That makes the advertised matrix standard misleading for HF. Please either limit HF to the standard that is actually implemented, or make the comparator path standard-specific.
There was a problem hiding this comment.
Fixed. HF now accepts only allclose with any other standard raises ValueError at plan time. hf_comparator also receives --standard so it acts on it rather than ignoring it. The matrix cannot claim a HF+bitwise or HF+row_count test; those combinations are rejected before any subprocess runs.
|
At the matrix level, the advertised
I do not think this PR should merge as-is. Please either fill in the missing test/comparator behavior so the advertised matrix standards are real, or restrict/validate the supported backend+standard combinations so the matrix cannot report misleading cells. |
Phase 4 of the test-suite refactor (plan §5 / PR4). The high-risk switch: the vLLM and HF E2E tests become thin wrappers on the configurable matrix (PR3), and the stale in-process bodies are deleted. Net -1991/+216 lines. Wrapper support in tests/e2e_matrix.py: - matrix_argv_from_env(): translate the legacy E2E_* env knobs (model, enforce-eager, dtype, ring, hook-selection, tolerance, db, ...) into a single-cell matrix argv, so wrappers honor exactly what the old tests did. - run_single(): parse argv to one cell, run it, return the CellResult (refuses anything but a single cell). test_e2e_correctness_vs_hf.py: 1667 -> ~85 lines. - test_e2e_correctness_hf -> matrix hf/eager/allclose cell. - test_e2e_cuda_graphs_vs_eager_hf -> matrix hf/cuda_graph/allclose cell, default tolerance 0.5 (eager ref vs CUDA-graph monitored). - Deleted: _test_e2e_correctness_hf_legacy and _test_e2e_cuda_graphs_vs_eager_hf_legacy (uncollected "kept for reference" copies), the permanently-disabled @skipif(True) test_e2e_correctness_hf_cuda_graphs (its compiled-rollout reference could not replicate generate()'s StaticCache handling and was explicitly superseded -- no still-passing assertion lost), and the now-unused in-process helpers (_make_ring_cfg, _make_host_cfg, get_num_layers_from_config, _canon_layer_and_act, bitwise_equal, ...). test_vllm_identical.py (262 -> ~55) -> matrix vllm/bitwise cell; test_vllm_rowcnt.py (117 -> ~50) -> matrix vllm/row_count cell. All four public test NAMES are preserved (tests/tools/verify_hf.sh, verify_vllm.sh, identical_vllm.sh invoke them by node id). Skips are now precise via tests/_requirements (require_cuda/require_vllm/require_clickhouse) instead of ad-hoc torch.backends.cuda checks. Matrix-parity (static dispatch) — each wrapper drives the identical runner+comparator chain the old test did: - test_e2e_correctness_hf : hf_reference_runner + hf_monitored_runner(eager) + hf_comparator - test_e2e_cuda_graphs_vs_eager_hf: hf_reference_runner(eager) + hf_monitored_runner(CG) + hf_comparator(tol=0.5) - test_vllm_identical : enable_ref_hooks + vllm_ref_runner + vllm_monitored_runner + vllm_identical_comparator - test_vllm_rowcnt : vllm_monitored_runner + vllm_rowcnt_comparator The numeric GPU/ClickHouse parity run (old result vs new JSONL) must be pasted on the PR before merge per the plan's PR3-4 gate. Acceptance: `pytest -m "not gpu and not e2e and not manual"` green (116 passed; +6 wrapper-translation unit tests). All four wrappers collect cleanly. Pre-existing failures stem from the unbuilt monitoring native layer (HOOK_TYPE_ROUTER_LOGITS) and are unrelated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `-m multi_gpu` suite documented in docs/testing.md collected zero tests -- no test carried the `multi_gpu` marker, so TP coverage existed only in the CPU shape-math unit test and the manual tests/tools sweeps. Add tests/test_e2e_tp2.py: two wrappers that drive the configurable matrix at tp=2 (vLLM transport_bitwise + HF eager allclose), forcing E2E_TP_SIZE=2 via matrix_argv_from_env's env override. Marked multi_gpu/gpu/e2e/clickhouse (+vllm/+hf) and skip-guarded with require_gpus(2)/require_vllm/require_clickhouse so a <2-GPU runner skips with a reason instead of failing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ship the per-hook numeric-difference study (plan §9 / Phase 5): enable one
hook at a time and report the drift its monitoring path introduces versus the
unhooked baseline. Non-goal preserved -- make drift visible/categorized/
reproducible, not fixed. Builds on the PR3 shared lib and the PR2 hardened
isolation contract.
- tests/numeric_study.py: configurable CLI + run_study() orchestration.
Captures the unhooked baseline once, then one monitored rollout per hook
(clean subprocess each, full [N, vocab] logits) and records per hook the
shared-lib Check verdict, max/mean abs diff, first differing token position,
top-k vocab diffs at that position, and greedy token-id divergence. Emits a
JSON artifact + human table; non-zero exit on any alert.
* Reuses tests.lib.compare standards as the §8 standards-by-mode policy:
eager -> bitwise, cuda_graph -> allclose(atol=per-model/dtype threshold).
* --variant p (default): production _p Hooked + hook_selection=H (the
hook_selection mechanism isolates a single hook).
* --variant compare: _compare variant run inside the PR2 hardened
tests.isolate_hook.isolated_hook context manager (byte-identical
restore), so only hook H's .copy_() fires.
- Alert policy: eager -> any non-bitwise diff; cuda_graph -> max abs over
threshold; always on greedy token divergence, runner error, shape mismatch
(possible hook identity swap), or empty capture.
- The comparison/alert/report core is pure and CPU-testable; torch is imported
lazily so the module loads on a torch-less box.
- tests/test_numeric_study.py: 12 CPU unit tests for the verdict logic +
one numeric/gpu/slow GPU smoke asserting serializable per-hook records and a
self-consistent eager verdict.
Acceptance (CPU): python -m pytest tests/test_numeric_study.py -m cpu -q
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| def _tp2_env(default_model: str = "qwen3") -> dict: | ||
| """os.environ with TP forced to 2 (model still overridable via E2E_MODEL).""" | ||
| env = dict(os.environ) | ||
| env["E2E_TP_SIZE"] = "2" | ||
| env.setdefault("E2E_MODEL", default_model) | ||
| return env | ||
|
|
||
|
|
||
| def _assert_cell(subtests, cr) -> None: | ||
| """Fail on a dispatch error; report each matrix check as a subtest.""" | ||
| if cr.error: | ||
| pytest.fail(f"matrix cell errored: {cr.error}") | ||
| assert cr.checks, "matrix produced no checks" | ||
| for chk in cr.checks: | ||
| with subtests.test(chk.name): | ||
| assert chk.passed, chk.detail | ||
|
|
||
|
|
||
| @pytest.mark.vllm | ||
| @require_gpus(2) | ||
| @require_vllm() | ||
| @require_clickhouse() | ||
| def test_vllm_identical_tp2(subtests) -> None: | ||
| """vLLM TP=2 transport-bitwise: reference D2D buffers vs ring -> ClickHouse. | ||
|
|
||
| Equivalent matrix cell: ``--backend vllm --standard transport_bitwise --tp 2``. | ||
| The bitwise standard stays exact under TP (sharding is a layout change, not a | ||
| numeric one), so any per-rank reassembly bug surfaces as a non-zero max_abs. | ||
| """ | ||
| argv = matrix_argv_from_env("vllm", "transport_bitwise", env=_tp2_env()) | ||
| _assert_cell(subtests, run_single(argv)) | ||
|
|
||
|
|
||
| @pytest.mark.hf | ||
| @require_gpus(2) | ||
| @require_clickhouse() | ||
| def test_e2e_correctness_hf_tp2(subtests) -> None: | ||
| """HF TP=2 eager: hooked model (ring -> ClickHouse) vs original model. | ||
|
|
||
| Equivalent matrix cell: ``--backend hf --mode eager --standard allclose --tp 2``. | ||
| """ | ||
| argv = matrix_argv_from_env("hf", "allclose", mode="eager", env=_tp2_env()) |
There was a problem hiding this comment.
This block presents the HF test as TP=2 coverage: _tp2_env() sets E2E_TP_SIZE=2, the test requires 2 GPUs, and the docstring/cell says --tp 2. However, the HF matrix path invoked here goes through hf_reference_runner and hf_monitored_runner, and those runners do not read E2E_TP_SIZE, do not launch under torchrun, and do not pass tp_plan="auto". So this appears to require 2 GPUs without actually exercising HF tensor parallelism. Please either route this through a real HF TP execution path or remove/rename this test so it does not claim HF TP=2 coverage.
There was a problem hiding this comment.
Fixed. test_e2e_correctness_hf_tp2 has been removed. The module docstring now explicitly states that HF TP=2 is not covered here because hf_reference_runner/hf_monitored_runner do not read E2E_TP_SIZE, do not launch under torchrun, and do not pass tp_plan="auto", so a 2-GPU HF test would be misleading. Only test_vllm_identical_tp2 remains, which genuinely exercises TP=2 sharding.
| def _run_rollout( | ||
| *, | ||
| framework: str, | ||
| model_key: str, | ||
| hook: str, | ||
| mode: str, | ||
| variant: str, | ||
| rollout: str, | ||
| out_path: Path, | ||
| max_new_tokens: int, | ||
| prompt: str, | ||
| env: dict, | ||
| timeout: int = 600, | ||
| ) -> None: | ||
| """Spawn one rollout subprocess; raise with captured output on failure.""" | ||
| runner = _HF_RUNNER if framework == "hf" else _VLLM_RUNNER | ||
| cmd = [ | ||
| sys.executable, "-c", runner, | ||
| "--model-key", model_key, | ||
| "--hook", hook, | ||
| "--mode", mode, | ||
| "--variant", variant, | ||
| "--rollout", rollout, | ||
| "--max-new-tokens", str(max_new_tokens), | ||
| "--prompt", prompt, | ||
| "--out", str(out_path), | ||
| ] |
There was a problem hiding this comment.
_run_rollout() also does not pass dtype to either child runner. Combined with the CLI --dtype argument below and the StudyResult(dtype=...) field, this makes dtype metadata-only today. Please include dtype in the subprocess contract if it is meant to be a real axis, or drop it from the CLI/report for now.
There was a problem hiding this comment.
Fixed. _run_rollout() now accepts dtype as an explicit parameter and passes --dtype to the child subprocess. dtype is a real execution axis, not metadata-only.
| _HF_RUNNER = dedent(""" | ||
| import argparse, os | ||
| import torch | ||
|
|
||
| ap = argparse.ArgumentParser() | ||
| ap.add_argument('--model-key', required=True) | ||
| ap.add_argument('--hook', required=True) | ||
| ap.add_argument('--mode', required=True) | ||
| ap.add_argument('--variant', required=True, choices=['p', 'compare']) | ||
| ap.add_argument('--rollout', required=True, choices=['baseline', 'hooked']) | ||
| ap.add_argument('--max-new-tokens', type=int, default=4) | ||
| ap.add_argument('--prompt', default='Hello') | ||
| ap.add_argument('--out', required=True) | ||
| args = ap.parse_args() | ||
|
|
||
| MODEL_ALIASES = { | ||
| 'gpt2': 'gpt2', | ||
| 'qwen3': 'Qwen/Qwen3-0.6B', | ||
| 'qwen2_moe': 'Qwen/Qwen1.5-MoE-A2.7B', | ||
| 'llama': 'meta-llama/Llama-3.1-8B', | ||
| } | ||
| hf_id = MODEL_ALIASES[args.model_key] | ||
| device = torch.device('cuda') | ||
| dtype = torch.float16 |
There was a problem hiding this comment.
The child HF runner does not parse a dtype argument and hardcodes dtype = torch.float16. This means the public --dtype option can change the recorded result/threshold selection without changing the actual HF rollout dtype. Please either wire dtype into the child runner or remove/limit the public dtype option until it is real.
There was a problem hiding this comment.
Fixed. The child HF runner now parses --dtype and resolves it through _DTYPE_MAP = {'float16': torch.float16, 'bfloat16': torch.bfloat16, 'float32': torch.float32}. The public --dtype option now controls the actual rollout dtype end-to-end.
| llm = LLM(**llm_kwargs) | ||
| params = SamplingParams(temperature=0.0, max_tokens=args.max_new_tokens, logprobs=20) | ||
| outputs = llm.generate([args.prompt], params) | ||
|
|
||
| completion = outputs[0].outputs[0] | ||
| ids = list(completion.token_ids) | ||
| step_logprobs = completion.logprobs or [] | ||
| # Dense [N, V] from the sparse top-k logprob dicts; unreported entries stay | ||
| # at a large negative floor (sufficient for chosen-token + top-k drift). | ||
| vocab = int(getattr(llm.llm_engine.model_config.hf_config, 'vocab_size', 0)) or 1 | ||
| N = len(ids) | ||
| logits = torch.full((N, vocab), -1e30, dtype=torch.float32) | ||
| for i in range(N): | ||
| if i < len(step_logprobs) and step_logprobs[i] is not None: | ||
| for tid, lp in step_logprobs[i].items(): | ||
| logits[i, int(tid)] = float(lp.logprob) | ||
| token_ids = torch.tensor(ids, dtype=torch.int64) | ||
| torch.save({'token_ids': token_ids, 'logits': logits}, args.out) |
There was a problem hiding this comment.
This implementation confirms the vLLM path is top-k logprobs, not full-vocab logits: logprobs=20 returns sparse per-step logprob dicts, then the code creates a dense-shaped tensor and fills unreported entries with -1e30. That can be acceptable if vLLM only exposes top-k here, but then metrics like vocab-wide mean_abs and any “full logits” wording are misleading. Please either report this as top-k logprob drift or restrict the metrics to values that are meaningful for sparse top-k capture.
| # Subprocess rollout runners (full-logits capture) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| # Each rollout subprocess saves {token_ids: int64[N], logits: float32[N, vocab]}. | ||
| # Same on-disk shape for HF and vLLM so the comparison code is framework-agnostic. |
There was a problem hiding this comment.
This section describes the rollout artifact as full [N, vocab] logits for both HF and vLLM. That is not true for the vLLM runner below, which requests top-k logprobs and fills missing vocab entries with -1e30. Please split the artifact semantics by framework or describe the vLLM artifact as sparse/top-k logprob based rather than full logits.
There was a problem hiding this comment.
Fixed. The section comment now splits artifact semantics by framework: HF saves full [N, vocab] logits; vLLM saves the same shape but only top-k logprob entries are real, with non-reported positions filled to -1e30. The module docstring states this upfront. compute_drift() uses sparse_floor=-1e30 to mask the vLLM tensor so all drift metrics (max_abs, mean_abs, top-k vocab diffs) are restricted to entries where at least one runner reported a real logit.
Samfisheryu
left a comment
Samfisheryu
left a comment
There was a problem hiding this comment.
I merged the contents of #59 and #60 into this PR and closed those two PRs, so we can review the shared E2E matrix, wrapper migration, and numeric study together here.
The main things to fix before this can merge are:
-
Align the
standardaxis with the actual comparator behavior. Right now some standards are routed to comparators that do not actually implement that standard, especiallyallclose/row_counton the vLLM side and non-allclose standards on the HF side. -
Align the matrix with the test implementation. If a combination is listed in the matrix, it should either be genuinely implemented and tested, or explicitly rejected/skipped with the right semantics.
I also left inline comments for the issues introduced by the merged #59/#60 content:
- the HF TP=2 test currently requires 2 GPUs but does not appear to actually run HF tensor parallelism;
- the vLLM numeric-study path captures top-k logprobs, not full logits, so the wording/metrics should reflect that;
--dtypeis currently metadata-only and should either be wired into the child runners or removed/limited.
This PR is not ready to merge yet. Please address the issues above first.
2 participants
Summary
This PR introduces a reusable E2E comparison library and a configurable E2E matrix harness to eliminate duplicated comparator logic, improve observability, and provide robust matrix execution with per-cell isolation.
The changes are intentionally additive, preserving existing runners and inference paths while consolidating shared functionality.
What's Included
Shared E2E Comparison Library
Consolidates logic that was previously reimplemented across four separate comparators.
compare.pyProvides a unified comparison interface supporting:
bitwiseallcloserow_counttransport_bitwiseAlso supports
Checkdataclass comparisons.Additional diagnostics are recorded even for passing comparisons:
max_abs_diff)mean_abs_diff)This provides visibility into "barely passing" comparisons and enables future trend analysis.
align.pyCentralizes alignment logic including:
<group_id>:<row_id>)report.pyStandardizes reporting functionality:
CellResult→ JSON record conversionresult.jsoncompatibility adapterclickhouse_io.pyIntroduces shared ClickHouse utilities:
act_namemappingsAdditional improvements:
clickhouse_driveris imported lazily to preserve CPU importability.Supporting Modules
segments.pymonitoring.segment_mergerdisk_ref.py.ptreference file parsinghf_reference.pytests/e2e_matrix.py— Configurable HarnessMatrix Expansion
Supports multi-value axes that expand into a Cartesian product of execution cells.
Each generated cell dispatches to the existing runners and comparators as subprocesses. No inference logic is reimplemented.
Environment Variable Translation
Maintains the environment contract introduced in PR #57 :
Translation occurs independently for each subprocess.
Per-Cell Isolation
Each matrix cell executes independently with:
Timeout Protection
Introduces
--cell-timeout.If a runner hangs:
ERRORDry-Run Mode
Introduces
--dry-run, which:This enables CPU-only testing of matrix expansion behavior.
Verification
tests/test_e2e_lib.py)ERROR