Skip to content

fix(linalg): report AVX-512 test coverage - #8664

Open
lance-gatefixer[bot] wants to merge 3 commits into
mainfrom
gatekeeper/fix-8663-1
Open

fix(linalg): report AVX-512 test coverage#8664
lance-gatefixer[bot] wants to merge 3 commits into
mainfrom
gatekeeper/fix-8663-1

Conversation

@lance-gatefixer

@lance-gatefixer lance-gatefixer Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add an always-run x86 test that reports the runtime AVX, FMA, AVX2, and AVX-512 feature set in ordinary test logs
  • preserve the existing runtime-gated AVX-512 parity tests so capable hosts continue to execute them automatically
  • preserve the full random, boundary, extreme, and zero-vector coverage for the u8 VNNI kernels

Root cause

The AVX-512 feature checks run inside normal test bodies, so unsupported hosts return before the parity assertions and libtest otherwise gives no indication that those paths were skipped.

The new feature-report test writes directly to stderr, bypassing passing-test capture. Every normal x86 test run now records whether each required ISA tier is present, while the existing runtime gates retain automatic direct-kernel coverage on capable hosts.

Validation

  • cargo test -p lance-linalg test_x86_runtime_feature_report (feature report visible without --nocapture)
  • cargo test -p lance-linalg (219 passed, 0 ignored; all AVX-512 tests executed on the capable validation host)
  • cargo fmt --all -- --check
  • cargo clippy --all --tests --benches -- -D warnings

Fixes #8663

@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer bug Something isn't working labels Aug 20, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Aug 20, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

No code change was made: this Review explicitly classifies conditional capable-host CI as a non-blocking risk and says it is not required for #8663's observability fix. The verified current head keeps all 18 checks visible as issue-linked ignored tests, and the supported-host ignored suite passes all 18 tests.

@LuciferYang

Copy link
Copy Markdown
Contributor

I filed #8663, so here is a note on whether this closes it. Three things I checked and can back up, plus one consequence I read off the diff rather than measured.

The "conditional x86 CI pass" the comments reference isn't in the tree. About ten doc comments now read "The conditional x86 CI pass runs this ignored test when AVX-512F is available." This PR touches eight .rs files and no workflow. The only --ignored invocation anywhere under .github/workflows/ is nightly_run.yml:46, for lance-encoding jumbo tests. So the sentence is false at merge time, and it is the sort of false comment that blocks its own fix: the next person who considers adding that job will read it and assume the job exists.

Coverage goes from "sometimes" to "never". #[ignore] here is unconditional, so no default cargo test executes these on any host, including a developer machine that does have AVX-512 and did run them before. #8663 asked for two things: make the skip visible, and establish whether the feature is present anywhere. This delivers the first, and by doing it this way it fixes the answer to the second at "no coverage anywhere". The log becomes honest, which is real progress, but the kernels end up less tested than they were.

A consequence of the same mechanism, which I read off the diff rather than ran (my machine is aarch64, so all of this is cfg'd out for me): cargo test -- --include-ignored will now fail on any host without AVX-512, because the assert! fires instead of the test skipping. That is the natural invocation for anyone who reacts to this PR by trying to run the ignored set in CI.

The u8 VNNI kernels lose their edge cases. check_all_backends and check_all_backends_accum were each called from four test fns, covering random input (4 seeds x 18 sizes), extremes (max*max, max*0, 0*max, 0*0, alternating 0/max), and zero vectors (b=0, a=0). The three new standalone VNNI tests replicate only the random loop. AVX2 keeps all of it, since it stayed in the helper, so the shrink is one-sided. Those extremes are load-bearing: for l2_u8, all-255 against 0 is 255^2 * n, the largest accumulator the kernel can reach, and the u8-times-i8 asymmetry in vpdpbusd is exactly what they probe. Looping the new tests over the same case sets is a few lines.

Smaller point: all 18 ignore reasons link to #8663, while the body says Fixes #8663. After merge, every reason string points at a closed issue whose second half is still open. Better to point them at a tracking issue for the missing job.

Checked and not disputing: .cargo/config.toml pins x86_64-unknown-linux-gnu to x86-64-v2, so the two batch tests gated on not(all(target_feature = "avx2", target_feature = "fma")) do get compiled into a default build, and the "18 explicitly ignored" count matches my own grep of the tree (24 gates across 18 functions). The avx512 filter in the validation command does match all 18 test names.

If the goal is visibility without giving up coverage, one alternative: leave the runtime detection in place so capable hosts keep running the assertions, and add a single test that always runs and records the detected feature set. The log then distinguishes "ran" from "skipped" without relying on an opt-in path that no job currently uses.

@lance-gatefixer lance-gatefixer Bot changed the title fix(linalg): make AVX-512 test skips explicit fix(linalg): report AVX-512 test coverage Aug 20, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Addressed in df152c2. The 18 runtime-gated tests again run automatically on capable hosts, the u8 VNNI checks retain their full shared-helper edge cases, and ordinary x86 test logs now report the detected ISA features without relying on ignored tests or a nonexistent workflow path.

@lance-gatekeeper lance-gatekeeper Bot removed K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Aug 20, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 20, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 20, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gate recommendation: approve.

The base refresh leaves the reviewed patch unchanged. The always-run x86 feature report remains visible under ordinary test capture, while the runtime-gated AVX-512 and u8 edge-case tests continue to execute automatically on capable hosts.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-index Vector index, linalg, tokenizer bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: 18 AVX-512 tests in lance-linalg pass without asserting anything when the runner lacks the feature

1 participant