Skip to content

Reduce embedding task storms: broker visibility timeout, bounded dispatch, chunk sizing - #3645

Open
mbertrand wants to merge 6 commits into
mainfrom
mb/embed-mechanics
Open

Reduce embedding task storms: broker visibility timeout, bounded dispatch, chunk sizing#3645
mbertrand wants to merge 6 commits into
mainfrom
mb/embed-mechanics

Conversation

@mbertrand

@mbertrand mbertrand commented Jul 19, 2026

Copy link
Copy Markdown
Member

What are the relevant tickets?

Addresses mitodl/hq#12453. Related: mitodl/hq#12015 (infra concurrency cap + grace period), mitodl/hq#12008 (Qdrant performance settings), mitodl/hq#12172 (embedding pipeline failures).

Description (What does it do?)

Phase 1 of the content-file embedding load reduction — task/broker mechanics that amplify legitimate work into storms. No change to what gets embedded.

  • Broker visibility_timeout → 2h (CELERY_BROKER_TRANSPORT_OPTIONS, env CELERY_BROKER_VISIBILITY_TIMEOUT). The default 3600s is shorter than a long summarize+embed task, so in-flight tasks were redelivered en masse during backlogs (the redelivery-storm driver). Sized against the new task time limits below, since it's also the recovery delay for messages orphaned by a hard pod kill.
  • Time limits on generate_embeddings: CELERY_EMBEDDINGS_SOFT_TIME_LIMIT=1800 / CELERY_EMBEDDINGS_TIME_LIMIT=2400, so a wedged summarize/embed call can't hold a message unacked for the whole visibility timeout.
  • Split chunk sizes: QDRANT_CHUNK_SIZE 10→100 (resources, cheap per item); new QDRANT_CONTENT_FILE_CHUNK_SIZE=25 (content files do inline summarization, kept small); new QDRANT_DELETE_CHUNK_SIZE for removal tasks (delete volume drives Qdrant's vacuum optimizer); new MARKETING_PAGE_SCRAPE_CHUNK_SIZE=10 so marketing-page scraping is no longer coupled to the embedding chunk size.
  • ignore_result=True on fire-and-forget embedding tasks; drop the finalize_embeddings tail and the embed_errors counter plumbing. Failure signal is now log.exception in generate_embeddings plus the weekly presence-based embeddings_healthcheck. (failure_key and finalize_embeddings are kept one release so in-flight messages from the previous release still run; remove next release.)
  • Bounded backfill dispatch: start_embed_resources / embed_learning_resources_by_id go from self.replace(chain(...)) over the whole catalog (O(N²) message bytes) to per-chunk apply_async publishes. Per-run dispatch is likewise individual publishes — deliberately not a group, since celery uplifts a replaced group into a chord, which reintroduces per-chunk result bookkeeping in Redis even for ignore_result tasks.
  • embed_run_content_files filters to published files — stop embedding files the very next chained task deletes.
  • generate_embeddings.rate_limit from a new CELERY_EMBEDDINGS_RATE_LIMIT setting, default 20/m. The rate limit counts tasks/minute, so with the 10× larger resource chunks this holds the pre-chunking item rate (~2k/min/worker) instead of multiplying it. Ops can tune without a deploy.
  • try_with_retry_as_task dispatches via a single apply_async() (celery's default publish retry, 3 attempts) instead of re-.delay() on any exception (which could double-publish during a broker hiccup).
  • LiteLLMEncoder.embed_documents batches requests (LITELLM_EMBEDDING_BATCH_SIZE=25): 25 × the 8191-token per-document limit stays under OpenAI's 300k-token-per-request cap, which fails as a non-transient 400.
  • Also batched Qdrant point retrieves in vector_search/utils.py (one sub-batched retrieve per chunk instead of one per document) and factored the upsert batching into _upsert_points.

How can this be tested?

  1. Config applied:
    docker compose run --rm web python manage.py shell -c \
      "from main.celery import app; from django.conf import settings; \
       print(app.conf.broker_transport_options.get('visibility_timeout'), \
             settings.QDRANT_CHUNK_SIZE, settings.QDRANT_CONTENT_FILE_CHUNK_SIZE, \
             settings.CELERY_EMBEDDINGS_RATE_LIMIT)"
    
    Expect 7200 100 25 20/m.
  2. ignore_result — a dispatched embedding task stores no result key:
    docker compose exec -T redis redis-cli EXISTS celery-task-meta-<id>0.
  3. Mixed published/unpublished ingest (hooks on): embed_run_content_files embeds only the published files, dispatched as ⌈published/25⌉ individual chunk tasks, points land in Qdrant with no redelivered-task warnings.
  4. Test suites: docker compose run --rm web uv run pytest vector_search/tasks_test.py vector_search/utils_test.py vector_search/encoders/litellm_test.py learning_resources_search/plugins_test.py learning_resources/tasks_test.py — all pass.

Additional Context

Ops note before merge: confirm the prod env doesn't pin QDRANT_CHUNK_SIZE in ol-infrastructure — its default is what changes here. This is the app-side complement to the infra-side fixes in mitodl/hq#12015.

Tradeoff called out for review: dropping finalize_embeddings trades loud per-run FAILURE finalization for log.exception + Sentry + the weekly presence-based healthcheck. Given hq#12172 was partly about silent degradation, reviewers should confirm that signal is sufficient.

Copilot AI review requested due to automatic review settings July 19, 2026 13:48
@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown

OpenAPI Changes

No changes detected

View full changelog

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts the Celery/Qdrant embedding pipeline’s dispatch mechanics to reduce broker amplification during backlogs, improve batching behavior, and make embedding execution more operationally tunable—without changing which resources/content files are embedded.

Changes:

  • Increased broker safety by configuring a longer broker visibility_timeout, and made generate_embeddings rate limiting configurable via settings.
  • Reduced broker/result-backend load by making embedding tasks fire-and-forget (ignore_result=True) and removing the prior “finalize” tail/counter plumbing.
  • Improved embedding efficiency by batching Qdrant point retrievals and skipping some redundant payload writes; updated related tests accordingly.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
main/settings.py Updates embedding chunk-size defaults and introduces a dedicated content-file chunk size setting.
main/settings_celery.py Adds broker visibility_timeout transport options and makes embedding task rate-limit configurable.
vector_search/tasks.py Refactors embedding task dispatch patterns (groups vs chains), adds ignore_result on embedding tasks, filters run embedding to published files.
vector_search/utils.py Adds batched Qdrant retrieve helper and uses retrieved points to reduce per-document Qdrant calls and some payload writes.
learning_resources_search/plugins.py Switches to publish-time retry on task dispatch via apply_async(retry=..., retry_policy=...).
vector_search/tasks_test.py Updates expectations for new dispatch semantics and removal of finalize/counter behavior.
vector_search/utils_test.py Adds coverage for batched retrieval and payload-write skipping behavior.

Comment thread vector_search/tasks.py Outdated
Comment thread vector_search/tasks.py
@mbertrand
mbertrand force-pushed the mb/embed-mechanics branch 2 times, most recently from ad1ac5b to f4050e9 Compare July 28, 2026 18:50
mbertrand and others added 4 commits August 3, 2026 11:55
Phase 1 (mechanics) of the content-file embedding load reduction:

- Raise Redis broker visibility_timeout to 6h so long-running
  summarization+embed tasks are not redelivered mid-flight during a
  backlog (the primary redelivery-storm driver).
- Split QDRANT_CHUNK_SIZE (10->100 for resources) from a new
  QDRANT_CONTENT_FILE_CHUNK_SIZE (25) so content-file tasks stay small.
- ignore_result on fire-and-forget embedding tasks; drop the
  finalize_embeddings tail and embed_errors counter plumbing (failure
  signal is log.exception + the weekly presence-based healthcheck).
- Convert whole-catalog backfill from self.replace(chain) to bounded
  apply_async batches; per-run dispatch becomes a group of chunk sigs
  (no O(N^2) chain payloads).
- embed_run_content_files filters to published files.
- generate_embeddings rate_limit from CELERY_EMBEDDINGS_RATE_LIMIT.
- try_with_retry_as_task dispatches via apply_async with publish-time
  retry_policy instead of re-.delay() on any exception.

Addresses mitodl/hq#12453
Related: mitodl/hq#12015 (infra concurrency/grace-period counterpart),
mitodl/hq#12008 (Qdrant performance settings), mitodl/hq#12172
(embedding pipeline failures).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Covers the published-only filter returning None (no group), per review
feedback.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Reorder the content_files_loaded chain to purge unpublished files'
  points before embedding (cherry-picked from the change-detection
  follow-up). celery uplifts a group replacement to a chord, so with
  embed first the purge task becomes the chord body; if the gap between
  two chunk completions exceeds result_expires (1h) the chord's Redis
  bookkeeping expires and the body is dropped with no error logged.
  Purging first makes the body a bare celery.accumulate, whose loss
  costs nothing. The two tasks touch disjoint sets (published=False vs
  published=True).
- Drop the payload-equality write-skip in update_learning_resource_payload.
  It never fired: Qdrant returns resource_age_date as a string while the
  serialized doc holds a datetime, so the payloads never compared equal.
  Its test asserted against a MagicMock echoing its own input, so the
  no-op looked covered. The batched retrieve it rode on is unaffected and
  still saves N retrieves per sweep.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (3)

main/settings_celery.py:37

  • PR description/testing steps expect broker visibility_timeout to be 6h (21600s), but the default here is 2h (7200s). If the intent is to raise the default to prevent redelivery storms without requiring env overrides, update the default to 6h (or adjust the PR description/test expectation).
CELERY_BROKER_TRANSPORT_OPTIONS = {
    "visibility_timeout": get_int(
        name="CELERY_BROKER_VISIBILITY_TIMEOUT", default=2 * 60 * 60
    ),
}

main/settings_celery.py:239

  • PR description/testing steps mention keeping the generate_embeddings rate_limit default at 200/m, but this setting defaults to 20/m. Either the default should be 200/m or the PR description/test instructions need to be updated to match 20/m.
# minute, so the item rate against Qdrant is this times the chunk size. Kept at
# 20/m so the QDRANT_CHUNK_SIZE=100 resource chunks hold the pre-chunking item
# rate (~2k/min/worker) instead of multiplying it. Changing it needs a worker
# restart; use celery's control.rate_limit for a live change.
CELERY_EMBEDDINGS_RATE_LIMIT = get_string("CELERY_EMBEDDINGS_RATE_LIMIT", "20/m")

vector_search/tasks.py:108

  • This docstring says callers get an "error string on failure", but _dispatch_signatures never returns an error string—publish failures will raise from apply_async(). Either adjust the docstring to match the behavior, or add exception handling that returns an error string consistently.
    Nothing waits on the results (embedding tasks are ignore_result), so this
    returns None: callers' return value stays "falsy on success, error string on
    failure".

mbertrand and others added 2 commits August 3, 2026 12:30
…string

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…etry_policy

should_generate_resource_embeddings / should_generate_content_embeddings now
require the batch-retrieved stored_point (all production callers already pass
it), removing the _UNSET sentinel, per-document fallback retrieves, and the
now-unused _retrieve_content_file_point.

try_with_retry_as_task relies on celery's default publish retry
(task_publish_retry=True, 3 attempts) instead of an explicit near-identical
retry_policy.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comment on lines +33 to +34
signature = function.si(*args) if isinstance(function, Task) else function
signature.apply_async()
@mbertrand mbertrand changed the title Embedding pipeline mechanics: broker safety, chunking, dispatch Reduce embedding task storms: broker visibility timeout, bounded dispatch, chunk sizing Aug 3, 2026
@shanbady
shanbady self-requested a review August 3, 2026 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants