Followup to #3646: skip contentless files in embed pre-pass, retry Qdrant blips, purge before embed - #3684
Conversation
34edf54 to
3536a09
Compare
OpenAPI Changes150 changes: 0 error, 0 warning, 150 info Unexpected changes? Ensure your branch is up-to-date with |
8526807 to
0eaef52
Compare
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. |
3536a09 to
fb72d6f
Compare
fb72d6f to
0d01a4e
Compare
shanbady
left a comment
There was a problem hiding this comment.
Looks good. i'm unsure if its even worth resolving here since it probably doesnt happen very often if ever but there is one edge case:
A published file whose content goes from non-empty to empty keeps its old qdrant point indefinitely
18e28c6 to
711ce89
Compare
|
@shanbady I made another commit to deal with those contentless files. |
…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>
…e empty The pre-pass excludes contentless files, and the unpublished purge only covers published=False, so a file whose content went non-empty -> empty kept the point embedded from its old content indefinitely. Detect those leftovers in the same batched payload retrieve (zero steady-state cost) and chain their removal after the embed chunks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
711ce89 to
95dd3da
Compare
shanbady
left a comment
There was a problem hiding this comment.
left some minor nitpicks
| .values_list("id", "key", "checksum", *CONTENT_FILE_PREPASS_PAYLOAD_FIELDS) | ||
| ] | ||
| contentless_rows = [ | ||
| (cf_id, chunk0_point_id(key)) |
There was a problem hiding this comment.
super tiny nit-pick with the name chunk0_point_id (it feels a bit odd). could we rename to chunk_0_point_id or first_chunk_point_id?
There was a problem hiding this comment.
Also this comment could be more clear. somehting along:
"returns the qdrant point id for the first chunk of the contentfile"
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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: