test_metal_mpp_equivalence can pass without varying the Metal4 route (non-discriminating on CUDA)
- Lenguaje dominante
- C
- Estrellas
- 22.3k
- Forks
- 2.1k
- Merge medio
- 1 d 3 h
- PR fusionados (30 d)
- 4
Descripción
## Summary
`test_metal_mpp_equivalence` is meant to compare a Metal4/TensorOps candidate against a legacy
reference. It never checks that the candidate arm actually took a *different* route, and there are
ordinary configurations, including on macOS, where it does not. When that happens the case can still
pass, and its log line still reads like an equivalence result.
The clearest instance is a CUDA build, where no compiled backend code reads the variable that is
supposed to separate the arms.
## How the arms are separated
```c
char *saved_disable_metal4 = test_save_env("DS4_METAL_DISABLE_METAL4");
setenv("DS4_METAL_DISABLE_METAL4", "1", 1);
ds4_engine *ref_engine = test_open_engine(false);
...
ds4_engine_close(ref_engine);
test_restore_env("DS4_METAL_DISABLE_METAL4", saved_disable_metal4);
test_run_mpp_candidate("auto", cases, ncase);
```
(`tests/ds4_test.c:5732`-`5755`)
The reference is forced to `1`. The candidate is not forced to anything: the caller's previous value
is **restored**, and the candidate is whatever that yields.
There is a `route=` field in the output that looks like it reports this, but it does not:
```c
static void test_run_mpp_candidate(const char *label,
test_mpp_eq_case *cases,
int ncase) {
fprintf(stderr, "ds4-test: Tensor equivalence candidate route=%s\n", label);
```
(`tests/ds4_test.c:5683`-`5686`)
`label` is caller-supplied, and the single call site passes the literal `"auto"`
(`tests/ds4_test.c:5755`), so the line prints `route=auto` whatever actually ran.
The two arms therefore differ only if the restored environment *and* the machine *and* the specific
workload all end up selecting the alternate path. They do not differ when:
- the caller already had `DS4_METAL_DISABLE_METAL4` set to a **disabling** value (both arms disabled,
on macOS too. Note `0`/`false`/`no`/`off` parse as *not* disabling, so this is narrower than "set");
- the Metal4/TensorOps path is unavailable at runtime: unsupported hardware, older SDK/runtime, or a
failed capability probe;
- the workload's tensor shapes/types do not select an alternate dispatch, or the specific alternate
pipeline falls back to the legacy kernel anyway. This one can happen even when Metal4 is globally
available, which is why availability is not the right thing to test;
- the test is run with `DS4_TEST_BACKEND=cpu`, which bypasses Metal even on a build that has it;
- the Metal backend is not in the build at all, which is the CUDA case below.
## The CUDA case
- the only non-test reader of the variable is `ds4_metal.m:2470`:
```c
const int metal4_disabled = ds4_gpu_env_bool("DS4_METAL_DISABLE_METAL4") > 0;
```
- `Makefile:27` puts `ds4_metal.o` in `CORE_OBJS` only inside `ifeq ($(UNAME_S),Darwin)`; the `else`
branch (`Makefile:53`) uses `ds4_cuda.o` instead. Ordinary native CUDA builds therefore do not
compile that Objective-C source and do not provide the Metal framework it imports at
`ds4_metal.m:2`.
- the `strix-halo` ROCm invocation (`Makefile:180`-`181`) likewise omits `ds4_metal.o` from its
`CORE_OBJS` override, though that target builds only the production binaries, not `ds4_test`, so
this is a source-level observation about the object list rather than a ROCm test run I have
exercised.
On such a build both engines are configured identically, so the case becomes a **same-route
repeatability comparison** rather than a Metal4-versus-legacy one, and the line at
`tests/ds4_test.c:5528`
```c
fprintf(stderr,
"ds4-test: Tensor equivalence %s top1 ref=%d cand=%d top5_overlap=%d/%d overlap=%d/%d max_rank_delta=%d rms=%g max_abs=%g top20_max_abs=%g\n",
```
can render with zero deltas throughout when the repeated execution agrees.
Worth stating carefully, because it cuts both ways. The arms are separately initialised engines
running separate inference, so zeros there are an **observed** result, not a structural identity.
CUDA has floating-point accumulation paths where execution order can matter. And the *logit-comparison*
pass flag is `nonfinite == 0 && same_top1` (`tests/ds4_test.c:5524`), so nonzero rms, max-abs and rank
deltas do not by themselves fail anything; the separate greedy-output assertions described below
still have to hold. Either way nothing here establishes cross-variant equivalence, because no variant
was varied.
## What a pass still means off the Metal4 path
Not nothing. The normal execution assertions still run (engine and session creation, synchronisation,
logit copying, evaluation, allocation, case loading), so it remains a real inference smoke test. The
*comparison-specific* assertions are:
```c
if (assert_thresholds) {
TEST_ASSERT(nonfinite == 0);
TEST_ASSERT(same_top1);
}
```
plus `cand_gen_len == tc->ref_gen_len` and the per-step `cand_gen[j] == tc->ref_gen[j]` in
`test_run_mpp_candidate`. `nonfinite == 0` is a genuine independent check; the rest now compare two
runs of the same configuration. So off the Metal4 path the comparison degrades to **greedy-output
repeatability plus logit finiteness**. Note that full-logit equality is logged but not asserted.
That is worth testing. The problem is only that it is filed under a name, and a log line, that claim
something stronger, so a passing `Tensor equivalence ... rms=0 max_abs=0` reads to a CUDA
contributor as "my change preserved numerics", which it cannot show.
## Suggestion
Key the case on whether the candidate arm **actually dispatched** the alternate route, recorded per
case rather than as a global availability flag, and report `N/A` (or skip) when it did not. That
covers the caller-disabled, unsupported-hardware, shape-fallback and not-compiled cases uniformly.
Conveniently the reporting slot already exists: populating `route=` with the observed route instead
of the literal `"auto"` would make the degenerate runs self-evident in the log even before any
skip logic is added.
Explicitly enabling Metal4 for the candidate is worth doing as well, but it is not sufficient on its
own, because clearing the disable switch cannot activate the path on unsupported hardware, an older SDK, or
after a failed probe, and those runs would go on passing without varying anything.
Happy to send a patch if you say which shape you prefer.
## Environment / how to reach it
sm_110 CUDA build (Jetson AGX Thor T5000, CUDA 13.0.48, L4T R38.4, aarch64). Note `make cuda` does
**not** build the test binary. It builds `ds4 ds4-server ds4-bench ds4-eval ds4-agent` (`Makefile:177`);
`ds4_test` is a separate target, and the case is selected with `--metal-tensor-equivalence`
(`tests/ds4_test.c:6432`).
Everything above is established by reading the source and the Makefile, which is why I have cited
line numbers rather than pasted a run: the conclusion does not depend on any particular execution.
Guía de contribución
Evaluación
Este issue todavía no se ha evaluado.