Skip to content

Add dedicated bulk embeddings pool for ingest - #2112

Open
JSv4 wants to merge 3 commits into
mainfrom
claude/implement-this-tukzlg
Open

Add dedicated bulk embeddings pool for ingest#2112
JSv4 wants to merge 3 commits into
mainfrom
claude/implement-this-tukzlg

Conversation

@JSv4

@JSv4 JSv4 commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Ingest embedding tasks and search queries currently share a single embeddings
microservice URL (EMBEDDINGS_MICROSERVICE_URL). That forces a compromise: a
pool sized for latency-sensitive search queries (needs to stay warm) also
absorbs the load of batch ingest (thousands of embeddings, happy to hit an
autoscaled / scale-to-zero pool), and vice versa.

This PR lets operators point ingest at a separate bulk pool while query call
sites stay on the always-warm pod, fully isolating search latency from ingest
load — with no change to the embedder client, base class, or any search
resolver
.

Changes

  1. New settingEMBEDDINGS_MICROSERVICE_URL_BULK in
    config/settings/base.py (next to EMBEDDINGS_MICROSERVICE_URL), defaulting
    to EMBEDDINGS_MICROSERVICE_URL so single-pool deployments need no config.
    Query call sites are untouched and stay warm automatically.

  2. Ingest routing — the Celery tasks in
    opencontractserver/tasks/embeddings_task.py resolve the bulk URL via a new
    _bulk_embeddings_service_url() helper and thread it through an optional
    service_url_override parameter added to the four ingest leaves —
    _create_text_embedding, _create_embedding_for_annotation,
    _batch_embed_text_annotations, _apply_dual_embedding_strategy — plus
    _embed_relationship. The override is translated into the embedder's
    existing call-time embeddings_microservice_url kwarg (read by
    MicroserviceEmbedder._get_service_config), so no embedder/base-class change
    is required. Embedders that don't read that kwarg (hosted providers, the
    multimodal image pool) simply ignore it.

  3. Backwards compatible by construction — when the override is None
    (the default, and any direct/legacy caller), no URL kwarg is passed and
    behavior is byte-identical to before. The dual-embedding strategy only
    forwards the keyword to embed_func when a bulk URL is set, so the original
    three-argument embed_func contract keeps working.

  4. Multimodal image pool left alone — the bulk setting is text-only; image
    embedding keeps its own CLIP_EMBEDDER_URL / QWEN_EMBEDDER_URL pool.

Deployment note

Rebuild/redeploy the Django image so Celery workers pick up the new code and
setting. Set EMBEDDINGS_MICROSERVICE_URL_BULK in the environment to point
ingest at the bulk pool; leaving it unset keeps the current single-pool
behavior.

Tests

opencontractserver/tests/test_batch_embedding.py gains regression tests
verifying:

  • _batch_embed_text_annotations forwards service_url_override as
    embeddings_microservice_url, and passes no URL kwarg when unset.
  • The full calculate_embeddings_for_annotation_batch task reads
    EMBEDDINGS_MICROSERVICE_URL_BULK (via override_settings) and routes ingest
    to it end-to-end.

Docs / changelog

  • docs/deployment/performance_tuning.md — new "Separate bulk embeddings pool
    for ingest" section.
  • Sample env files updated with the new (commented, optional) variable.
  • Changelog fragment changelog.d/bulk-embeddings-pool.added.md.

Generated by Claude Code

Ingest embedding tasks and search queries currently share one embeddings
microservice URL, so a scale-to-zero / cold-starting pool sized for batch
ingest also serves latency-sensitive search queries (and vice versa).

Add EMBEDDINGS_MICROSERVICE_URL_BULK (config/settings/base.py, defaults to
EMBEDDINGS_MICROSERVICE_URL) and route only the ingest Celery tasks in
opencontractserver/tasks/embeddings_task.py through it, via a new optional
service_url_override parameter threaded into the four ingest leaves
(_create_text_embedding, _create_embedding_for_annotation,
_batch_embed_text_annotations, _apply_dual_embedding_strategy) plus
_embed_relationship. The override reuses the embedder's existing call-time
'embeddings_microservice_url' kwarg, so the embedder client, base class, and
all search resolvers are untouched. When the setting is absent the override is
None and no kwarg is passed, leaving default behavior unchanged.

Query call sites keep reading EMBEDDINGS_MICROSERVICE_URL (the always-warm
pod), fully isolating search latency from batch ingest load. The multimodal
image pool intentionally stays on its own URL (text-only bulk setting).

Adds regression tests covering the override threading at both the helper and
task-entry-point level.
Comment thread opencontractserver/tasks/embeddings_task.py Fixed
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review

Went through the diff (config/settings/base.py, opencontractserver/tasks/embeddings_task.py, tests, docs). Overall this is a clean, well-scoped change — the mechanism (reusing MicroserviceEmbedder's existing call-time embeddings_microservice_url kwarg instead of touching the embedder base class) is a good choice, and I verified it end-to-end:

  • BaseEmbedder.embed_text/embed_texts_batch accept **direct_kwargs and merge them over get_component_settings(), and every _embed_text_impl/embed_texts_batch override (sent_transformer_microservice.py, openai_embedder.py, multimodal_microservice.py) uses a **all_kwargs catch-all, so passing embeddings_microservice_url to hosted/multimodal embedders is a genuine no-op, not just an assumption in the PR description.
  • The multimodal image pool (_get_service_config in multimodal_microservice.py) keys off clip_embedder_url, so it's confirmed unaffected by this change.
  • Query call sites are untouched — confirmed no other reference to EMBEDDINGS_MICROSERVICE_URL/_BULK outside embeddings_task.py and the embedder settings dataclasses.
  • Threading of service_url_override through _apply_dual_embedding_strategyembed_func → the four ingest leaves is consistent and the _EmbedFunc Protocol is a nice touch for keeping functools.partial(_embed_relationship, ...) and the closures (doc_embed_func, note_embed_func) type-checkable.
  • Tests follow the existing patterns in test_batch_embedding.py closely (mock structure, _make_result, RecordingEmbedder pattern) and cover both the helper-level and task-entry-point-level threading.

One thing worth double-checking (not blocking)

EMBEDDINGS_MICROSERVICE_URL_BULK in config/settings/base.py:1180-1182 defaults to EMBEDDINGS_MICROSERVICE_URL's value, not to None/unset:

EMBEDDINGS_MICROSERVICE_URL_BULK = env(
    "EMBEDDINGS_MICROSERVICE_URL_BULK", default=EMBEDDINGS_MICROSERVICE_URL
)

So _bulk_embeddings_service_url() (embeddings_task.py:53-62) will almost never actually return None in a real deployment — it always resolves to a non-empty string (equal to EMBEDDINGS_MICROSERVICE_URL when the bulk var is unset). That means service_url_override is truthy on effectively every ingest call, and _service_url_override_kwargs() always injects the embeddings_microservice_url kwarg — the "no kwarg passed / behavior is byte-identical" framing in the docstrings and PR description is true in effect (same URL value in, same URL value out) but the "kwarg omitted" code path it describes isn't actually exercised in single-pool deployments. Worth a doc tweak, or just confirming that's the intended framing.

A related, more speculative point: because the override is unconditionally forwarded to both the default-embedder pass and the corpus-specific-embedder pass in _apply_dual_embedding_strategy, if a corpus's preferred_embedder is ever a MicroserviceEmbedder-family class pointed at a different dedicated URL than the global default (there's only one such class today, so this isn't live), this mechanism would silently redirect that corpus's ingest traffic to the bulk/default URL instead of its own pool. Not a bug against the current codebase (only one MicroserviceEmbedder subclass exists), but worth keeping in mind if a second microservice-embedder variant is added later — might be worth a comment near _apply_dual_embedding_strategy's embed_extra forwarding calling this out.

Minor nits

  • from django.test import override_settings is imported inline inside test_bulk_url_from_settings_threaded_to_embedder (test_batch_embedding.py:944) rather than at module top with the other imports — harmless, just inconsistent with the rest of the file's import style.
  • Coverage is solid for the batch-annotation path (_batch_embed_text_annotations, calculate_embeddings_for_annotation_batch) but there's no direct test asserting the override reaches the embedder for calculate_embedding_for_doc_text, calculate_embedding_for_note_text, or calculate_embeddings_for_relationship_batch. Since they all funnel through the same _apply_dual_embedding_strategy/_service_url_override_kwargs machinery that is tested, this is low risk, but a quick test for one of the other entry points would close the loop.

Changelog fragment, settings comment, and sample env files all look correct and follow the repo's conventions (changelog.d/<slug>.<type>.md, no CHANGELOG.md edit). Nothing here blocks merging.

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

claude added 2 commits July 5, 2026 07:22
CI on the initial commit failed two checks:

- linter (mypy): the new _EmbedFunc Protocol required embed_func to accept a
  service_url_override keyword, which the pre-existing 3-arg embed_func closures
  in test_embeddings_task.py do not, producing 5 arg-type errors. Replace the
  Protocol with Callable[..., bool], which accepts both the legacy 3-arg
  callables and the optional 4th-kwarg call the dual strategy makes when a bulk
  URL is set. Also removes the ellipsis-default the code-quality bot flagged.

- codecov/patch: the document, note, and relationship ingest entry points had
  no direct test exercising the override, leaving added lines uncovered. Add
  TestBulkEmbeddingsPoolRouting asserting the bulk URL threads through to the
  embedder for calculate_embedding_for_doc_text, calculate_embedding_for_note_text,
  and calculate_embeddings_for_relationship_batch, plus a no-bulk negative case.

Also make EMBEDDINGS_MICROSERVICE_URL_BULK opt-in (default None instead of
EMBEDDINGS_MICROSERVICE_URL): when unset, ingest stays on the embedder's
configured URL and no override kwarg is threaded at all, so the 'byte-identical
default behavior' claim is literally exercised and a corpus's differently-pooled
embedder is never redirected unless bulk is explicitly configured. Documents the
forwarding scope near _apply_dual_embedding_strategy. Docs, sample env files, and
changelog updated to match. Moves an inline override_settings import to module
scope.
The pytest CI check failed on one pre-existing test:
TestCalculateEmbeddingsForRelationshipBatch::test_explicit_embedder_counts_outcomes.
It patches _embed_relationship with a strict 3-arg side_effect
(rel, embedder, embedder_path); threading service_url_override=... into that
call raised TypeError, which the loop caught, miscounting every relationship as
failed.

Fix: in the explicit relationship path, forward service_url_override only when a
bulk URL is configured (the same conditional-forwarding pattern already used for
embed_extra in _apply_dual_embedding_strategy). When unset, _embed_relationship
is called with its original three positional args, so the mocked side_effect
matches and the test passes untouched.

Also add direct unit tests for _service_url_override_kwargs (both branches) and
_bulk_embeddings_service_url (set / unset) so the pure helpers have reliable
coverage attribution independent of the task-level mocks.
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review: Dedicated bulk embeddings pool for ingest

Overall this is a clean, well-scoped change. The mechanism (reusing the embedder's existing call-time embeddings_microservice_url kwarg instead of touching the embedder client/base class) is a nice minimal-surface-area design, and the opt-in default (None, byte-identical behavior when unset) is the right call for a production routing change like this.

What I checked

  • config/settings/base.py: EMBEDDINGS_MICROSERVICE_URL_BULK defaults to None, consistent with other optional settings in the file (AWS_S3_REGION_NAME, GS_PROJECT_ID, etc.).
  • opencontractserver/tasks/embeddings_task.py: traced service_url_override through all five ingest leaves (_create_text_embedding, _create_embedding_for_annotation, _apply_dual_embedding_strategy, _batch_embed_text_annotations, _embed_relationship) and confirmed the conditional-forwarding pattern ({"service_url_override": ...} if bulk_service_url else {}) is applied consistently everywhere a legacy 3-arg embed_func/_embed_relationship contract needs to keep working.
  • Confirmed MicroserviceEmbedder._get_service_config (sent_transformer_microservice.py) reads embeddings_microservice_url from call kwargs, while the multimodal embedders (CLIPMicroserviceEmbedder, QwenMicroserviceEmbedder) read distinct kwarg names (clip_embedder_url/qwen_embedder_url), so the claim that the multimodal image pool is unaffected holds — and in the one place it matters (_create_embedding_for_annotation's multimodal branch calling generate_multimodal_embedding), the override isn't even forwarded, so there's no reliance on the kwarg being silently ignored.
  • Walked the 3-commit history: the CI-driven fixes (mypy Callable[..., bool] typing, the test_explicit_embedder_counts_outcomes regression from unconditionally forwarding service_url_override into a strict 3-arg mock, and switching the setting from defaulting-to-EMBEDDINGS_MICROSERVICE_URL to opt-in None) are all correctly reflected in the final diff, including the docs/sample-env comment updates that went with the opt-in change.
  • Test coverage is thorough: helper-level unit tests (_service_url_override_kwargs, _bulk_embeddings_service_url), per-task-entry-point routing tests for doc/note/annotation/relationship paths, and an explicit no-bulk-set negative case verifying no kwarg is threaded.

Minor notes (non-blocking)

  1. _apply_dual_embedding_strategy forwards the bulk override to both the default-embedder pass and the corpus-preferred-embedder pass (embeddings_task.py:299-301). If a corpus's preferred_embedder is ever a second MicroserviceEmbedder-family class pointed at its own dedicated pool (distinct from the default bulk pool), this would silently redirect that corpus's ingest to the default bulk URL instead. This is already called out in an inline comment as a known/accepted tradeoff given today's single microservice-embedder class, so no action needed — just flagging that it's the one place this PR takes on a little coupling between bulk and default pools that's worth remembering if a second microservice embedder variant is ever added.

  2. Empty-string edge case: _bulk_embeddings_service_url() and _service_url_override_kwargs() both treat an empty string the same as unset (falsy check). If an operator sets EMBEDDINGS_MICROSERVICE_URL_BULK= (empty) in the environment, ingest silently falls back to the default pool rather than surfacing a config error. Low risk in practice (env files ship the var commented out, not empty), but worth a one-line doc note if you want to guard against copy-paste misconfiguration later.

  3. config/settings/base.py:1183 sits right at 88 chars — within black's default limit, but worth double-checking pre-commit run --all-files stays green if the setting name/URL ever grows.

Nice test discipline overall — particularly appreciated that the regression in test_explicit_embedder_counts_outcomes was root-caused (strict-arity mock vs. unconditional kwarg forwarding) rather than just loosened, and that the fix was generalized into the same conditional-forwarding pattern already used elsewhere in the file.

@JSv4

JSv4 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

This introduces separate configuration pathway besides our singletons... take another pass

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