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
Open
Followup to #3646: skip contentless files in embed pre-pass, retry Qdrant blips, purge before embed#3684mbertrand wants to merge 1 commit into
mbertrand wants to merge 1 commit into
Conversation
mbertrand
force-pushed
the
mb/embed-change-detection-followup
branch
from
July 27, 2026 16:55
34edf54 to
3536a09
Compare
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
mbertrand
force-pushed
the
mb/embed-change-detection
branch
from
July 27, 2026 17:00
8526807 to
0eaef52
Compare
Contributor
There was a problem hiding this comment.
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_filesto: skip content-less and unpublished files, batch-compare DB vs stored Qdrant payload fields, and retry transient gRPC errors with backoff. - Reorder
content_files_loadedindexing 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. |
mbertrand
force-pushed
the
mb/embed-change-detection-followup
branch
from
July 27, 2026 17:18
3536a09 to
fb72d6f
Compare
…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
force-pushed
the
mb/embed-change-detection-followup
branch
from
July 27, 2026 19:32
fb72d6f to
0d01a4e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
ContentFilewith null/empty
contentnever 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_filesgetsmax_retries=3, and aDEADLINE_EXCEEDED/UNAVAILABLEfrom the batched payload fetch retries with the existing
_retry_countdownbackoff.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_filesis now queued ahead of
embed_run_content_filesin thecontent_files_loadedhook. Thetwo tasks touch disjoint sets (
published=Falsevspublished=True), so ordering isfree — 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, runeca120b8a78ad938e60920323e127f21) — 58 published contentfiles, 3 of which are content-less. Get its run pk:
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:
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:
Content-less files excluded (fix 1)
Queue it a second time once the first pass settles. Expect
0 of 55— i.e.Nequalspublished-with-content, not total published:
On
mb/embed-change-detectionthe same run reports3 of 58on every single load,forever. The three files genuinely have no Qdrant point, which you can confirm directly:
Qdrant blip retries (fix 2)
Queue the task and watch the log — it should retry rather than fail terminally:
Bring Qdrant back inside that backoff window and the retry completes the pre-pass:
Before this change the
UNAVAILABLEwas terminal and the run's embedding waited forthe next load.
Purge before embed (fix 3)
Unpublish one already-embedded file and fire the hook:
Expected order — the purge fully completes before embedding starts:
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 shouldbe 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: