Skip to content

fix(index): serialize spill-sort tests and skip NaN centroid initialization - #8819

Closed
u70b3 wants to merge 2 commits into
lance-format:mainfrom
u70b3:fix/json-sort-mem-pressure
Closed

fix(index): serialize spill-sort tests and skip NaN centroid initialization#8819
u70b3 wants to merge 2 commits into
lance-format:mainfrom
u70b3:fix/json-sort-mem-pressure

Conversation

@u70b3

@u70b3 u70b3 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Two CI flake fixes from the consolidated tracking in #8789:

  1. Spill-pool contention (test_json_btree_index_null_at_path failing with "Not enough memory to continue external sort" — 3 occurrences on 2026-08-26 across independent PRs, all in the linux-arm job).
  2. Torch IVF NaN centroid init (test_torch_index_with_nans failing with StopIteration — 4+ occurrences across PRs and Python versions; confirmed in fix(index): make IVF-PQ test training deterministic #8767 to be a separate, uncovered root cause).

Fix 1: serialize spill-sort tests against the shared memory pool

Root cause

  • Every spill-enabled SortExec reserves a non-spillable 40MB merge buffer on its first input batch (sort_spill_reservation_bytes = min(pool/3, 40MB) in rust/lance-datafusion/src/exec.rs), so the default 150MB FairSpillPool fits exactly three concurrent spill sorts; a fourth always fails with ResourcesExhausted.
  • Spill sessions/pools are cached process-wide (get_session_context), so the collision domain is the process. The error text confirms it: three foreign ExternalSorterMerge reservations of 40MB each + the victim's own 4-row sorter, 30MB remaining, 40MB requested.
  • nextest runs each test in its own process → cross-test contention is impossible there. The linux-arm job runs plain cargo test → the whole lance-index lib binary shares one process, one cached session, one 150MB pool. Any four of the crate's 15 spill-sort tests in flight can exhaust it — which is why all three observed failures were linux-arm only, and intermittent (the reservations are held for ~ms, so collisions need contended runners).
  • FLOAT_INDEX_CASE_GUARD already serialized two json tests for exactly this reason (fix(index): sort JSON-path values once after extraction, not the raw column #7835), but the rtree tests (8), the btree-update tests (2), and json's own update tests (3) were unguarded.

Fix

Promote the guard to a crate-wide SPILL_POOL_TEST_GUARD (documented at the definition) and hold it in every test that drives a spill-enabled execution: json (5), rtree (8), btree update (2). Under cargo test this serializes them — one 40MB reservation against a 150MB pool, exhaustion impossible by construction. Under nextest it is a no-op. No production code touched.

Fix 2: skip non-finite vectors in torch IVF centroid initialization

Root cause

test_torch_index_with_nans writes 320 vectors, 8 of them float-NaN (not Arrow nulls, so the vector is not null filter doesn't remove them). In train_ivf_centroids_on_accelerator the initial centroids are drawn from a random sample (python/python/lance/vector.py):

  1. With num_partitions=1, the init draw is a single row — P(NaN row) = 8/320 = 2.5% per run, matching the observed intermittency.
  2. A NaN centroid makes every distance NaN (_l2_distance maps NaN → partition id -1), so kmeans reports total_dist == 0 and "converges" on the first epoch without ever updating the centroids.
  3. compute_partitions then drops every row (partitions >= 0 mask), the residuals dataset ends up empty, and the PQ init sample yields zero batches — next(iter(ds_init)) raises StopIteration.

This is a production-path bug, not just a test issue: any NaN-containing vector column + accelerator one-pass IVF-PQ could produce an empty index or the cryptic StopIteration.

Fix

New _sample_finite_vectors helper (python/python/lance/vector.py): draws init vectors from the sample stream, skipping rows with any NaN/inf values, until k finite vectors are collected; raises a descriptive ValueError (with scanned/available counts) when the data can't provide them. Applied in train_ivf_centroids_on_accelerator when filter_nan is set — extending that parameter's existing intent (it already filtered Arrow nulls) to float-NaN values.

Verification

  • Fix 1: the failure needs ≥4 concurrent reservations held for ~ms each; 48 rounds of forced-overlap runs on a 128-core workstation never reproduced it (needs CI's contended ARM runners) — the guarantee is structural: pool capacity is exactly 3 concurrent sorts by construction, and the guard caps test concurrency at 1. cargo test -p lance-index --lib --features geo ×3: 1145 passed each; targeted spill tests ×5 (--test-threads=16): 28 passed each; linux-arm CI job on this PR green (the original failure environment).
  • Fix 2: new regression tests in python/tests/test_vector_index.py — deterministic pre-fix failure / post-fix pass (verified by reverting the change): init batch is all-NaN in storage order; all-NaN column raises the descriptive error; helper unit test covers tensor and dict batch forms. The original test_torch_index_with_nans[V3/Legacy] passes locally.
  • uv run make lint · cargo clippy -p lance-index --all-features --tests -- -D warnings · cargo fmt --all

Alternatives considered

  • Fix 1 — bump LANCE_MEM_POOL_SIZE only on the linux-arm job: works, but hides the hazard in CI config and doesn't tell future test authors why their new spill test flakes. Switch linux-arm to nextest: process isolation eliminates this whole class, but that's a CI-policy call for maintainers; this fix is correct under either runner.
  • Fix 2 — seed the sampler in the test fixture: masks the production bug and still leaves unseeded users exposed (and fix(index): make IVF-PQ test training deterministic #8767 already covers seeding for the Rust path; it explicitly does not cover this Python one-pass sampler).

Partially addresses #8789 (families 1 and 2 of the consolidated list).

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 27, 2026
@github-actions github-actions Bot added A-python Python bindings and removed A-python Python bindings labels Aug 27, 2026
@u70b3 u70b3 changed the title test(index): serialize spill-sort tests against shared memory pool fix(index): serialize spill-sort tests and skip NaN centroid initialization Aug 27, 2026
@github-actions github-actions Bot added the bug Something isn't working label Aug 27, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 27, 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 27, 2026
@u70b3

u70b3 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@Xuanwo could you take a look when you have a moment? This PR fixes the two most frequent flake families from #8789:

  1. linux-arm spill-pool OOM — structural fix: serializes all 15 spill-sort tests in lance-index against the process-wide pool (each spill SortExec locks 40MB of the 150MB pool; cargo test's shared process made ≥4 concurrent reservations possible).
  2. torch NaN centroid StopIteration — production-path fix in one-pass IVF-PQ init: a float-NaN vector row could be drawn as an init centroid (2.5% for the fixture), collapsing kmeans and emptying the residuals dataset. _sample_finite_vectors now skips non-finite rows.

Worth noting the second one is also blocking lance-gatefixer's #8767, so landing this unblocks that pipeline too. CI is fully green, including linux-arm and the Python jobs that exercise both original failure environments.

@u70b3
u70b3 force-pushed the fix/json-sort-mem-pressure branch from d30d02d to b2298ac Compare August 28, 2026 09:06
@github-actions github-actions Bot added the A-python Python bindings label Aug 28, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 28, 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 rebase leaves both fixes unchanged at their intended boundaries: spill-enabled tests are serialized around the process-wide pool, while accelerated IVF initialization excludes non-finite candidates and reports insufficient finite data clearly.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 28, 2026
Xuanwo added a commit that referenced this pull request Aug 31, 2026
## Problem

Plain `cargo test` runs `lance-index` tests in a single process.
Spill-enabled index builds therefore share the cached 150 MiB DataFusion
memory pool, and concurrent 40 MiB `ExternalSorterMerge` reservations
can exhaust it. This caused the [Linux ARM main
job](https://github.com/lance-format/lance/actions/runs/33294579023/job/99211936332)
to fail in an otherwise unrelated JSON index test.

## Change

Put all 15 spill-sort tests in the named `LANCE_DF_SPILL_POOL`
`serial_test` resource group. This preserves the production memory-pool
configuration while keeping unrelated tests parallel, and replaces the
JSON-only mutex with one shared test resource.

This supersedes the Rust test-isolation portion of #8819. That PR
currently conflicts and also bundles the already-landed Python NaN fix.

## Validation

- `cargo test -p lance-index --lib --features geo -- --test-threads=16`
(3 consecutive runs, 1191 passed / 0 failed each)
- `cargo clippy --all --tests --benches -- -D warnings`
- `cargo fmt --all -- --check`
@u70b3

u70b3 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded: the Python NaN-centroid fix landed in #8782, and the spill-sort test serialization landed in #8892 (thanks @Xuanwo). Nothing unique remains here — for future flake fixes from a tracking issue like #8789 I'll claim a family in the issue first and keep PRs single-purpose.

@u70b3 u70b3 closed this Aug 31, 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 A-python Python bindings bug Something isn't working chore K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant