Skip to content

indexer: order-preserving concurrent embedding requests (#107) - #117

Merged
IceRhymers merged 1 commit into
integration/indexer-performancefrom
feat/107-concurrent-embeddings
Jul 25, 2026
Merged

indexer: order-preserving concurrent embedding requests (#107)#117
IceRhymers merged 1 commit into
integration/indexer-performancefrom
feat/107-concurrent-embeddings

Conversation

@IceRhymers

Copy link
Copy Markdown
Owner

Summary

Implements issue #107: the indexer's app/embed.py:databricks_embedder.embed() now dispatches batches concurrently instead of one HTTP call at a time, while guaranteeing vectors return in submission order regardless of completion order — the load-bearing property, since indexer/job.py:_precompute_chunk_writer re-slices the flat result positionally and a silent reorder would attach the wrong file's embeddings with no error.

Executed from the critic-approved plan at .omc/plans/issue-107/approved-plan.md (APPROVE after 4 review rounds + a base-revalidation pass against 732a7d7).

Correction to a stale figure carried in epic #110 / the issue body: the "30k-chunk cap" / "~470 sequential requests" language there is stale — the real cap is semantic_max_chunks_per_repo = 8000 (app/config.py), i.e. ~125 batches of 64 at the default batch size. The direction of the argument is unaffected.

What changed

  • app/embed.pydatabricks_embedder gains concurrency: int = 1. When workers = max(1, min(concurrency, len(batches))) > 1, batches dispatch via ThreadPoolExecutor(max_workers=workers).map(...)never as_completed, which yields in completion order and would defeat the whole point. concurrency <= 1, or a text list short enough to produce one batch (e.g. every query-time call), constructs no pool and spawns no thread — a strict no-op on that path. concurrency deliberately does not mirror this file's usual default-mirroring convention (every other param matches its Settings twin): the concurrent path is opt-in only at the call site that knows it's the indexer (get_embedder).
    _query_batch gains keyword-only ordinal/offset so a count mismatch names the offending batch (AC2), e.g. embedder returned 63 vectors for 64 texts (batch 3, texts[192:256]). Both existing message-matching tests pass unmodified (match= is re.search, substring preserved).
  • 429 posture — unchanged, on purpose. The installed databricks-sdk (0.122.0) already absorbs AI Gateway 429/503s via Retry-After-honoring backoff (_RetryAfterCustomizer, defaulting to 1s without the header) before _query_batch's own bounded retry ever sees them. No third retry layer added; _query_batch's retry shape and max_retries default are untouched. The one gap this creates — that absorption is invisible at the job's INFO log level (the SDK logs at DEBUG) — is documented in the runbook as a single-named-logger operator step (databricks.sdk.retries), never a code change (two existing tripwires in test_job_redaction.py guard against exactly that landing in indexer/*.py).
  • app/config.pysemantic_embedding_concurrency: int = 4. Total in-flight gateway requests is effective_workers x concurrency: 2 x 4 = 8 at the default, 2 x 8 = 16 at the config.yaml le=8 ceiling, both under the SDK's 20-connection pool (pool_block=True, so exceeding it silently serializes rather than erroring — staying under 20 is load-bearing). The Settings field itself is intentionally unbounded, mirroring its semantic_embedding_batch_size sibling — the le=8 bound lives on the config.yaml overlay, with databricks_embedder's own clamp as the real safety net.
  • indexer/repo_config.pySemanticOverrides.embedding_concurrency: int | None = Field(ge=1, le=8), wired into settings_overrides(), class docstring updated.
  • config.yaml, docs/runbooks/semantic-enablement.md (§4, §6), docs/runbooks/indexing-parallelism.md (§3), app/AGENTS.md, indexer/AGENTS.md — documentation updated with the in-flight/memory arithmetic, the rollback switch (concurrency: 1), and the 429/DEBUG-logger posture.
  • scripts/measure_embedding_concurrency.py (new) — offline harness (fake client, fixed sleep, no network, not run in CI) measuring dispatch scaling. Output below.
  • Not touched: indexer/job.py (zero changes — still calls embed_fn(all_texts) once and re-slices positionally), indexer/timing.py (no context propagation into embed threads — deliberately rejected after prototyping copy_context(); see the plan's round-2 finding 3 — PhaseTimer is documented not-thread-safe and its cross-thread-isolation docstring, which names indexer: concurrent embedding requests, order-preserving #107, stays true), INDEX_SEMANTICS_VERSION (this changes dispatch mechanics, not extraction output; app/embed.py/app/config.py are deliberately outside the semantics-version tripwire's watched paths).

Tests

24 cases in tests/unit/test_embed.py (13 planned + defensive additions from an independent code-review pass — see below) plus 3 sites in tests/unit/test_repo_config.py. Highlights:

  • Submission-order proof (test_concurrent_batches_return_in_submission_order) with a non-vacuity companion proving the fixture's completion order genuinely differs from submission order.
  • AST-based (not substring) tripwire that app.embed never references as_completed, including via an aliased import.
  • A structural, not scheduler-derived, bound on queued-batch cancellation after a failure (test_failure_cancels_queued_batches) — every non-failing batch parks on an Event that is never set, so started is fixed by the code path, not by timing. Verified stable across 40+ repeated runs, including under 4x CPU oversubscription.
  • Deterministic lowest-ordinal-failure reporting under concurrent, out-of-order completion.
  • The concurrency clamp's degenerate paths (concurrency <= 0, 0 batches, 1 batch) all route to the serial no-pool path.
  • A defensive pin on the default itself (concurrency defaults to 1) and a non-degenerate-batch_size test for the offset arithmetic — both added after an independent code-reviewer pass found the original fixtures could pass with either mutated.

Independent code review

Ran a fresh code-reviewer agent in a separate context. Verdict: no CRITICAL/HIGH. Found 2 MEDIUM (both test-coverage gaps, both mutation-proven — fixed: a direct assertion pinning the default to 1, and a non-degenerate-batch_size test for the offset math) and several LOW (fixed: closed an aliased-import gap in the AST tripwire; clarified runbook wording that the le=8 bound is config.yaml-only, not the env surface; documented the bounded failure-path latency increase under concurrency). Two LOW suggestions were not applied because they'd contradict explicit, already-adversarially-reviewed design decisions in the approved plan: making _query_batch's ordinal/offset required (the plan explicitly keeps them defaulted so the function stays callable in isolation) and bounding the Settings field itself (deliberately unbounded, mirroring its batch_size/timeout_s siblings, with the real clamp inside databricks_embedder).

Measurement (AC3)

Part 1 — scaling shape (committed, deterministic):

32 batches x 0.05s fake latency each
concurrency  wall_clock_s   speedup
          1         1.609     1.00x
          2         0.806     2.00x
          4         0.403     3.99x
          8         0.202     7.96x

Parts 2 & 3 — real-repo embed= timing comparison and the databricks.sdk.retries DEBUG-logger 429 check require a live dev workspace. Not run in this environment — no workspace access available. Deferred with this named reason, per the plan's honesty requirement; the scaling-shape result plus the offered-load ceiling (§ above) is the evidence available at this time.

Gates

  • make lint (ruff check + format + mypy over app indexer webui): clean.
  • make test (pytest -m "unit or observability"): 1238 passed, 1 failed. The one failure — tests/unit/test_semantics_version_tripwire.py::test_semantics_change_bumps_the_index_semantics_version — is an inherited failure, present on the unmodified base branch (origin/integration/indexer-performance @ 732a7d7) too: an earlier merged PR (indexer: single-pass in-memory tarball ingestion (drop extract-to-disk) #106) added indexer/ingest.py without a version bump. Verified identical on both branches; not this PR's to fix, and the tripwire's own docstring forbids silencing it. No other test fails.
  • make test-integration (pytest -m "integration or e2e", against the local codesearch-pg container): 8 failed / 205 passed / 43 errors, identical count with and without this diff. Cause: the local Postgres container lacks the lakebase_tokenizer/lakebase_vector extensions real Lakebase provides — a pre-existing environment gap, not related to this change. No integration test touches app/embed.py.

Closure ownership

Refs #107 (not Closes) — this is an arc child of epic #110; #107 closes when the umbrella PR #111 merges to master (or is closed manually alongside its siblings), not automatically from this PR.

Test plan

  • make lint clean
  • make test — green except the one named inherited failure
  • make test-integration — run locally; failure signature identical to the unmodified base
  • Independent code-reviewer pass; all actionable findings fixed
  • Measurement script committed and run; output captured above
  • Real-repo embed= timing comparison and DEBUG-logger 429 check (parts 2–3 of AC3) — deferred, no live workspace available in this environment

Batches now dispatch through a ThreadPoolExecutor.map (never as_completed,
which yields in completion order), so vectors always return in submission
order regardless of which request finishes first -- the whole point of the
issue, since _precompute_chunk_writer re-slices the flat result positionally
and a reorder would silently corrupt embeddings for the wrong file.

- app/embed.py: databricks_embedder gains concurrency (default 1, a
  deliberate divergence from this file's default-mirroring convention so the
  concurrent path is opt-in only at the indexer call site); _query_batch
  gains ordinal/offset so a count mismatch names the offending batch
  (AC2). Existing retry semantics and the SDK's own 429/Retry-After
  handling are unchanged -- no third retry layer added.
- app/config.py: semantic_embedding_concurrency = 4 (workers x concurrency
  = 8 in-flight by default, 16 at the config.yaml le=8 ceiling, both under
  the SDK's 20-connection pool).
- indexer/repo_config.py: SemanticOverrides.embedding_concurrency (1..8)
  overlays the setting from config.yaml, the job's only reachable config
  surface.
- scripts/measure_embedding_concurrency.py: offline scaling harness (no
  network); measured ~1x/2x/4x/8x at concurrency 1/2/4/8.
- Docs: config.yaml, semantic-enablement.md, indexing-parallelism.md,
  app/AGENTS.md, indexer/AGENTS.md.
- Tests: 24 cases in test_embed.py (submission-order proof plus a
  non-vacuity companion, AST-based as_completed tripwire, a structural
  queued-batch-cancellation bound, deterministic lowest-ordinal-failure
  reporting, the concurrency clamp's degenerate paths) and 3 sites in
  test_repo_config.py for the new config field.

indexer/job.py is unchanged: it still calls embed_fn(all_texts) once and
re-slices positionally. No INDEX_SEMANTICS_VERSION bump -- this changes
dispatch mechanics, not extraction output.

Refs #107
@IceRhymers
IceRhymers force-pushed the feat/107-concurrent-embeddings branch from 6765e7c to 045bdaf Compare July 25, 2026 18:18
@IceRhymers
IceRhymers merged commit 5f290c6 into integration/indexer-performance Jul 25, 2026
4 checks passed
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