feat(#2088): migrate marts__micromasters_dedp_exam_grades to dim_course_run

[quazi-h]

What are the relevant tickets?

Partially addresses #2088 (follow-up to #2319 — dim_course_run semester/passing_grade)

Description (What does it do?)

Migrates the MITxOnline branch of marts__micromasters_dedp_exam_grades to read semester and passing_grade from dim_course_run instead of re-deriving them through a duplicate join to MicroMasters staging inside int__mitxonline__proctored_exam_grades.

#2319 added those columns to dim_course_run but populated semester from micromasters_examruns only, dropping the courserun_tag fallback the old mart applied to course runs with an embedded proctored-exam unit — which is why a straight swap nulled 8,137 rows when Kate first tried it. dim_course_run now restores that fallback, gated on the same course-structure predicate the intermediate model used (a block titled 'proctored exam', read from its newest snapshot per block) so it only applies to runs that actually have an exam — 100 course runs, not all 4,513. passing_grade deliberately gets no fallback: the old model emitted null on that branch, so a bare er.examrun_passing_grade reproduces it. A new singular test asserts the dimension keeps covering every run the upstream reports with a semester.

Mart output is unchanged and the dimension changes in exactly one column, both reproduced in step 3. Expect a one-time SCD2 re-version of 87 dim_course_run rows on the first production run after merge.

How can this be tested?

Everything below runs on this PR's branch — the baseline side is read out of origin/main with git show, so there is no branch switching. Start from the repo root; step 1 cds into src/ol_dbt and steps 2 and 3 continue from there.

1. Register the sources and build both sides of the comparison.

# Skip if a worktree already holds this branch — cd into it instead.
gh pr checkout 2403

# The baseline is read from origin/main, so it has to be current.
git fetch origin main

# override_ref.sql resolves unchanged ancestors to production Glue views, so only these
# three layers need registering. Register immediately before building.
for db in staging intermediate dimensional; do
  uv run --frozen ol-dbt local register --database ol_warehouse_production_$db
done

# Pre-migration copy of each changed model, from origin/main. Absolute paths so they
# survive the cd below.
R=$(git rev-parse --show-toplevel)
MART=$R/src/ol_dbt/models/marts/micromasters/marts__micromasters_dedp_exam_grades
DIM=$R/src/ol_dbt/models/dimensional/dim_course_run
git show origin/main:src/ol_dbt/models/marts/micromasters/marts__micromasters_dedp_exam_grades.sql > ${MART}_pre.sql
git show origin/main:src/ol_dbt/models/dimensional/dim_course_run.sql > ${DIM}_pre.sql

# One invocation, so all four read identical sources. --full-refresh is required because
# dim_course_run is incremental and would otherwise merge without re-deriving semester.
cd "$R/src/ol_dbt"
uv run --frozen dbt run -t dev_local --full-refresh --select \
  dim_course_run dim_course_run_pre \
  marts__micromasters_dedp_exam_grades marts__micromasters_dedp_exam_grades_pre

PASS=5.

2. Test the two models that changed.

# buildable, not cautious (drops this PR's new test) and not eager (adds 8 relationships_*
# tests that report orphans by construction; one scans tfact_grade, 42.7M rows).
uv run --frozen dbt test -t dev_local --indirect-selection=buildable --select \
  dim_course_run marts__micromasters_dedp_exam_grades

PASS=13 WARN=1 ERROR=0, including assert_dim_course_run_semester_covers_proctored_exam_runs. The warning is not_null_dim_course_run_course_fk, pre-existing and identical on main.

3. Diff both changed models, old against new.

The mart must not change:

uv run --frozen ol-dbt diff \
  --old marts__micromasters_dedp_exam_grades_pre \
  --new marts__micromasters_dedp_exam_grades \
  -k user_micromasters_email,user_mitxonline_email,proctoredexamgrade_created_on

MATCH, 12 columns, 20867 / 20867 Δ=0, exit 0. Negative control: replace semester with a literal in ${MART}_pre.sql, rebuild that model, rerun — it reports mismatch.

dim_course_run must change, in one column only:

# effective_date/end_date are current_timestamp, so they differ on every build.
uv run --frozen ol-dbt diff --old dim_course_run_pre --new dim_course_run \
  -k courserun_pk --exclude-columns effective_date,end_date

MISMATCH, exit 1 — expected here. 8510 / 8510 Δ=0, one column moving: semester: 1.02% (87 rows), samples all None → '<tag>'. No passing_grade. Non-null semester 12 → 99.

Both diffs are needed: the mart diff proves nothing about dim_course_run, which has eight other downstream consumers.

rm ${MART}_pre.sql ${DIM}_pre.sql

Two known snags: ~/.ol-dbt/local.duckdb is shared across worktrees, so if ol-dbt diff fails with a DuckDB Conflicting lock error another session has it open — rerun when free. And a build failing on an HTTP 404 for a __dbt_tmp metadata path just means that layer's registration went stale; re-register it and rerun.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR updates the marts__micromasters_dedp_exam_grades mart so the MITxOnline branch sources semester and passing_grade from the dimensional model dim_course_run (intended to centralize and de-duplicate that derivation).

Changes:

• Add a dim_course_run-backed CTE for MITxOnline semester/passing_grade.
• Replace MITxOnline output columns to read from dim_course_run instead of the intermediate proctored-exam grades model.
• Keep the pure-MicroMasters branch unchanged (still sourcing those fields directly from the MicroMasters intermediate model).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[github-actions]

🔎 ol-dbt impact — column-level blast radius

✅ No column-level downstream impact detected for the changed models.

_{Posted by ol-dbt impact (annotate-only — does not block merge).}

[Copilot]

🟡 Changes recommended

The semester fallback currently populates every unmatched MITxOnline course run, contrary to the documented exam-only contract.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[quazi-h]

Reviewer input wanted: dim_course_run.semester semantics (f3909a6)

This PR changes what dim_course_run.semester means for MITxOnline, and I'd like a second opinion on the call rather than landing it quietly.

What changed. dim_course_run.sql now uses coalesce(er.examrun_semester, cr.courserun_tag). Measured on dev_local, that moves semester from 12 populated to 4,513 of 4,513 mitxonline is_current rows — 4,501 of them via the courserun_tag fallback.

Why it's needed. It isn't new behaviour, it's restored behaviour. int__mitxonline__proctored_exam_grades is a UNION whose second branch has derived courserun_tag as semester (line 66) since well before #2319. #2319 populated the dimension from MicroMasters exam runs only and dropped that fallback — which is why semester was ~99.7% NULL, and why migrating the mart onto it without this fix would have nulled out live values that users currently see.

What I corrected. _dim_course_run.yml documented the column as "Null for all other platforms and non-exam MITxOnline course runs". The second half is now false for 4,501 rows, so the description is rewritten to describe the actual derivation. Worth knowing that description was added in ac99b174 — the same commit as the column — so it described that implementation rather than an agreed contract, and nothing enforces it (ol-dbt validate checks column presence and sync, never semantics).

The judgement call. Should semester mean "this course run's academic term" (broad, what it now does), or stay narrowed to "the MicroMasters proctored exam run's semester" (the old documented wording, restoring NULLs for non-exam runs)?

I went with broad, for three reasons: it reproduces the derivation users have actually been served all along; courserun_tag genuinely is the run's term designation; and narrowing it would need a subquery against exam-unit grades to reconstruct the old branch condition, adding complexity that benefits no current consumer.

Blast radius is small, and I verified it rather than assuming. marts__micromasters_dedp_exam_grades is the only model in the repo that selects dim_course_run.semester — 14 other models reference dim_course_run but never that column — and it reads just 84 distinct course runs. A whole-row EXCEPT ALL between the pre-migration and migrated mart returns 0 rows in both directions, so today's data is provably unchanged. The exposure is prospective: a future consumer of dim_course_run.semester will now see values where it previously saw NULL.

If you'd rather keep the column narrow, say so and I'll implement the restricted fallback instead — the mart's output is identical either way, so this is purely about what the dimension promises.

[Copilot]

🟡 Changes recommended

The regression test can miss absent dimension rows and allows up to ten affected runs without failing.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 4/5 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The fallback gate currently includes historical course-structure snapshots instead of only the latest structure.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/ol_dbt/models/dimensional/dim_course_run.sql:56

• int__mitxonline__course_structure retains every retrieval and marks only the newest snapshot with coursestructure_is_latest (see int__mitxonline__course_structure.sql:7-12,55-60). Without filtering here, a block that existed only in an old snapshot keeps this run in the fallback forever, so semester does not become null when the current structure stops reporting the proctored-exam unit as the new comments and regression test specify. Restrict the gate to the latest structure snapshot.

• Files reviewed: 4/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[quazi-h]

Re: Copilot's "fallback gate includes historical course-structure snapshots" — the diagnosis is correct and is fixed in a01101b. The recommended remedy was not, and I measured before applying it.

The diagnosis holds

int__mitxonline__course_structure retains every retrieval. The gate had no snapshot filter, so a run whose exam block was renamed or removed stayed in the fallback forever — which contradicts what the regression test claims to guard.

It also meant my own code comment was false: it said we reuse int__mitxonline__proctored_exam_grades's predicate, and we didn't.

The recommended fix would have caused a regression

coursestructure_is_latest keeps only blocks present in the newest whole-course retrieval. But the upstream doesn't use that flag. int__mitxonline__proctored_exam_grades reaches course structure via int__mitxonline__courserun_subsection_grades, which dedups to the newest snapshot per (course run, block):

row_number() over (partition by courserun_readable_id, coursestructure_block_id
                   order by coursestructure_retrieved_at desc) as row_num
...
where course_structure.row_num = 1

Those differ precisely for a block that has left the structure: per-block dedup still returns its most recent snapshot, is_latest excludes it. Measured on production against the 79 course runs that have proctored exam grades with a semester:

gate	runs matched	of the 79, missed
any snapshot (as shipped)	107	0
coursestructure_is_latest	105	1
newest snapshot per block (applied)	106	0

The run is_latest drops is course-v1:MITxT+14.310GZ+3T2024 — it has proctored exam grades and a semester the pre-migration mart populated. Filtering on is_latest would have nulled that value, broken migration parity, and failed the regression test added in this PR.

What landed

Mirrored the per-block dedup. The only run removed is course-v1:MITxT+14.73GZ+1T2025, which has no proctored exam grades, so no mart output changes — the migration parity result stands.

Verified against a locally built dim_course_run:

run	semester	expected
course-v1:MITxT+14.310GZ+3T2024	3T2024	retained ✓
course-v1:MITxT+14.73GZ+1T2025	NULL	removed ✓

assert_dim_course_run_semester_covers_proctored_exam_runs still passes. The code comment now explains why coursestructure_is_latest is deliberately not used, so the next reader doesn't re-apply it.

[Copilot]

🟡 Changes recommended

The regression test derives its expected rows through the same course-structure predicate, so it cannot detect the documented source-drift failure.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 4/5 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The current revision resolves the previously identified correctness gaps and preserves the mart’s established output semantics.

Review details

• Files reviewed: 4/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[quazi-h]

Out of scope for this PR, but directly relevant context — and it sharpens something I said earlier in this thread.

When I narrowed the gate in a01101b I noted the only run removed was course-v1:MITxT+14.73GZ+1T2025, "which has no proctored exam grades at all, so no mart output changes." That is accurate, and the parity result stands. But I went and checked why it has none, and the answer is a pre-existing bug this PR neither causes nor worsens:

That run has a block that used to be titled proctored exam and is now titled Final Exam, carrying 32 learners with graded attempts. Because int__mitxonline__proctored_exam_grades.exam_unit_grades keys on that exact title string, the rename silently evicted all of them:

check (production, 2026-09-16)	result
rows in int__mitxonline__proctored_exam_grades for that run	0
rows in marts__micromasters_dedp_exam_grades for that run	0
mart total	20,867

It is the only affected run today. 14.73 is a DEDP course, so this is probably a genuine reporting gap rather than an artifact — but whether Final Exam is that proctored exam, or the block was legitimately repurposed, is a question for the course team rather than something the warehouse can answer. Flagging rather than guessing.

Tracked as tk-dedp-exam-grades-silently-drops-a-course-run-whe-03c080, with the proposed drift guard: key on block ids that have ever carried the title, flag any that still have graded attempts but no longer carry it. It has to ship as warn initially, since the 14.73GZ case would make an error-severity version land red. It belongs on the intermediate model, not on dim_course_run — the dimension's semester is the symptom, the lost rows are the disease.

@KatelynGit flagging in case the 32 missing learners matter for DEDP reporting independently of this migration.

[KatelynGit]

These definitions are not great and clearly not written or reviewed by a human

[KatelynGit]

• I got one warning when running a build but otherwise it was clean:

[WARNING]: in test not_null_dim_course_run_course_fk (models/dimensional/_dim_course_run.yml)
[WARNING]: Got 626 results, configured to warn if != 0

• The definitions could be improved they are definitely not written by a human and don't make sense

• Was a data diff run against old and new?