Skip to content

[MOD-14959] Add SQ8 Python bindings and initial flow tests - #1048

Open
dor-forer wants to merge 2 commits into
mainfrom
dor-forer-MOD-14959-sq8-python-flows
Open

dor-forer wants to merge 2 commits into
mainfrom
dor-forer-MOD-14959-sq8-python-flows

Conversation

@dor-forer

@dor-forer dor-forer commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Describe the changes in the pull request

Expose SQ8 configuration through the VecSim Python bindings and exercise the tiered HNSW training and migration lifecycle. Targets main.

  • Expose VecSimQuantType, HNSWParams.quantType, and TieredHNSWParams.QuantNormalizationSetSize.
  • Add optional, keyword-only quantization_mean to HNSWIndex, Tiered_HNSWIndex, and generic VecSimIndex with HNSW parameters. Validate shape, dimension, and finite float32 values; retain the buffer through construction while native code copies it. A positive tiered training threshold takes precedence over the supplied mean.
  • Reject null creation results in the generic and HNSW Python constructors with ValueError, including safe tiered HNSW worker-context cleanup.
  • Add testing controls manual_jobs and _run_pending_jobs for deterministic partial-migration checks. Live batch iterators must be depleted or destroyed before draining jobs.

Coverage

  • FLOAT32/FLOAT16 x L2/IP/cosine x single/multi, with zero and positive training thresholds. Plain HNSW cosine remains unsupported; tiered cosine is supported through normalization.
  • Before training, results must match an equivalent native flat index, including datatype-specific arithmetic and equal-distance ties. After asynchronous migration, KNN checks enforce result shape, unique valid labels, finite scores, and score ordering using the default search budget. These random-data tests do not assert recall-quality thresholds or complete graph reachability; crafted fixtures retain numerical distance and partial-migration checks.
  • Deletion during accumulation, live-vector threshold boundaries, and pending-job cleanup. Native cleanup tests verify expired index ownership and allocator-only memory baselines for FLOAT32/FLOAT16 with zero and positive training thresholds.
  • Supplied-mean ownership, conversions, invalid inputs, score correctness, and training precedence. Mean-trained FLOAT16 L2 is supported and tested positively.
  • Native concurrent-transition KNN, range, and batch-result validation; rejection of unsupported SQ8 configurations.

Which issues this PR fixes

Implements the VecSim portion of MOD-14959. The ticket remains open for RediSearch integration flows, including FT.CREATE, FT.INFO, ingestion/query/deletion, and RDB round trips. The remaining checklist is in the ticket.

ARM Python-flow CI enablement is separately tracked in MOD-18762.

Main objects this PR modified

  • src/python_bindings/bindings.cpp
  • tests/flow/test_hnsw_tiered.py
  • tests/unit/test_hnsw_sq8.cpp

BF/SVS-specific constructor hardening and its tests are excluded from this PR and preserved separately for follow-up.

Validation

  • Rebased onto main at fcdeb37b as one commit, preserving the reviewed three-file scope. Independent review verified both the original PR changes and new main changes are preserved.
  • Rebuilt on dorer-intel: 115 focused Python cases passed, including SQ8/manual-control and the new update_vectors flows. Three expected NumPy overflow warnings came from invalid-mean cases.
  • Five native tests passed: four pending-migration cleanup cases and the concurrent quantization-transition test.
  • Earlier revisions also passed SQ8 Python coverage under ASan (leak detection disabled) and 25 native concurrent-transition runs. ASan was not rerun after this rebase.
  • Follow-up fa9608bf fixes the pre-training reference: float64 NumPy rankings can differ from native FP16 accumulation and cosine normalization. All 115 focused Python cases passed on dorer-intel after the fix; independent review passed. The AVX-512 FP16 path requires CI confirmation because dorer-intel lacks that CPU feature.
  • CI for the latest head is pending. No ARM Python-flow validation is claimed.

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Note

Medium Risk
New public Python API for quantization and stricter constructor errors affect index creation paths; tiered lifecycle and async migration are heavily exercised but remain complex.

Overview
Exposes SQ8 quantization through the VecSim Python API and adds broad flow/unit coverage for tiered HNSW training, migration, and search.

Bindings: Adds VecSimQuantType, HNSWParams.quantType, and TieredHNSWParams.QuantNormalizationSetSize. HNSW and generic VecSimIndex (HNSW-only) accept an optional keyword-only quantization_mean (1-D float, length = dim, finite); it is validated and wired into native quantParams for SQ8. Unsupported index parameters now raise ValueError instead of constructing a null native index; tiered creation resets the mock thread pool context on failure.

Tiered testing hooks: Tiered_HNSWIndex gains manual_jobs and _run_pending_jobs so tests can step migration deterministically; wait_for_index drains the queue in manual mode.

Tests: New Python flow tests cover SQ8 type/metric/multi matrices, training thresholds, supplied vs trained means, partial migration queries, deletion before training, and invalid configs. C++ adds pending-job destruction cleanup and tighter concurrent-transition checks for KNN, range, and batch iterators.

Reviewed by Cursor Bugbot for commit 589f5fa. Bugbot is set up for automated code reviews on this repo. Configure here.

Base automatically changed from dor-forer-MOD-14957-tiered-hnsw-sq8 to main September 15, 2026 09:00
@dor-forer
dor-forer marked this pull request as ready for review September 17, 2026 12:33
@codecov

codecov Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.55%. Comparing base (fcdeb37) to head (fa9608b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1048   +/-   ##
=======================================
  Coverage   97.55%   97.55%           
=======================================
  Files         142      142           
  Lines        9095     9095           
=======================================
  Hits         8873     8873           
  Misses        222      222           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dor-forer added a commit that referenced this pull request Sep 22, 2026
Covers the default: return nullptr; branch in TieredSVSFactory::NewIndex
so codecov/patch no longer flags it as a missing line on PR #1048.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e062f68. Configure here.

Comment thread src/VecSim/index_factories/svs_factory.cpp Outdated
Comment thread tests/flow/test_bruteforce.py Outdated
Comment thread src/python_bindings/bindings.cpp
Comment thread tests/flow/test_svs.py Outdated
Comment thread tests/flow/test_hnsw_tiered.py Outdated
Comment on lines +470 to +471
def test_manual_tiered_destruction_with_pending_jobs():
code = """

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is this?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Still don't get it... We want to isolate native crashes and hangs in all tests, no? Why does this one have to be a subprocess? looks strange

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok for some reason it didn't post it.
So there's no special reason for a subprocess here just over confident ai again. Removed it and kept the test with jobs still pending when we delete the index.
Also added a native SQ8 test to check that the index is actually released and the tracked memory is freed, instead of just checking that it doesn't crash. Passed for FP32/FP16 with zero and positive training thresholds.

Comment thread tests/flow/test_hnsw_tiered.py Outdated
@dor-forer
dor-forer requested a review from alonre24 September 24, 2026 07:04
Comment thread tests/flow/test_hnsw_tiered.py Outdated
assert len(set(found[0])) == k, found
assert set(found[0]).issubset(valid_labels), found
assert np.isfinite(scores).all(), scores
assert np.all(np.diff(scores[0]) >= 0), scores

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

consider also asserting the sorted order of results

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It was already covered with assert np.all(np.diff(scores[0]) >= 0), but changed the assertion to compare against np.sort so it’s clearer.

Comment thread tests/flow/test_hnsw_tiered.py Outdated
Comment on lines +470 to +471
def test_manual_tiered_destruction_with_pending_jobs():
code = """

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Still don't get it... We want to isolate native crashes and hangs in all tests, no? Why does this one have to be a subprocess? looks strange

@dor-forer
dor-forer force-pushed the dor-forer-MOD-14959-sq8-python-flows branch from eee8e1a to 589f5fa Compare September 24, 2026 11:34
@dor-forer
dor-forer requested a review from alonre24 September 24, 2026 11:36
@dor-forer
dor-forer enabled auto-merge September 25, 2026 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants