fix(linalg): enforce the cosine length contract - #8875
Conversation
|
Fixed, and the reproducer is now a test case. Both batch entries call
I checked the tightening against the callers before trusting it, since
|
|
The red Worth ruling out explicitly rather than asserting it, since that test does use |
|
Another review pass, three corrections. Two of the six batch cases were dead weight. The f64 reverse direction was missing, and it turned out to be the one case where the assert is the only protection: Two sentences in the test docs were false on some configuration. "Every |
|
Ran the benchmarks rather than reasoning about them, since
|
d011db8 to
681d6a9
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The latest revision replaces the tier-specific regression-test explanation with the durable reason the tests assert full operand lengths: dot can emit the same contract message on some paths. No executable code or assertions changed, and the focused mismatch test still passes.
What this changes
The five pairwise float
Cosineoverrides now callassert_equal_lengthsfirst:cosine_fastforbf16,f16,f32andf64, pluscosine_with_normsforf32. Bothcosine_batchentries, the trait default and thef32override, now callassert_batch_layout; thef32one previously approximated it withlet _ = batch.chunks_exact(dimension).Why
Each kernel behind the pairwise overrides derives one loop bound from one of its two arguments and then indexes both vectors with it. For the f32 and f64 kernels that bound is
x.len(); for the fp16 C kernels, which only exist with the non-defaultfp16kernelsfeature, it isy.len(), passed in asdimension. So a mismatch could reach a load past the end of the shorter vector, or return a distance computed over the shorter count, from safe Rust with nounsafeat the call site. Some pairs were already caught: where a kernel's tail handsdottwo slices of unequal length,dot's own assert fired. Which outcome a given pair got depends on the type, the tier, which argument is short and by how much. I am not tabulating that, because the entry assert makes the whole matrix unreachable.The batch path is measurable without that matrix. Built with
RUSTFLAGS="-C debug-assertions=off", a batch of dimension-8 targets with a length-3f32query returned large negative floats from all-small-positive inputs, becausecosine_once_8loads eight lanes from a three-element query. Those particular values are whatever was adjacent in memory, so they are one observation rather than something to reproduce. With this PR the same input panics atdistance.rs:43withdistance vector length must match dimension: vector=3, dimension=8, fromassert_batch_layout. l2 and dot already behaved that way, since both batch traits call that helper.Two of the three pairwise trait defaults need nothing of their own:
cosine_fastandcosine_with_normsbottom out incosine_scalarandcosine_scalar_fast, which go throughdot, which asserts. Thecosinedefault forwards toSelf::cosine_fast, so for the four float types it is covered by the asserts this pull request adds, andcosine_free_functions_reject_bad_inputis that path. Only the batch default gains a check.Where cosine sat relative to the other metrics
L2::l2andDot::dotassert in all five of their impls, and both batch traits callassert_batch_layout. Cosine's float entries are what this brings in line with them.Two gaps nearby stay open, and neither is this pull request's.
Cosine for u8overrides onlycosine, and nothing on that path checks the two lengths outside debug assertions; #8737 closes that, and its description is the place for which kernel does what. Hamming is the milder shape:hammingtruncates silently throughchunks_exactandzip, andhamming_distance_batchcarries onlydebug_assert_eq!, while what #8639 added tohamming_batch_u64is an always-on assert on result slots per target rather than on equal input lengths.Which assert a test catches
distance inputs must have equal lengthsexists in exactly one place in the crate, so that text alone cannot separate the assert this adds from the onedotalready had. The two lengths in the message can, wherever a kernel handsdotonly a scalar tail:dotthen reports 7 and 8 where the entry assert reports 15 and 16. That is an f32 path, and the f32 cases assert the fullleft=..., right=...for that reason. The f64 cases assert the lengths too, where they are redundant rather than load-bearing, since no f64 tail callsdot.Where a call reaches
cosine_scalarorcosine_scalar_fastinstead of a kernel,dotgets the same two slices and reports the same numbers, so no assertion on the message can tell the two asserts apart. That is f32 and f64 on an x86_64 host below theAvxtier, and f16 and bf16 either withoutfp16kernelsor with them whereverSIMD_SUPPORTnames no tier that has a compiled C kernel. The half-precision cases assert the contract only, and the test says so; the f32 and f64 cases keep asserting the two lengths even on the hosts where those numbers no longer separate the two asserts.Measured on this aarch64 host, one assert at a time. Deleting
f32::cosine_fast's failscosine_rejects_mismatched_lengths, and two of its cases would each catch it independently: long-then-short panics inside the kernel with a slice range error, which is the failure the run reports, and short-then-long reachesdot, which reportsleft=7, right=8against the assertedleft=15, right=16. Deletingf64::cosine_fast's also fails it, by a different route: long-then-short panics inside the kernel on the same out-of-range tail slice as f32, and short-then-long returns a value, because the f64 tail zips instead of callingdot.--target x86_64-apple-darwin, where Rosetta reports no AVX--features fp16kernels, derived rather than runf32::cosine_fast,f32::cosine_with_norms,f64::cosine_fastf16::cosine_fast,bf16::cosine_fastcosine_batchentriesThe batch cases need no length in the assertion.
must match dimension,divisible by dimensionandgreater than zeroexist only inassert_batch_layout, so deleting it fails all four cases on both targets. Three of them fail on this host by producing no panic at all: both key-length cases reachcosine_once_16, which reads a fixed sixteen lanes from the key without consulting its length, past the end of the short key and ignoring the surplus of the long one, and the remainder case is dropped bychunks_exact. The fourth panics somewhere else with another message, so that case fails on the message assertion rather than on the missing panic.Does this reject anything that used to work
Only inputs that were already wrong. The batch entries now reject
dimension == 0with a different message; both previous paths already panicked on it, thef32override through thechunks_exactprobe this replaces and the trait default through its ownchunks_exact, which asserts a non-zero chunk size in its constructor.Every in-workspace production caller passes two vectors of the same length:
lance-index's flat search througharrow_batch_func(), the twoarrow_batch_func()sites inindex/vector/builder.rs, the memtable brute-force scan,flat::storage'scosine_with_normsand itsdistance_fnarms,pq::storage'sbuild_pairwise_distance_table,multivec_distance, and the memtable HNSW store'scompute_f32_distance. The benches pass equal lengths too, though they are not production.lance-index's HNSW reachesflat::storagethroughdist_calculatorrather than calling cosine itself.The memtable HNSW store is worth naming separately for a different reason. Its two entries compare a query against a stored vector, or two stored vectors, and the same
compute_f32_distancesends L2 and Dot tol2_f32anddot_f32, which forward tof32::l2andf32::dot, whose entry asserts are always on. So a mismatch there would already panic under either of the other two metrics; cosine was the one with no assert.The flat search path also runs inside
spawn_blocking, so a panic there surfaces as an error rather than taking the process down.Test plan
cosine_rejects_mismatched_lengthsuses 16 and 15 rather than something smaller so the f32 and f64 cases reach a kernel body on this host rather than only a tail; the mutation results above are what establish that it catches a missing assert.cosine_batch_rejects_bad_layoutcovers the three layout conditions in four cases, key too long, key too short, batch remainder and zero dimension, forf32andf64, andcosine_free_functions_reject_bad_inputcoverscosine_distance, whichDistanceType::func()hands out, rather than only the trait method behind it.cargo test -p lance-linalg: 400 passed, 1 ignoredcargo test -p lance-linalg --target x86_64-apple-darwin: 459 passed, 1 ignored. That target is where the sub-AVX2cosine_batchbranch is live, andtest_cosine_batch_matches_per_vectorhas adim_8case, so the droppedx.len() >= 8conjunct is covered by an executing test rather than only a compile checkcargo test -p lance-index: 1180 passed and 3 ignored in the lib, 9 doc-testscargo fmt --all -- --checkandcargo clippy -p lance-linalg --all-targets -- -D warnings: cleanNot run: the AVX, AVX+FMA and AVX-512 kernels themselves. I have no x86 host, and Rosetta reports no AVX at all, so on the
x86_64-apple-darwintargetSIMD_SUPPORTisNoneand those arms are unreachable even though that target's test run is real. What those kernels do on a mismatch comes from reading them.