From 2cfa2c345fc74e5f414bccfdd2337e5be24b6d27 Mon Sep 17 00:00:00 2001 From: Julien Cornebise Date: Fri, 17 Jul 2026 00:58:32 +0100 Subject: [PATCH] fix(math): restore priority-based comment routing (regression from #1961) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `:comment-priorities` node passed the meta-tid lookup value `(if meta-tids (get meta-tids tid 0) 0)` into `priority-metric`'s `is-meta` argument. `(get meta-tids tid 0)` returns `0` for non-meta tids, and `0` is TRUTHY in Clojure, so `(if is-meta ...)` took the meta branch for EVERY comment — every priority collapsed to `meta-priority^2 = 49`. The TypeScript server's `selectProbabilistically` then degraded to uniform-random next-comment selection, reverting routing to its pre-2018 behavior. Introduced by #1961 (2025-03-15, "cutoff for large-convo processing if > 5000 comments"), which changed the meta-tid default from `(meta-tids tid)` (nil for non-meta) to `(get meta-tids tid 0)`. Fix: pass a real boolean — `(priority-metric (contains? meta-tids tid) ...)`. `contains?` is false for a non-meta tid and false when `meta-tids` is nil, restoring meta=49 / non-meta=importance*novelty. Verified end-to-end: a math image built with this fix regenerates the vw cold-start blob with varied priorities (125 distinct, 5.16-61.95) instead of all-49. Scope: Clojure only, which is what feeds production routing. The Python port still mirrors the bug (`priority_metric` returns META_PRIORITY**2). Un-mirroring it revealed that Python and Clojure priorities are not yet at parity (rank-uncorrelated on vw), so the Python un-mirror + cold-start blob regeneration is deferred to #2571, pending extremity/PCA parity (D1/D1b). See delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md (2026-07-17). Co-Authored-By: Claude Opus 4.8 commit-id:ec8e2093 --- delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md | 38 ++++++++++++++++++++++ delphi/docs/PLAN_DISCREPANCY_FIXES.md | 10 +++++- math/src/polismath/math/conversation.clj | 12 +++---- math/test/conv_edge_cases_test.clj | 41 ++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md b/delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md index 5ca54f840..49e6b3bd8 100644 --- a/delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md +++ b/delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md @@ -1913,3 +1913,41 @@ Verified: local full suite **403 passed / 0 failed** (baseline was 1 failed + _(Storage-v2 CI green-up — delphi_storage Dockerfile COPY, the postgres://→postgresql:// backend hardening, and the PG-conformance CI wiring — is tracked in `STORAGE_V2_IMPLEMENTATION_NOTES.md`, not here.)_ + +### Session: Clojure routing-bug (#1961) fix + D12 priority-parity discovery (2026-07-17) + +**Clojure comment-routing bug fixed** (`math/src/polismath/math/conversation.clj`, +`:comment-priorities`). The node passed `meta-tid-value = (if meta-tids (get meta-tids tid 0) 0)` +into `priority-metric`'s `is-meta` slot. `(get … 0)` returns `0` for non-meta tids, and **0 is +truthy in Clojure**, so `(if is-meta …)` took the meta branch for EVERY comment → all +priorities = `meta-priority^2 = 49` → the TypeScript server's `selectProbabilistically` +degraded to uniform-random routing (the pre-2018 behavior). Introduced by **#1961** +(2025-03-15, "cutoff for large-convo processing if > 5000 comments"). Fix: pass a real +boolean — `(priority-metric (contains? meta-tids tid) A P S extremity)` — `contains?` is +false for non-meta tids and safe when `meta-tids` is nil. Verified end-to-end: regenerating +the vw cold-start blob from the rebuilt (fixed) math image yields **varied** priorities +(125 distinct, 5.16–61.95) instead of all-49. + +**NEW — D12 comment-priorities are NOT actually at parity (discovered here).** The all-49 +bug was *masking* a real priority non-parity. With the Clojure bug fixed and the Python +`priority_metric` bug-mirror hypothetically un-mirrored (honoring `is_meta`), fixed-Python +and fixed-Clojure vw priorities are **rank-uncorrelated** (Spearman −0.03; top-10 comment +overlap 0/10; Python range 0.18–16.8, Clojure 5.16–61.95). While both sides returned the +constant 49, D12's parity assertion passed **trivially** (49 == 49). Likely contributors: +participant filtering (Clojure `in-conv` = 67 vs Python ~68–69), a vote-replay delta in the +cold-start generator (copies 4555 of vw's 4683 votes), and — most importantly — the still-open +**extremity/PCA parity gaps** (priority = importance × novelty × extremity²; extremity is the +L2 norm of the PCA comment projection, exactly what D1/D1b are still closing). + +**Decision (Julien, 2026-07-17): ship the Clojure fix ALONE.** Only Clojure's math blob feeds +production routing (via the TS server), so the Clojure fix restores correct routing on its own. +We do NOT un-mirror Python, do NOT regenerate the committed cold-start blobs (regenerating +flips them to varied and turns D12 parity legitimately RED — not achievable until extremity/PCA +parity lands), and keep the `priority_metric` bug-mirror in place. The Python un-mirror + blob +regen + true D12 value-parity is now **follow-up work under #2571, blocked on extremity/PCA +(D1/D1b) parity**. + +**What's next for D12:** (1) close extremity/PCA parity; (2) reconcile participant-filtering and +the generator's vote-copy delta so cold-start inputs match; (3) THEN un-mirror `priority_metric`, +regenerate cold-start blobs, and change D12's test from the trivial constant-49 check to a real +varied-value / rank-parity assertion. diff --git a/delphi/docs/PLAN_DISCREPANCY_FIXES.md b/delphi/docs/PLAN_DISCREPANCY_FIXES.md index 3105d02b4..449661cbf 100644 --- a/delphi/docs/PLAN_DISCREPANCY_FIXES.md +++ b/delphi/docs/PLAN_DISCREPANCY_FIXES.md @@ -393,6 +393,14 @@ This replaces the reading from the math blob with a proper Python computation ma 3. Implement: comment projection/extremity in PCA, `importance_metric`, `priority_metric`, full computation 4. Fix buggy `_compute_votes_base()` method +> **⚠️ Parity blocker (2026-07-17):** step 2's Spearman comparison does **not** pass yet. +> The Clojure `#1961` truthy-0 bug made every Clojure priority `49`, so any Python output +> "matched" trivially; the Python side currently *mirrors* that (`priority_metric` returns +> `META_PRIORITY**2`). Now that the Clojure bug is fixed, fixed-Python vs fixed-Clojure vw +> priorities are rank-**uncorrelated** (Spearman −0.03) — priority parity depends on the +> extremity/PCA parity (D1/D1b) closing first. Do not un-mirror `priority_metric` or +> regenerate cold-start blobs until then. See `CLJ-PARITY-FIXES-JOURNAL.md` 2026-07-17. + --- ### PR 12: Fix D15 — Moderation Handling @@ -533,7 +541,7 @@ See `delphi/docs/INVESTIGATION_K_DIVERGENCE.md` for the full investigation. | D9 | Z-score thresholds | **PR 3** | **#2518** | **DONE** ✓ | | D10 | Rep comment selection | PR 8 | **#2566** | Code-complete + 18 synthetic tests (was mislabeled "VM draft" until 2026-07-04); Copilot-review fixes in **#2586**; open in stack, merge pending edge freeze | | D11 | Consensus selection | PR 9 | **#2567** | Code-complete + 12 synthetic tests; consensus entries now Clojure blob shape (tid/n-success/… — #2586); open in stack, merge pending edge freeze | -| D12 | Comment priorities | PR 11 | **#2568** | Code-complete + 11 synthetic tests (bug-mirror per #2571); Decimal-preserving serialization (#2586); open in stack, merge pending edge freeze | +| D12 | Comment priorities | PR 11 | **#2568** | Code-complete + 11 synthetic tests (bug-mirror per #2571); Decimal-preserving serialization (#2586); open in stack, merge pending edge freeze. **⚠️ 2026-07-17: Python↔Clojure priority PARITY is NOT achieved.** The Clojure all-49 routing bug (#1961) was masking it — while both sides returned constant 49, the D12 test passed trivially. Clojure-side fix (`(contains? meta-tids tid)`) ships separately; with it, fixed-Python vs fixed-Clojure vw priorities are rank-**uncorrelated** (Spearman −0.03). Un-mirroring `priority_metric` + regenerating cold-start blobs is **BLOCKED on extremity/PCA parity (D1/D1b)**. See journal 2026-07-17. | | D13 | Subgroup clustering | — | — | **Deferred** (unused) | | D14 | Large conv optimization | — | — | **Deferred** (Python fast enough) | | D15 | Moderation handling | PR 12 | **#2523** | **DONE** ✓ (zero-out-columns + downstream `to_math_blob` / `_compute_vote_stats` regressions fixed 2026-06-09 — `to_dict` now routes through `_compute_user_vote_counts()` / `_compute_votes_base()`; `_compute_vote_stats` uses `_get_clean_matrix(raw=True)`) | diff --git a/math/src/polismath/math/conversation.clj b/math/src/polismath/math/conversation.clj index 8fa0799ec..abf19d412 100644 --- a/math/src/polismath/math/conversation.clj +++ b/math/src/polismath/math/conversation.clj @@ -670,12 +670,12 @@ extremity (or (get extremities tid) (do (log/warn "No extremity for tid" tid "zid" (:zid conv)) - 0)) - ;; Use 0 as the default when meta-tids is null or doesn't contain the tid - meta-tid-value (if meta-tids - (get meta-tids tid 0) - 0)] - (priority-metric meta-tid-value A P S extremity))) + 0))] + ;; Pass a real boolean. (get meta-tids tid 0) defaults non-meta + ;; tids to 0, which is TRUTHY in Clojure, so priority-metric took + ;; the meta branch for every comment (regression #1961). See #2571 + ;; and delphi/docs/MATH_ALGORITHM_HISTORY.md. + (priority-metric (contains? meta-tids tid) A P S extremity))) tids))) diff --git a/math/test/conv_edge_cases_test.clj b/math/test/conv_edge_cases_test.clj index 51bf8385f..8d92a0514 100644 --- a/math/test/conv_edge_cases_test.clj +++ b/math/test/conv_edge_cases_test.clj @@ -125,3 +125,44 @@ (is (= 1 (first result))) ;; bucket 1 has :p2 (voted) → count 1 (is (= 1 (second result)))))) + + +;; ============================================================================ +;; Bug 4: comment-priorities collapsed every comment to META_PRIORITY^2 (=49) +;; +;; #1961 (2025-03-15) changed the meta-tid lookup in the :comment-priorities fnk +;; from (meta-tids tid) to (get meta-tids tid 0). For a non-meta tid, +;; (get meta-tids tid 0) returns 0 — and 0 is TRUTHY in Clojure — so +;; priority-metric took the meta branch for EVERY comment, collapsing all +;; priorities to meta-priority^2 = 49 and degrading routing to uniform-random. +;; Fixed by passing a real boolean: (contains? meta-tids tid). See #2571. +;; ============================================================================ + +(deftest comment-priorities-only-meta-tids-get-meta-priority + (testing "only genuine meta tids get meta-priority^2; non-meta tids get varied importance-based priorities" + (let [priorities-fnk (:comment-priorities conversation/small-conv-update-graph) + tids [1 2 3] + meta-tids #{2} ; only tid 2 is a meta comment + group-votes {0 {:votes {1 {:A 5 :D 1 :S 8} + 2 {:A 3 :D 0 :S 6} + 3 {:A 1 :D 2 :S 7}}} + 1 {:votes {1 {:A 2 :D 1 :S 5} + 2 {:A 1 :D 1 :S 4} + 3 {:A 4 :D 0 :S 9}}}} + conv {:zid 1 :group-votes group-votes} + pca {:comment-extremity [0.5 1.2 0.8]} ; one per tid, in tids order + meta-priority-sq (double (* conversation/meta-priority conversation/meta-priority)) + priorities (priorities-fnk {:conv conv + :group-votes group-votes + :pca pca + :tids tids + :meta-tids meta-tids})] + (testing "the meta tid gets exactly meta-priority^2" + (is (== meta-priority-sq (double (get priorities 2))))) + ;; Regression guard for #1961: with the truthy-0 bug, non-meta tids also + ;; hit the meta branch and returned meta-priority^2. + (testing "non-meta tids do NOT get meta-priority^2" + (is (not (== meta-priority-sq (double (get priorities 1))))) + (is (not (== meta-priority-sq (double (get priorities 3)))))) + (testing "non-meta priorities are varied, not a single constant" + (is (not (== (double (get priorities 1)) (double (get priorities 3)))))))))