docs(cohort-4): spec the ContentFile scoped-pull trigger contract - #2582
Open
blarghmatey wants to merge 7 commits into
Open
blarghmatey wants to merge 7 commits into
blarghmatey wants to merge 7 commits into
Conversation
Resolves the open design question for Cohort 4. The task framed it as "webhook vs Learn polling", but the webhook shipped some time ago and runs in production from three Dagster locations -- so the real question was what that webhook should mean once the platform, rather than MIT Learn, does the Tika extraction. Specs a new endpoint rather than reinterpreting the existing one. All three senders share one path, so changing the handler's behaviour would flip openedx, edxorg and Canvas at once, with no per-source rollback; a new path lets sources move one at a time and makes the coexistence explicit in routing instead of hidden in a per-ETLSource branch. The part that needs new code is scoping. BaseWarehouseETLTask has two modes -- full refresh, which prunes, and incremental, which deliberately never prunes because a partial pull is not the complete state of the source. A per-course trigger is neither: within one course it IS complete and a removed file must be unpublished, but pruning globally from a single-course pull would unpublish the whole corpus. The spec calls for a third mode whose prune predicate is scoped to the same key as the fetch, plus a published-row-count guard -- the podcast full-sync hazard at a different granularity. Also records the O(N^2) automation-condition outage already documented in openedx.py as a hard constraint on the per-asset cadence choice, and flags that OCW has no sender at all and needs one built regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VcWh3q3Bi2mxBmaiPCbBW9
Contributor
There was a problem hiding this comment.
Pull request overview
Documents the proposed ContentFile scoped-pull contract for Cohort 4.
Changes:
- Defines the new webhook payload and receiver workflow.
- Specifies scoped pruning safeguards and scheduling constraints.
- Documents migration sequencing and the missing OCW sender.
Suppressed comments (1)
docs/design/contentfile_scoped_pull_contract.md:141
- Section 8 says there is no OCW sender, so “all four senders are already partitioned” is incorrect and could cause the new OCW asset to omit an explicit partitioning/batching design.
Cadence is chosen per sending asset via Dagster automation conditions / freshness policies, not a
hardcoded cron. All four senders are already partitioned per course, so per-partition triggering
needs no restructuring.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Constraint for §9, surfaced reviewing #2589 (#2589 (comment)): MIT Learn's So unless the view derives text from |
mbertrand
reviewed
Sep 2, 2026
The prune predicate named run.learning_resource.readable_id, but that's course-level and would unpublish every other run's files — confirmed against mit-learn's process_create_content_file_request and ContentFile.objects.filter(run=course_run, ...), the scope key MIT Learn actually prunes on is run.run_id (sent today as course_readable_id). Added etl_source alongside it since run_id alone collides across sources. Also: BaseWarehouseETLTask moved from the closed mit-learn#3566 (Trino) to merged mit-learn#3807 (StarRocks) — updated the PR link and engine name, and flagged that iter_rows/run() don't support a scope predicate yet. Fixed line-number drift on four other citations and corrected "all four senders" to three (OCW has no sender yet, per §8). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGqjZ3r5u7kpyTLRMwwLdJ
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…trictness
Canvas: the new sender computes readable_id itself (course_id +
course_code, matching Learn's f"{course_folder}-{course_code}"
derivation confirmed in etl/canvas.py:138 and canvas_course_folder()),
rather than depending on Canvas's sis_course_id being set the way the
existing webhook does today.
Count guard: any content_file_count mismatch skips the prune, not just
a "material" one — both numbers come from the same publish event, so a
gap is a read-consistency race (StarRocks/Iceberg catalog lag, a dbt
rebuild in flight), not a real content change, and the wrong-direction
error (unpublishing files that still exist) is worse than pruning
stalling. Left the zero-file-course override as an explicit open
question since the strict guard as written blocks that case too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WGqjZ3r5u7kpyTLRMwwLdJ
Self-heal: a daily reconciliation sweep on the Learn side replaces the retiring Celery beat sweeps as the backstop against a dropped webhook POST. Chose Learn-side over a platform-side re-announce because the sweep is now just a SQL pull against an already-materialized warehouse view, not a re-triggered extraction, so it doesn't hit the O(N^2) per-partition hazard §5 documents. Prune race: serialize scoped pulls per readable_id with a lock, rather than a staleness check against a payload timestamp, to stop an earlier-started pull's stale prune from unpublishing a later pull's upserts for the same run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGqjZ3r5u7kpyTLRMwwLdJ
readable_id in §9's schema meant the payload's course/run-level field and the view's own per-row identity at once, since this view is one row per file. Renamed to run_readable_id, keeping key as the file identity. Also added edx_module_id, source_path, file_extension, and checksum, confirmed present on ContentFile in mit-learn's models.py, since MIT Learn persists all four today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGqjZ3r5u7kpyTLRMwwLdJ
The platform, not Learn, needs to hear about an OCW publish once the platform owns extraction — narrowed to Concourse calling a new platform-side endpoint, or a Dagster sensor on the OCW S3 bucket, and set aside polling OCW Studio's publish_date to stay closer to today's push-based, low-latency model. Flagged that whichever is chosen still needs to cover unpublish, which the existing webhook handles immediately and which the §7 daily sweep is too slow to replace. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WGqjZ3r5u7kpyTLRMwwLdJ
…ble_id
Sentry flagged that the readable_id formula named course_folder without
saying how the sender gets it. Learn's own canvas_course_folder()
docstring says that value is the Canvas course id, just reached via the
archive's S3 key path today — so the sender's formula is
f"{course_id}-{course_code}" using fields it already has, no separate
course_folder lookup needed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WGqjZ3r5u7kpyTLRMwwLdJ
Member
Author
|
Investigated further — this needs a capa/OLX-aware tree-walk over the parsed block XML that splits student-safe question text from embedded |
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?
MIT Learn ETL → OL Data Platform migration, Cohort 4: https://github.com/mitodl/hq/issues/11513
Resolves the open design question tracked as
tk-define-contentfile-dagster-mit-learn-trigger-mec-8641eb.Description (What does it do?)
Adds
docs/design/contentfile_scoped_pull_contract.md, specifying how Dagster tells MIT Learn that extracted ContentFiles are ready, now that Cohort 4 moves Tika extraction to the platform. Docs only — no code.The question the task asked was already answered, but not the one that mattered. It framed the decision as "(a) webhook from platform to Learn vs (b) Learn polls Dagster asset metadata". (a) shipped some time ago and runs in production from three Dagster locations:
dg_projects/openedx/openedx/assets/openedx.py:725partitions_defdg_projects/edxorg/edxorg/assets/openedx_course_archives.py:340course_and_source_partitionsdg_projects/canvas/canvas/assets/canvas.py:294canvas_course_ids(DynamicPartitionsDefinition)All three POST
{course_id, course_readable_id, content_path, source}to/api/v1/webhooks/content_files/, and MIT Learn responds by firingingest_edx_run_archive— i.e. "go extract this archive yourself." Cohort 4 inverts that. So the real question is what the message should mean once the platform does the extracting.Things worth a reviewer's attention
New endpoint, not a reinterpretation. All three senders share one path, so changing
process_create_content_file_requestwould flip openedx, edxorg and Canvas simultaneously — no per-source rollback. Keeping them independent under the old path means branching perETLSource, which is the same dual-meaning problem relocated. A new path lets sources migrate one at a time with the old one still serving whatever hasn't moved. Canvas migrates too, so the old path retires rather than living on.The part that needs genuinely new code is scoping, and it is the risky part.
BaseWarehouseETLTask(from mitodl/mit-learn#3566) has two modes and neither fits:full_refresh=True→ prunes; "the only mode that ever sees deletes/unpublishes upstream"full_refresh=False→ never prunes; "a partial pull must never be treated as the complete state of the source"A per-course trigger is neither. Within one course it is the complete state — a removed file must be unpublished — but globally it is not, and pruning globally from a single-course pull would unpublish the entire corpus. §4 specifies a third mode whose prune predicate is scoped to the same key as the fetch, plus a published-row-count guard so a short read skips pruning instead of mass-unpublishing. This is the same full-sync hazard as
MIN_PODCASTS/MIN_EPISODESin #2579, one level down.§5 records a constraint that has already caused an outage.
openedx.py:196documents it: per-partitionAutomationConditionevaluation on a 3,500-partition asset produced O(N²) load against the LMS and saturated the run queue until no export ran at all. The spec turns that into a rule — per-partition conditions only where evaluating them is per-partition and cheap; otherwise one sweeping sensor batching into a single N-element POST. Hence the payload is a list even for one course.§8 flags a gap independent of the decision: there is no OCW content-file sender anywhere in
dg_projectsand no OCW dg project. OCW's only Learn-facing model is the course-levelintegrations__learn__ocw_courses. Whatever is chosen above, OCW needs a new sending asset built.How can this be tested?
Not applicable — documentation only, no code paths.
pre-commit run --files docs/design/contentfile_scoped_pull_contract.mdpasses: trailing-whitespace, end-of-files, added-large-files, merge-conflicts and detect-secrets all run on it and pass.Every code reference in the doc was read from source at
origin/mainon 2026-08-19 rather than recalled: the three sender call sites and theirpartitions_defs,learn_api.py:55,webhooks/urls.py,webhooks/views.py:162/:179, theBaseWarehouseETLTaskdocstring, and theopenedx.py:196note quoted verbatim in §5.Additional Context
Decisions this encodes, made by @tmacey: Option C (new endpoint, scoped pull), Canvas migrates with the others, and cadence is set per asset via automation conditions / freshness policies rather than a fixed cron.
Three questions are deliberately left open at the end of the doc rather than settled unilaterally — abort-vs-skip-prune on count mismatch, a batch-size ceiling, and whether the expected count belongs in the task signature or is re-read from the view.
Neither warehouse view (
integrations__learn__ocw_content_files,integrations__learn__content_files) exists yet; §9 states the minimum columns they must expose.🤖 Generated with Claude Code
https://claude.ai/code/session_01VcWh3q3Bi2mxBmaiPCbBW9