Skip to content

Add Redis HNSW SQ8 support and k10 E2E configurations - #324

Closed
dor-forer wants to merge 2 commits into
masterfrom
dor-forer-MOD-14960-hnsw-sq8-e2e
Closed

dor-forer wants to merge 2 commits into
masterfrom
dor-forer-MOD-14960-hnsw-sq8-e2e

Conversation

@dor-forer

Copy link
Copy Markdown
Collaborator

The Rust Redis adapter currently ignores HNSW compression and training settings, so a configuration labeled SQ8 can create an uncompressed index. This adds explicit COMPRESSION SQ8 and TRAINING_THRESHOLD handling, validates the options, verifies the created schema through FT.INFO, and records declared and effective settings in result metadata.

Adds matched FLOAT32/FLOAT16 configurations for uncompressed HNSW, SQ8 threshold 0, and learned-mean SQ8 threshold 10240, with explicit k=10 for the initial DBpedia 100K workload. A dedicated integration target tests SQ8-capable servers.

E2E groundwork for MOD-14960, separate from VectorSimilarity microbenchmarks #1065. Runbook.

Initial Arm results

Measured with harness commit 5aa6253658274ad6322037b76c7e70b1adeedef3. The focused revision retains the same runtime SQ8 implementation and k=10 configuration; it removes the unused k=100 configuration and an unrelated test-only filter fix.

Full DBpedia 100K corpus, dimension 1536, cosine; first 100 of 5000 queries, k=10, one client, one repetition. HNSW M=32, efConstruction=200, efRuntime=400. Arm Neoverse-V2, 8 cores, four Search workers. The dataset supplies only ten ground-truth neighbors per query, so these runs use the k=10 configuration.

Input Index Vector index MiB Total Redis used memory MiB Recall@10
FLOAT32 Uncompressed 629.96 1461.88 99.8%
FLOAT32 SQ8, threshold 0 190.12 949.55 96.6%
FLOAT32 SQ8, threshold 10240 190.58 949.75 99.0%
FLOAT16 Uncompressed 336.01 777.13 97.9%
FLOAT16 SQ8, threshold 0 190.12 607.76 97.1%
FLOAT16 SQ8, threshold 10240 190.58 607.95 99.2%

These are short, unreplicated functional checks, not throughput evidence. Independent graph construction and input conversion affect recall. Every case verified the requested schema, full document/backend counts, empty vector frontend, no background indexing and no indexing errors before search. Only one configuration was resident at a time. Total memory includes the stored HASH vectors as well as the vector index.

Server: Redis 8.10.1, Search reported commit 8d1dae1. Search binary SHA256: 1dee9895192d2331db447e744f0483d71fbac24e3085db3952af5250159fc02c. Dataset vectors SHA256: 1836616fc92894a899798e1998f698f2d4d39b8e13bb162c95e9f96a933e5dfb; queries/ground truth: e04882b681de2650a79e05c5302935e01a81fed2438da54a8c0b0a70539448b6.

Validation

On dorer-arm:

  • Focused revision: make agent-check passed in a clean checkout (formatting, Clippy, release tests and harness invariants).
  • Focused revision, general Redis integration: 44 passed, one explicitly ignored SQ8 test.
  • Focused revision, REDIS_TEST_PORT=14960 make integration-test-redis-sq8: passed against the feature-capable server.
  • Original implementation mutation check: forcing emitted threshold 0 made the learned-threshold test fail with differs from requested 4; restored code passed again.
  • Original six full-corpus smoke cases passed; independent artifact review completed.

The existing GitHub CI runs code checks and general integration tests. It does not run the Arm performance matrix or produce the table above. The SQ8-specific integration test is opt-in because the default CI Redis image may predate HNSW SQ8 support.

The existing downloaded-dataset shape test rejects DBpedia empty filters (conditions: {}). That independent test fix is excluded from this PR. Clean-checkout validation does not cover the downloaded DBpedia shape scan.

Remaining benchmark work

This draft does not complete MOD-14960: k=100 configurations for datasets with sufficiently wide ground truth, explicit accumulation/training-transition measurements, the 64/128/768/1536 dimension matrix at agreed sizes, sustained repeated throughput/latency runs with A/A controls, and calibrated regression thresholds remain outstanding. The registered 100K and 1M DBpedia families use different embedding models and must not be presented as a same-model scaling comparison.

🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

What I checked: the flow from the hnsw_config knob through to FT.CREATE (config.rs typed fields, Sq8Options::from_hnsw_fields, the num_attrs arithmetic in redis.rs), the FT.INFO read-back parser, what gets recorded as declared and effective, per-config index naming (#151-4), ownership of the destructive integration test (#292/#300), and whether the new knobs could be silently dropped anywhere else. The core path is sound. SQ8 on a non-HNSW index, on a non-FLOAT32/16 type, or with --skip-vector-index is a hard error. TRAINING_THRESHOLD without COMPRESSION is a hard error. Explicit null is rejected and a declared 0 is kept distinct from "absent". After FT.CREATE, the server read-back fails the run when the index doesn't match what was asked for. That last check is exactly the right shape for this repo's main bug class: before this PR, a config labelled SQ8 silently built an uncompressed index and still reported numbers. The six configs get distinct idx:<config-name> names through derive_index_name, so there's no #151-4 collision. The PR body is also unusually well-evidenced: pinned server/module/dataset SHA256s, a mutation check on the emitted threshold, and the table labelled as unreplicated smoke rather than throughput evidence.

Worth fixing before merge

  1. --skip-upload publishes "SQ8" without ever checking the server. record_effective("hnsw_compression", …) runs unconditionally in RedisEngine::new() (redis.rs, diff ~L463–478). verify_ft_info only runs inside create_index() (diff ~L525–530). run_single_experiment skips configure() entirely under --skip-upload (experiment.rs:1200). The runbook's own search step (docs/hnsw-sq8-e2e.md, second command) is --skip-upload. So the documented workflow searches whatever index is sitting at idx:<config-name> and records hnsw_compression: "SQ8" as effective. The index name is derived only from the config name, so one realistic trigger is an index uploaded by a pre-PR binary, which by this PR's own description silently dropped COMPRESSION. That run reports uncompressed recall and QPS as SQ8. Suggested fix: run verify_ft_info on the reuse path too, next to the existing corpus-reuse guard. Alternatively, record the effective values only after a successful read-back, so an unverified run publishes nothing rather than the declared value.

  2. The new typed fields hide the knobs from the ignored-key report for every other HnswConfig consumer. HnswConfig is shared: per the doc comment at config.rs:54–58 it is read by qdrant, valkey, pgvector, dragonfly and kividb as well as redis. Before this PR, COMPRESSION/TRAINING_THRESHOLD fell into the extra catch-all. That meant Qdrant's unsupported_keys() warned about them and ignored_declared_keys ("hnsw_config keys serde could not type", effective_config.rs:278) listed them. Now they deserialize into typed fields that only redis.rs reads. A user config like {"engine": "valkey", "collection_params": {"hnsw_config": {"COMPRESSION": "SQ8"}}} builds an uncompressed index, publishes COMPRESSION: "SQ8" under declared, and reports ignored_declared_keys: []. The new test redis_sq8_declared_values_are_preserved_and_not_ignored pins that same empty list for redis, where it's correct. Nothing pins the other engines. shipped_config_knob_guard only covers shipped JSON, so it won't catch a user-supplied file. Suggested fix: have the non-Redis consumers hard-error when compression/training_threshold is Some, the same way ROOT CAUSE: no engine distinguishes "no filter" from "filter that parsed to nothing" — silent unfiltered runs scored against filtered ground truth #219 handles unbuildable filters. At minimum, add them to that engine's ignored report. A per-engine test that goes red if either field is dropped would close it.

Optional / follow-up (not blockers)

  • The default 10_240 is hard-coded in two places: effective recording in redis.rs (diff ~L476) and verify_ft_info in redis_hnsw_sq8.rs (diff ~L639). The 102_400 upper bound is a client-side copy of a server limit. A shared const would keep the two defaults from drifting. The drift would fail loudly at read-back rather than silently, so this is cosmetic.
  • test_binary_redis_hnsw_sq8_reaches_server checks the server through Sq8Options::verify_ft_info, which is the same parser production uses. A parser bug (for example, matching the wrong attribute or mis-chunking a RESP2 array) would pass in both places. Asserting the raw compression/training_threshold values from the FT.INFO reply once, independently, would make it a real oracle. Also, the test doesn't assert anything about the threshold-4 index having actually trained or migrated. It checks the declared threshold and num_docs == 32, not backend state. If "crosses the learned threshold" is meant as a tested claim, _FT.DEBUG VECSIM_INFO backend size would show it.
  • redis_utils.rs already has RESP2/RESP3 FT.INFO field walking (as_chunks::<2> at L396/L460). The new field/as_string helpers duplicate it. That's fine if the goal is to keep this in src/ for unit-testability, but worth a second look from whoever owns the Redis engine.
  • The runbook says the existing wait_for_indexing checks document indexing, not tiered backend migration, and that "its timeout also returns success". If that holds, it's a taxonomy-Add 1M,10M,20M,40M,100M,200M laion datasets #3 bug that already applies to every Redis HNSW run, not just SQ8. It deserves its own issue rather than living only as a manual step in a doc. The same goes for the DBpedia conditions: {} shape-test failure the PR body mentions but excludes.
  • CI never exercises the SQ8 path, because the target is #[ignore] and opt-in. That's reasonable given the default image, and the PR says so plainly. It's worth noting that everything past the unit tests rests on the author's Arm host run.

Net: a careful, well-scoped fix for a real problem where the harness ran and reported the wrong thing, and the read-back is the right design. The two "before merge" items are both cases where that guarantee doesn't reach far enough: the reuse path, and the other engines that share HnswConfig. Both are small and localized. The accept decision is the maintainer's call.

@dor-forer

Copy link
Copy Markdown
Collaborator Author

CI note: every check passes except Integration tests (Milvus), which fails at the "Start Milvus" step before any test runs, on both the first run and a re-run. This PR does not touch the Milvus engine, and the same job failed on master CI on 2026-09-24, so it looks like a container-startup problem in the workflow rather than a regression here.

Follow-up PR stacked on this branch: #325 (env overrides, k=100 calibration matrix, config generator).

@github-actions

Copy link
Copy Markdown

Docker Build Validation

Platforms tested:

  • linux/amd64 (built and tested)
  • linux/arm64 (build validated)

Tests performed:

  • --help command
  • --describe datasets command
  • Integration test (random-100 dataset + Redis)
  • Multi-platform build validation (amd64 + arm64)

@dor-forer

Copy link
Copy Markdown
Collaborator Author

Folded into #325, which now targets master and contains this commit (ce4ec9a) plus the SQ8 coverage work. Closing to keep a single PR to review.

@dor-forer dor-forer closed this Sep 30, 2026
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.

1 participant