Skip to content

fix(python): accept numpy ivf_centroids without num_partitions - #9001

Open
LuciferYang wants to merge 2 commits into
lance-format:mainfrom
LuciferYang:fix/ivf-rq-centroids-numpy-guard
Open

fix(python): accept numpy ivf_centroids without num_partitions#9001
LuciferYang wants to merge 2 commits into
lance-format:mainfrom
LuciferYang:fix/ivf-rq-centroids-numpy-guard

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

Problem

The numpy ivf_centroids branch compared the centroid count against num_partitions in the same condition as the shape check. num_partitions defaults to None and is documented as deprecated in favor of target_partition_size, so a valid 2D array supplied without it failed, reported as "Ivf centroids must be 2D array" against a 2D shape.

Fixes #9000.

What this changes

The two checks are separate now: a non-2D array is still rejected with the same message, and the count is compared only when the caller set num_partitions, with a message that names both numbers. The Rust core already resolves the partition count from the centroids when num_partitions is absent.

The ivf_centroids docstring said num_partitions x dimension, which no longer describes how the count is derived; it now says the row count determines the number of partitions.

Test plan

test_pre_populated_ivf_centroids gets two cases. The first supplies a 7 cluster array with target_partition_size=250 and no num_partitions, then asserts the built index has 7 partitions. 7 tells it apart from the 5 partition index built earlier in the same test and from the 4 that target_partition_size would have produced. The second passes num_partitions=4 against the same array and asserts the new mismatch message.

Without the fix the first case raises ValueError: Ivf centroids must be 2D array: (clusters, dim), got (7, 128).

  • uv run pytest python/tests/test_vector_index.py 92 passed, 11 skipped, 1 failed
  • The failure is test_create_index_progress_callback_error_before_completion_propagates, which fails the same way with upstream's dataset.py on this machine, so it is not from this change.
  • uv run make lint clean

The numpy branch compared the centroid count against num_partitions unconditionally, but num_partitions defaults to None and is documented as deprecated in favor of target_partition_size. A valid 2D centroid array supplied without num_partitions therefore failed the shape check and was reported as "must be 2D array".

Split the two checks: reject a non-2D array, and compare against num_partitions only when the caller set it. The Rust core already resolves the partition count from the centroids when num_partitions is absent.
@github-actions github-actions Bot added A-python Python bindings bug Something isn't working labels Sep 4, 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: request changes.

The normal omitted-count path works, but the relaxed NumPy guard also admits a zero-row centroid matrix and exposes a Rust panic. Rejecting empty centroid sets at this boundary and covering that case will preserve the intended inference contract without turning malformed input into a PanicException.

len(ivf_centroids.shape) != 2
or ivf_centroids.shape[0] != num_partitions
):
if len(ivf_centroids.shape) != 2:

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.

An empty but 2-D array now passes this guard when num_partitions is omitted. For a 128-D dataset, np.empty((0, 128), dtype=np.float32) makes Rust derive zero IVF partitions and panic in rust/lance-index/src/vector/residual.rs:87, surfaced as PanicException: range end index 128 out of range for slice of length 0 instead of a normal input error. Reject shape[0] == 0 with a descriptive ValueError before constructing the Arrow batch, and cover it with a regression assertion; positive-row inference remains unchanged.

Reproducer
import tempfile

import lance
import numpy as np
import pyarrow as pa

rng = np.random.default_rng(0)
values = pa.array(rng.standard_normal(32 * 128).astype(np.float32))
vectors = pa.FixedSizeListArray.from_arrays(values, 128)
table = pa.Table.from_arrays([vectors], names=["vector"])
with tempfile.TemporaryDirectory(dir="/home/agent/tmp") as uri:
    dataset = lance.write_dataset(table, uri)
    try:
        result = dataset.create_index(
            "vector",
            index_type="IVF_PQ",
            metric="cosine",
            ivf_centroids=np.empty((0, 128), dtype=np.float32),
            num_sub_vectors=8,
        )
    except BaseException as exc:
        print(f"EDGE_RESULT=ERROR {type(exc).__name__}: {exc}")
    else:
        stats = result.stats.index_stats("vector_idx")
        print(f"EDGE_RESULT=SUCCESS {stats['indices'][0]['num_partitions']}")

Expected: a descriptive ValueError for zero centroids. Observed: PanicException: range end index 128 out of range for slice of length 0.

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.

Fixed in 859ce5f: zero-row NumPy centroids now fail with a descriptive ValueError before reaching Rust, and the added regression covers the former panic input.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 5, 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 zero-row panic path is fixed: invalid empty centroids now fail with a descriptive ValueError, while valid NumPy centroids still determine the IVF partition count when num_partitions is omitted. The added regression covers both behaviors.

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

Labels

A-python Python bindings 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: create_index rejects numpy ivf_centroids when num_partitions is omitted

1 participant