Skip to content

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

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

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

Conversation

@dor-forer

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

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 #1062. 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.

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

🤖 Automated first-pass review. A human maintainer still needs to review this before merge.

What I checked: config parsing (present_string/present_u64), Sq8Options validation, how FT.CREATE args are built and num_attrs counted, the FT.INFO read-back, provenance and effective-config recording, the shipped-config knob guard, the opt-in integration test and its ownership of the test server, and the six shipped configs. The core Redis path does what this repo asks for. A config it can't honor fails hard: SQ8 on a non-HNSW index, on INT8, with --skip-vector-index, or a threshold without compression. The schema is then read back from the server instead of trusted, so the PR's own motivating bug (an index labeled SQ8 that is built uncompressed) now fails the run instead of publishing a number. Other good work: the mutation check on the threshold read-back (forcing 0 goes red with differs from requested 4), matched FLOAT32/FLOAT16 baselines, k=10 set to match DBpedia 100K's 10-neighbor ground truth, and the explicit warning against --upload-end-idx with full-corpus ground truth. The new integration test goes through test_port(), so it gets the #292/#300 ownership claim before flush_db. Index names come from idx:<config-name>, so the six configs don't collide (#151-4).

Worth fixing before merge

  1. COMPRESSION/TRAINING_THRESHOLD become silently ignored on every other engine that reads hnsw_config. Before this PR, an unrecognized hnsw_config key landed in HnswConfig::extra. set_declared (effective_config.rs:833-841) then recorded it as ignored_declared_keys, and Qdrant also warned on stderr via hnsw_config_diff() (qdrant.rs:311). With the new typed fields (config.rs diff at HnswConfig), both keys stop appearing in extra for every engine. Only redis.rs reads them, but valkey (valkey.rs:101), dragonfly (dragonfly.rs:104), pgvector (pgvector.rs:163), kividb, mongodb and qdrant all parse the same struct.

    • Concrete failure: copy redis-hnsw-float32-sq8-zero-k10 and change "engine" to "valkey". The run succeeds, reports nothing ignored, and publishes uncompressed-HNSW numbers under an SQ8 name. That is the bug this PR fixes, moved to a neighboring engine.
    • The shipped_config_knob_guard edits ("compression"/"training_threshold" added to the typed-leaf list) and the global DECLARED_KEY_ALLOWLIST entries make it look covered, but nothing checks which engine read the key.
    • Smallest fix I can see: in set_declared, when the engine isn't redis, call note_ignored for a set compression/training_threshold. Or better, have the non-Redis constructors hard-error on them (this repo's ROOT CAUSE: no engine distinguishes "no filter" from "filter that parsed to nothing" — silent unfiltered runs scored against filtered ground truth #219 posture). Add a test that goes red if the note is removed, e.g. a valkey config with COMPRESSION must show up in ignored_declared_keys or fail.
  2. The shipped configs can publish search numbers before the SQ8 backend has finished building, and the harness doesn't catch it. docs/hnsw-sq8-e2e.md:87-90 states it directly: wait_for_indexing checks document indexing, not tiered frontend-to-backend migration, and "its timeout also returns success." This is taxonomy item 3 (a count trusted as readiness) plus a timeout that fails open. The docs push the real check onto the operator: manual _FT.DEBUG VECSIM_INFO for FRONTEND_INDEX.INDEX_SIZE=0 / BACKGROUND_INDEXING=0. Anyone running experiments/configurations/redis-hnsw-sq8-k10.json without the runbook gets recall/QPS measured partly against the flat frontend. That hits hardest on the -trained-k10 configs, where the first 10240 vectors sit below threshold. The PR's table is fine because the author checked by hand. The shipped configs aren't. I'm not certain this has to block merge, because the fail-open timeout predates this PR. But this PR is what makes it reachable. At minimum, file the fail-open timeout as its own issue before merge. Ideally, gate the search phase on backend drain when hnsw_sq8.compression.is_some().

Optional / follow-up (not blockers)

  • CI never sends the new FT.CREATE args: the only test that does is #[ignore]d. The hand-counted num_attrs (redis.rs diff, 10 + 2 + 2) is guarded only on an SQ8-capable box. A unit test on the built command, one per combination of compression/threshold set or unset, would let the default CI run catch a miscount.
  • requires_server_read_back only covers the RESP2 Array shape. The RedisValue::Map arm of field() (RESP3) has no test.
  • record_effective("hnsw_training_threshold", … unwrap_or(10_240)) records the assumed server default when none was declared. verify_ft_info does check it against the server, so a wrong assumption fails loudly. Recording the value actually read back would keep the effective block honest without relying on that.
  • The PR notes the downloaded-dataset shape test rejects DBpedia's conditions: {}. That's worth a tracking issue so it isn't lost: per ROOT CAUSE: no engine distinguishes "no filter" from "filter that parsed to nothing" — silent unfiltered runs scored against filtered ground truth #219, {} is supposed to mean "no filter declared."

Net: careful, well-verified groundwork. The read-back, the typed rejection of null/negative/string values, and the mutation check all meet this repo's bar. Item 1 is small and local, but it's the same silent-wrong-answer class this PR exists to close. Item 2 is mostly a question of whether the shipped configs should be runnable without the runbook. The accept decision, especially on configs targeting a Search build newer than the CI image, is a human maintainer's call.

@dor-forer dor-forer changed the title Add Redis HNSW SQ8 E2E benchmark support and configurations Add Redis HNSW SQ8 support and k10 E2E configurations Sep 29, 2026
@dor-forer

Copy link
Copy Markdown
Collaborator Author

Superseded by #324: same commit (ce4ec9a), opened from a branch in this repository now that I have write access.

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