Skip to content

Add bulk embeddings pool for ingest (via the pipeline settings singleton) - #2282

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

Add bulk embeddings pool for ingest (via the pipeline settings singleton)#2282
JSv4 wants to merge 2 commits into
mainfrom
claude/implement-this-tukzlg

Conversation

@JSv4

@JSv4 JSv4 commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

Search queries and batch ingest share a single embeddings microservice URL
(EMBEDDINGS_MICROSERVICE_URL), forcing a compromise: a pool sized to keep
latency-sensitive search queries warm also absorbs the load of batch
ingest
(thousands of embeddings, happy to hit an autoscaled / scale-to-zero
pool), and vice versa.

This lets operators point ingest at a separate bulk pool while query call
sites stay on the always-warm pod — routed entirely through the existing
PipelineSettings singleton
, not a separate configuration pathway.

This supersedes #2112, which was closed because it read a standalone Django
setting directly in the ingest task (a config pathway parallel to the
component settings singletons). This version configures the bulk URL the same
way every other embedder setting is configured.

Changes

  • Embedder settings singletonMicroserviceEmbedder.Settings gains an
    optional embeddings_microservice_url_bulk field
    (opencontractserver/pipeline/embedders/sent_transformer_microservice.py),
    seeded from the EMBEDDINGS_MICROSERVICE_URL_BULK env var via
    migrate_pipeline_settings — the same PipelineSettings pathway as the
    existing embeddings_microservice_url.
  • URL selection_get_service_config routes calls tagged
    use_bulk_pool=True to the bulk URL when one is configured, and falls back to
    embeddings_microservice_url otherwise (and for every untagged search query).
  • Ingest tags intent, not URLs — the Celery tasks in
    opencontractserver/tasks/embeddings_task.py tag their three text embed calls
    (_create_text_embedding, _batch_embed_text_annotations,
    _embed_relationship) with use_bulk_pool=True. Because ingest is inherently
    bulk, no override parameter is threaded through the task helpers — their
    signatures are unchanged, and no query call site changes.
  • Env bindingconfig/settings/base.py adds
    EMBEDDINGS_MICROSERVICE_URL_BULK (default empty) purely as the seed source
    for migrate_pipeline_settings; it is not read directly at runtime.
  • Docsdocs/deployment/performance_tuning.md gains a "Separate bulk
    embeddings pool" section; sample env files note the optional variable;
    changelog fragment changelog.d/bulk-embeddings-pool.added.md.

Opt-in and backwards compatible: when no bulk URL is configured the
use_bulk_pool flag is a no-op, so single-pool deployments are unaffected.
Hosted / multimodal embedders (which read distinct kwargs) ignore the flag.

Test plan

  • New unit tests:
    • MicroserviceEmbedder._get_service_config — bulk selection when flagged,
      fallback to query URL when the bulk URL is empty, and no bulk use without
      the flag (opencontractserver/tests/test_batch_embedding.py).
    • Ingest leaves tag use_bulk_pool=True_create_text_embedding /
      _embed_relationship (test_embeddings_task.py) and the batch path
      _batch_embed_text_annotations (test_batch_embedding.py).
  • black, isort, flake8 clean on all changed files; targeted mypy clean
    on the embedder and task modules.
  • python scripts/collate_changelog.py --check passes.
  • The full Docker-based test suite requires infrastructure not available in this
    environment; the URL-selection logic was verified in isolation and the rest is
    left to CI (pytest).

Checklist

  • Tests added for the code this PR touches (full suite runs in CI)
  • black / isort / flake8 pass on changed files; targeted mypy clean (no frontend changes, so no prettier/TS)
  • A changelog fragment was added under changelog.d/
  • No new dependencies

Contributor License Agreement

By submitting this pull request, you agree to license your contribution
under the project's Contributor License Agreement.


Generated by Claude Code

Search queries and batch ingest share one embeddings microservice URL, forcing
a compromise between a warm query pod and an autoscaled ingest pool. This lets
operators split them without adding a parallel configuration pathway.

The bulk URL is a new optional field on MicroserviceEmbedder.Settings
(embeddings_microservice_url_bulk), seeded from the EMBEDDINGS_MICROSERVICE_URL_BULK
env var via migrate_pipeline_settings — configured through the same PipelineSettings
singleton as the existing query URL, not read ad hoc from Django settings. The
ingest Celery tasks in embeddings_task.py tag their embed calls with
use_bulk_pool=True; MicroserviceEmbedder._get_service_config routes tagged calls to
the bulk URL when one is configured and leaves every (untagged) search query on
embeddings_microservice_url. When no bulk URL is set the flag is a no-op, so
single-pool deployments are unaffected and no query call site changes.

Because ingest is inherently bulk, the leaves just tag their embed calls — no
override parameter is threaded through the task helpers, so their signatures are
unchanged.

Tests: MicroserviceEmbedder._get_service_config bulk selection + fallback +
no-flag cases; ingest leaves (_create_text_embedding, _embed_relationship,
_batch_embed_text_annotations) tag use_bulk_pool=True. Docs: performance_tuning.md
section, sample env files, changelog fragment.
Comment thread opencontractserver/tasks/embeddings_task.py Fixed
@claude

claude Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review: Add bulk embeddings pool for ingest

Overall this is a clean, well-scoped change that follows the existing PipelineSettings pathway correctly (no parallel config surface, generic **kwargs flag that other embedders safely ignore). Went through _get_service_config, the ingest call sites, migrate_pipeline_settings, and the tests. One operational gap worth fixing before merge; everything else is minor/non-blocking.

Correctness of the routing logic — looks solid

  • _get_service_config in opencontractserver/pipeline/embedders/sent_transformer_microservice.py:208-224 correctly falls back to query_url when use_bulk_pool is unset OR when it's set but bulk_url is empty — matches the "opt-in, no-op by default" claim.
  • maybe_add_cloud_run_auth is called with the already-resolved service_url, so Cloud Run IAM audience is derived from whichever pool was actually selected — no bug there.
  • Confirmed all search-side call sites (utils/embeddings.py:252, discovery/views.py:895, llms/vector_stores/core_vector_stores.py:868, utils/multimodal_embeddings.py:329, mcp/tools.py:441) do not pass use_bulk_pool, so they correctly stay pinned to the query URL.
  • _batch_embed_items (shared by both _batch_embed_text_annotations and _batch_embed_relationships) hardcodes use_bulk_pool=True inside _embed_one — verified both callers are ingest-only, so this is safe, not a leak into query paths.
  • Other embedders (openai_embedder.py, multimodal_microservice.py) declare _embed_text_impl(self, text, **all_kwargs) / have their own unrelated _get_service_config, so the unconditional use_bulk_pool=True tag from ingest tasks is silently and safely ignored for non-microservice embedders, as the PR description claims.

Operational gap: the documented "how to enable it" steps will likely no-op on an existing deployment

docs/deployment/performance_tuning.md, and both sample .django files, instruct operators to:

EMBEDDINGS_MICROSERVICE_URL_BULK=http://vector-embedder-bulk:8000
python manage.py migrate_pipeline_settings

But production.yml's migrate profile already runs migrate_pipeline_settings (no --force) on every deploy:

python manage.py migrate_pipeline_settings --sync-preferences --init-only && python manage.py migrate_pipeline_settings

Once this PR merges and is deployed once, that auto-run will persist embeddings_microservice_url_bulk: "" into PipelineSettings.component_settings for MicroserviceEmbedder — even if the env var was never set (see migrate_pipeline_settings.py:238: final_value = env_value if env_value is not None else default, then stored since final_value is not None). On every subsequent run, has_existing_db becomes True for that key, and without --force the command preserves the existing (empty) DB value (migrate_pipeline_settings.py:232-239) rather than picking up a newly-set env var.

So an operator who deploys, later decides to set EMBEDDINGS_MICROSERVICE_URL_BULK, and follows the docs' plain migrate_pipeline_settings command will find the bulk URL silently ignored — the setting stays "" and ingest keeps going through the query pool. This is easy to miss since the command exits 0 and only prints [PRESERVED] in --verbose mode.

Suggested fix: update the three docs (performance_tuning.md, both sample env files) to recommend a scoped force, e.g.:

python manage.py migrate_pipeline_settings --component MicroserviceEmbedder --force

(--component accepts either the registered name or class name — see migrate_pipeline_settings.py:154-159 — so this avoids force-overwriting unrelated component settings that may have been hand-tuned in the DB.)

Minor / non-blocking

  • The bulk pool shares vector_embedder_api_key / use_cloud_run_iam_auth with the query pool (_get_service_config). If the bulk pool is a genuinely separate deployment (e.g. different Cloud Run service or API key), there's no way to configure that independently yet. Probably fine as a deliberate v1 scope limitation given the PR is specifically about URL routing, but worth a one-line callout in the docs so it's a known limitation rather than a surprise.
  • Test coverage for the routing logic itself (test_batch_embedding.py) and the ingest tagging (test_embeddings_task.py) is good and targeted. No test exercises the migrate_pipeline_settings preserve-on-rerun interaction described above, which is how the doc gap above would actually surface — consider a regression test or at least explicitly calling out --force/--component in the runbook since this is the actual repro path.

Nice, focused PR otherwise — the "tag intent, not URLs" design (ingest tags use_bulk_pool=True, the embedder decides the URL) is a good way to avoid threading an override parameter through every task helper signature.

@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

The ingest tasks now tag their embedder calls with use_bulk_pool=True, which
broke 15 tests that either used mock embedders with a narrow embed_text(self,
text) signature or asserted the exact embed_text/embed_texts_batch call args:

- test_dual_embeddings.py: MockEmbedder / MockCorpusEmbedder embed_text now
  accept **kwargs, matching the BaseEmbedder.embed_text(self, text,
  **direct_kwargs) contract that every real embedder already satisfies.
- test_embeddings_task.py: the embed_text / embed_texts_batch call assertions on
  ingest paths (annotation text, multimodal fallback, relationship single +
  batch) now expect use_bulk_pool=True.

Also address the review note that migrate_pipeline_settings preserves existing
PipelineSettings values without --force, so an operator who sets
EMBEDDINGS_MICROSERVICE_URL_BULK on an already-deployed instance and runs the
plain command would keep the seeded empty value. Docs and both sample env files
now use 'migrate_pipeline_settings --component MicroserviceEmbedder --force', and
note the shared api-key/Cloud-Run-auth scope limitation.
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