Skip to content

Followup to #3646: skip contentless files in embed pre-pass, retry Qdrant blips, purge before embed - #3684

Open
mbertrand wants to merge 1 commit into
mainfrom
mb/embed-change-detection-followup
Open

Followup to #3646: skip contentless files in embed pre-pass, retry Qdrant blips, purge before embed#3684
mbertrand wants to merge 1 commit into
mainfrom
mb/embed-change-detection-followup

Conversation

@mbertrand

@mbertrand mbertrand commented Jul 27, 2026

Copy link
Copy Markdown
Member

What are the relevant tickets?

Followup to #3646 (mitodl/hq#12454). Three enhancements to the change-detection pre-pass.

Description (What does it do?)

1. Content-less files are excluded from the pre-pass. A published ContentFile
with null/empty content never produces a Qdrant point, so the pre-pass counted it as
"missing" and re-flagged it on every load. Those files now never enter the candidate
list, so an unchanged run settles at a true 0 of N.

2. Transient Qdrant errors during the pre-pass retry instead of deferring the run.
embed_run_content_files gets max_retries=3, and a DEADLINE_EXCEEDED / UNAVAILABLE
from the batched payload fetch retries with the existing _retry_countdown backoff.
Previously a single blip failed the task and pushed the whole run's embedding to the
next load.

3. Purge runs before embed in the indexing chain. remove_unpublished_run_content_files
is now queued ahead of embed_run_content_files in the content_files_loaded hook. The
two tasks touch disjoint sets (published=False vs published=True), so ordering is
free — but with purge first, unpublished files' points are removed even when the embed
step fails.

How can this be tested?

Verified locally against OCW 15.S12 Blockchain and Money, Fall 2018
(15.S12+fall_2018, run eca120b8a78ad938e60920323e127f21) — 58 published content
files, 3 of which are content-less. Get its run pk:

from learning_resources.models import LearningResourceRun
run = LearningResourceRun.objects.get(run_id="eca120b8a78ad938e60920323e127f21")
print(run.id, run.content_files.filter(published=True).count())

If that run isn't in your DB, any run with a mix of content-bearing and content-less
published files works — find one with:

from django.db.models import Q, Count
from learning_resources.models import ContentFile
ContentFile.objects.filter(published=True).values("run_id").annotate(
    empty=Count("id", filter=Q(content__isnull=True) | Q(content="")),
    total=Count("id"),
).filter(empty__gt=0, total__gt=5).order_by("total")[:5]

Restart the celery worker first — prefork workers don't reload changed code, so a
worker started before checkout will silently run the old task and log nothing:

docker compose restart celery

Content-less files excluded (fix 1)

from vector_search.tasks import embed_run_content_files
embed_run_content_files.delay(RUN_PK)
docker compose logs celery --since 5m | grep 'need embedding'

Queue it a second time once the first pass settles. Expect 0 of 55 — i.e. N equals
published-with-content, not total published:

embed_run_content_files run 4232: 0 of 55 files need embedding

On mb/embed-change-detection the same run reports 3 of 58 on every single load,
forever. The three files genuinely have no Qdrant point, which you can confirm directly:

from django.db.models import Q
from learning_resources.models import ContentFile, LearningResourceRun
from vector_search.tasks import _stored_content_payloads
from vector_search.utils import vector_point_id, vector_point_key

run = LearningResourceRun.objects.select_related("learning_resource__platform").get(id=RUN_PK)
res = run.learning_resource

def pid(key):
    return vector_point_id(vector_point_key(
        {"platform": {"code": res.platform.code}, "resource_readable_id": res.readable_id,
         "run_readable_id": run.run_id, "key": key},
        chunk_number=0, document_type="content_file"))

empty = ContentFile.objects.filter(run=run, published=True).filter(
    Q(content__isnull=True) | Q(content="")).values_list("id", "key")
stored = _stored_content_payloads([pid(k) for _, k in empty], fields=("checksum",))
for cid, k in empty:
    print(cid, k, stored.get(pid(k)))   # all None

Qdrant blip retries (fix 2)

docker compose stop qdrant

Queue the task and watch the log — it should retry rather than fail terminally:

embed_run_content_files[...] retry: Retry in 64s: <_InactiveRpcError ...
        status = StatusCode.UNAVAILABLE

Bring Qdrant back inside that backoff window and the retry completes the pre-pass:

docker compose start qdrant
embed_run_content_files run 4232: 0 of 55 files need embedding
Task vector_search.tasks.embed_run_content_files[...] succeeded in 18.2s

Before this change the UNAVAILABLE was terminal and the run's embedding waited for
the next load.

Purge before embed (fix 3)

Unpublish one already-embedded file and fire the hook:

from learning_resources.models import ContentFile, LearningResourceRun
from learning_resources.utils import content_files_loaded_actions

run = LearningResourceRun.objects.get(id=RUN_PK)
cf = ContentFile.objects.filter(run=run, published=True).exclude(
    content="").exclude(content__isnull=True).first()
print(cf.id, cf.key)
ContentFile.objects.filter(id=cf.id).update(published=False)
content_files_loaded_actions(run)
docker compose logs celery --since 5m | grep -E 'remove_unpublished|remove_embeddings|embed_run_content_files|need embedding'

Expected order — the purge fully completes before embedding starts:

remove_unpublished_run_content_files[...] received
remove_embeddings[...] received
remove_embeddings[...] succeeded
embed_run_content_files[...] received
embed_run_content_files run 4232: 0 of 54 files need embedding

Then confirm the unpublished file's point is gone (re-use the pid() helper above —
_stored_content_payloads([pid(cf.key)], fields=("checksum",)) returns {}). It should
be absent regardless of how the embed step ends.

Republish it and fire the hook again to restore local state; the pre-pass picks it back
up as missing, which also shows the self-heal path:

embed_run_content_files run 4232: 1 of 55 files need embedding

@mbertrand
mbertrand force-pushed the mb/embed-change-detection-followup branch from 34edf54 to 3536a09 Compare July 27, 2026 16:55
@github-actions

github-actions Bot commented Jul 27, 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).

@mbertrand
mbertrand requested a review from Copilot July 27, 2026 16:58
@mbertrand
mbertrand changed the base branch from mb/embed-change-detection to main July 27, 2026 16:58
@mbertrand
mbertrand changed the base branch from main to mb/embed-change-detection July 27, 2026 16:59
@mbertrand
mbertrand force-pushed the mb/embed-change-detection branch from 8526807 to 0eaef52 Compare July 27, 2026 17:00

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 is a follow-up to the Qdrant change-detection work, improving the content-file embedding workflow by (1) excluding content-less files from the embed pre-pass, (2) retrying transient Qdrant blips during the pre-pass instead of deferring embedding, and (3) reordering the indexing hook to purge unpublished points before embedding so purges still happen even if embedding fails.

Changes:

  • Add batched Qdrant payload retrieval (_stored_content_payloads) and use it to gate re-embedding vs payload-only refresh.
  • Update embed_run_content_files to: skip content-less and unpublished files, batch-compare DB vs stored Qdrant payload fields, and retry transient gRPC errors with backoff.
  • Reorder content_files_loaded indexing chain to run unpublished-point purge before embedding, with tests for the new ordering.

Reviewed changes

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

Show a summary per file
File Description
vector_search/utils.py Adds batched payload retrieval and uses stored payloads to avoid unnecessary re-embedding.
vector_search/utils_test.py Updates tests for new batching/gating logic and adds coverage for _stored_content_payloads.
vector_search/tasks.py Enhances embed_run_content_files with pre-pass filtering, batched comparisons, and transient retry behavior.
vector_search/tasks_test.py Adds coverage for skipping unpublished/content-less files, pre-pass staleness detection, and retry behavior.
vector_search/constants.py Defines the set of ContentFile fields compared in the pre-pass for payload drift detection.
learning_resources/utils.py Improves docstring clarity for content_files_loaded_actions.
learning_resources/hooks.py Improves hook docstring clarity for content_files_loaded.
learning_resources_search/plugins.py Reorders Qdrant tasks to purge unpublished points before running embed.
learning_resources_search/plugins_test.py Adds a test asserting purge precedes embed in the chained tasks.

Comment thread vector_search/utils_test.py
@mbertrand
mbertrand force-pushed the mb/embed-change-detection-followup branch from 3536a09 to fb72d6f Compare July 27, 2026 17:18
Base automatically changed from mb/embed-change-detection to main July 27, 2026 19:26
…mbed

Follow-up to the embed change-detection PR (#3646):

- Exclude content-less files from the embed_run_content_files pre-pass:
  they never produce Qdrant points, so they were permanently flagged as
  stale and re-dispatched on every load, defeating the zero-task fast
  path for unchanged runs.
- Retry transient Qdrant errors (DEADLINE_EXCEEDED/UNAVAILABLE) in the
  pre-pass with the existing jittered backoff instead of failing the
  run's embedding outright.
- Reorder the content_files_loaded chain to purge unpublished files'
  points before embedding, so a failed embed task can never strand
  unpublished points in Qdrant. The two tasks touch disjoint sets.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mbertrand
mbertrand force-pushed the mb/embed-change-detection-followup branch from fb72d6f to 0d01a4e Compare July 27, 2026 19:32
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