Skip to content

Commit 94f103e

Browse files
committed
fix(delphi): address Copilot review on D10-D12 stack (consensus blob keys, priority serialization, defensive fixes)
Verified triage of all 83 Copilot review threads on PRs #2564-#2573: 1 real blocker + 4 escalations confirmed by direct code verification; the rest nitpicks / already-fixed / intentional bug-mirrors. All fixes TDD RED->GREEN. - Consensus entry keys -> Clojure blob shape {tid, n-success, n-trials, p-success, p-test} (was Python-convention comment_id/n_success/...). Narrows the S1 deferral: these entries flow raw into result['consensus'], where server-helpers.ts:298-313 and client-report majorityStrict.jsx:23-27 pluck `tid` - the Python keys broke both consumers. Rep-comment entries keep comment_id until the deferred math-blob alignment PR. - DynamoDB writer read D11 consensus from a key to_dynamo_dict never emits (repness.consensus_comments) -> always wrote the empty default; D11 data never reached Delphi_PCAResults. Now reads top-level result['consensus']. The round-trip test had stubbed to_dynamo_dict with the writer's wrong nested shape - stub corrected to the real producer shape. - to_dynamo_dict comment priorities: int(value) -> Decimal-preserving. int() floors sub-1 priorities to 0 (real formula spans ~0.18-31.46 per D12.6), which the TS server's weighted routing reads as "no priority data". Harmless today (bug-mirror pins 49.0), landmine once #2571 resolves. Legacy writer path Decimal-wrapped too. - DynamoDB reader normalizes legacy list-shaped consensus to the dict shape (pre-D11 blobs). - mod_out truthiness -> `is not None` x2 in repness.py (numpy array/Index-safe). - _compute_comment_priorities fails closed (error log + empty dict) on PCA/columns desync instead of silently zip-truncating extremities. - bench_repness.py imported the 14a-deleted comment_stats (ImportError on any benchmark run); benchmark import test added. - Per-variant xfails replace blanket xfail(strict=False) on D11/D12 real-data tests, so the variants that match Clojure gate again. DISCOVERY: scoping the blanket unmasked two previously-invisible incremental divergences (bg2018-incremental, pakistan-incremental consensus vs Clojure) that the blanket had silently absorbed - documented as known-bad incremental xfails, same family as biodiversity-incremental, deferred to the sequential-parity work. 3 pre-existing CCR failures (verified identical on edge 722640e) marked with precise per-variant reasons. PGR regression tests skip with the 2026-06-11 goldens-deferral reason - the mark S3-5 claimed to add but never committed. - test_repness_smoke consensus-entry structure assertion updated to the Clojure key shape (exact key-set check). - ns docstrings corrected (ns includes PASS post ns-PASS fix; pa+pd <= 1 reasoning in select_consensus_comments_df). - PERF deferral comment on the _compute_group_votes scan in _compute_comment_priorities (follow-up issue to be filed). Baseline (2026-07-04, stack top, --include-local): 13 failed / 476 passed / 18 skipped / 143 xfailed. All 13 accounted for: 10 stale-PGR comparisons (now skipped per deferral), 3 pre-existing CCR (now precise xfails). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013CiESAwcrZ6BcVfdkumnxa commit-id:4cc93a23
1 parent a8b18f0 commit 94f103e

13 files changed

Lines changed: 460 additions & 114 deletions

delphi/polismath/benchmarks/bench_repness.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
from polismath.conversation import Conversation
2929
from polismath.pca_kmeans_rep.repness import (
3030
conv_repness,
31-
comment_stats,
3231
compute_group_comment_stats_df,
3332
select_rep_comments_df,
3433
select_consensus_comments_df,

delphi/polismath/conversation/conversation.py

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,13 +1132,34 @@ def _compute_comment_priorities(self) -> Dict[Any, float]:
11321132
cmnt_proj = pca_project_cmnts(center, comps)
11331133
extremity_arr = compute_comment_extremity(cmnt_proj)
11341134

1135+
# Fail closed on desync: if the PCA vectors were computed on a
1136+
# different column set than the current rating_mat (e.g. moderation
1137+
# changed between recomputes), zip() would silently truncate and
1138+
# assign E=0 to the overflow tids — wrong priorities with no
1139+
# signal. Empty priorities degrade the TS server to uniform
1140+
# routing, which is honest; silently wrong extremities are not.
1141+
# (Copilot review 2026-07-04, g4.)
1142+
n_cols = len(self.rating_mat.columns)
1143+
if len(extremity_arr) != n_cols:
1144+
logger.error(
1145+
f"comment_priorities: extremity length {len(extremity_arr)} "
1146+
f"!= rating_mat column count {n_cols} (stale PCA?); "
1147+
f"skipping priorities for this tick")
1148+
self.comment_priorities = {}
1149+
return self.comment_priorities
1150+
11351151
# Column order of `center`/`comps`/`extremity_arr` matches
11361152
# `self.rating_mat.columns` (PCA is computed on rating_mat).
11371153
tid_extremity = dict(zip(self.rating_mat.columns, extremity_arr))
11381154

11391155
# Per-group A/D/S aggregation. `_compute_group_votes` returns
11401156
# {str(gid): {'n-members': N, 'votes': {tid: {A, D, S}}}}. S includes
11411157
# PASS (line ~1222: `np.sum(~np.isnan(votes))`), matching Clojure.
1158+
# PERF (deferred, Copilot on PR #2568): this is an O(groups ×
1159+
# comments × members) scan on every recompute; vectorize or reuse
1160+
# the repness-stage aggregation — tracked in the follow-up issue
1161+
# "delphi: _compute_comment_priorities recomputes group votes on
1162+
# every tick".
11421163
group_votes = self._compute_group_votes()
11431164

11441165
priorities: Dict[Any, float] = {}
@@ -2454,10 +2475,18 @@ def float_to_decimal(obj):
24542475
logger.info(f"[{time.time() - start_time:.2f}s] Processing comment priorities...")
24552476
priorities = {}
24562477
for cid, priority in self.comment_priorities.items():
2478+
# Preserve the float VALUE as Decimal (boto3 rejects raw
2479+
# floats). The previous int() truncation was harmless while
2480+
# the D12.6 bug-mirror pins every priority to 49.0, but the
2481+
# real formula (restored when issue #2571 resolves) spans
2482+
# ~0.18–31.46 on real data: int() floors sub-1 priorities
2483+
# to 0, which the TS server's weighted routing treats as
2484+
# "no priority data" — those comments would never be routed.
2485+
value = float_to_decimal(float(priority))
24572486
try:
2458-
priorities[int(cid)] = int(priority)
2487+
priorities[int(cid)] = value
24592488
except (ValueError, TypeError):
2460-
priorities[cid] = int(priority)
2489+
priorities[cid] = value
24612490
result['comment_priorities'] = priorities
24622491

24632492
# Process repness data efficiently

delphi/polismath/database/dynamodb.py

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -300,12 +300,16 @@ def write_conversation(self, conv) -> bool:
300300
if analysis_table:
301301
if dynamo_data:
302302
# Use pre-formatted data
303-
# D11 cascade fix (Investigation B, Site 1): source the FULL
304-
# consensus dict from repness.consensus_comments, preserving
305-
# both `agree` and `disagree` lists. The old code dropped
306-
# `disagree` entirely (`.get('consensus', {}).get('agree', [])`).
307-
consensus_comments = dynamo_data.get('repness', {}).get(
308-
'consensus_comments', {'agree': [], 'disagree': []}
303+
# D11 cascade fix (Investigation B, Site 1), corrected
304+
# 2026-07-04: `to_dynamo_dict()` surfaces consensus at
305+
# TOP-LEVEL `result['consensus']` — its `repness` dict
306+
# carries only `comment_repness`. The previous read of
307+
# `repness.consensus_comments` matched a key that never
308+
# exists, so the writer always stored the empty default
309+
# (the round-trip test masked this by stubbing
310+
# to_dynamo_dict with the wrong nested shape).
311+
consensus_comments = dynamo_data.get(
312+
'consensus', {'agree': [], 'disagree': []}
309313
)
310314
analysis_table.put_item(Item={
311315
'zid': zid,
@@ -450,7 +454,13 @@ def write_conversation(self, conv) -> bool:
450454
batch.put_item(Item={
451455
'zid_tick': zid_tick,
452456
'comment_id': str(comment_id),
453-
'priority': comment_priorities.get(comment_id, 0),
457+
# Legacy branch reads conv.comment_priorities
458+
# directly (raw floats) — convert like the
459+
# stats/consensus_score fields above, or
460+
# boto3 rejects the write (Copilot
461+
# 2026-07-04, e).
462+
'priority': self._replace_floats_with_decimals(
463+
comment_priorities.get(comment_id, 0)),
454464
'stats': stats,
455465
'consensus_score': consensus_score,
456466
'zid': zid,
@@ -861,9 +871,16 @@ def read_math_by_tick(self, zid: str, math_tick: int) -> Dict[str, Any]:
861871
# new dict shape `{'agree': [], 'disagree': []}` rather than
862872
# the obsolete empty list `[]`, so downstream consumers
863873
# always receive a uniformly-shaped value.
864-
result['consensus'] = analysis.get(
874+
stored_consensus = analysis.get(
865875
'consensus_comments', {'agree': [], 'disagree': []}
866876
)
877+
# Normalize legacy blobs (Copilot 2026-07-04, g3):
878+
# pre-D11 writers stored consensus as a (hardcoded-empty)
879+
# LIST. Map any list to the empty dict shape so readers
880+
# of old ticks never see a list.
881+
if isinstance(stored_consensus, list):
882+
stored_consensus = {'agree': [], 'disagree': []}
883+
result['consensus'] = stored_consensus
867884

868885
# 2. Get groups data
869886
groups_table = self.tables.get('Delphi_KMeansClusters')

delphi/polismath/pca_kmeans_rep/repness.py

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,12 @@ def prop_test(succ, n):
9191
9292
Args:
9393
succ: Series of success counts (e.g. `na` or `nd` per row).
94-
n: Series of trial counts. In all current callers this is `ns = na + nd`
95-
(AGREE + DISAGREE per row) — PASS votes are NOT included, matching
96-
what Clojure passes as `n-trials`. If you call this from elsewhere,
97-
supply `na + nd` rather than a "total votes seen including pass" count.
94+
n: Series of trial counts. In all current callers this is `ns` = the
95+
count of ALL non-nil votes INCLUDING PASS (`notna().sum()`),
96+
matching Clojure's `count-votes` with no vote arg
97+
(`(count (filter identity votes))` — 0/PASS is truthy in Clojure,
98+
repness.clj:56-61; ns-PASS fix 2026-06-11). If you call this from
99+
elsewhere, supply the PASS-inclusive non-nil count, NOT `na + nd`.
98100
99101
Returns:
100102
Series of z-scores. Positive when the smoothed proportion (succ+1)/(n+1)
@@ -519,7 +521,9 @@ def select_rep_comments_df(stats_df: pd.DataFrame,
519521
if stats_df.empty:
520522
return empty_df, None
521523

522-
mod_out_set = set(mod_out) if mod_out else set()
524+
# `is not None`, not truthiness: mod_out may be a numpy array / pandas
525+
# Index, whose bare truth value raises for len>1 (Copilot 2026-07-04).
526+
mod_out_set = set(mod_out) if mod_out is not None else set()
523527
sufficient: List[Dict[str, Any]] = []
524528
best: Optional[Dict[str, Any]] = None
525529
# Track best's max(rat, rdt) as a sidecar scalar so we never have to mutate
@@ -685,7 +689,9 @@ def consensus_stats_df(vote_matrix_df: pd.DataFrame,
685689
df['pat'] = prop_test_vectorized(df['na'], df['ns'])
686690
df['pdt'] = prop_test_vectorized(df['nd'], df['ns'])
687691

688-
if mod_out:
692+
# `is not None`, not truthiness: mod_out may be a numpy array / pandas
693+
# Index, whose bare truth value raises for len>1 (Copilot 2026-07-04).
694+
if mod_out is not None:
689695
mod_out_set = set(mod_out)
690696
df = df[~df.index.isin(mod_out_set)]
691697

@@ -706,19 +712,25 @@ def select_consensus_comments_df(
706712
- Agree: `pa > 0.5 AND z-sig-90(pat)`, sorted desc by `am = pa * pat`.
707713
- Disagree: `pd > 0.5 AND z-sig-90(pdt)`, sorted desc by `dm = pd * pdt`.
708714
709-
With PSEUDO_COUNT smoothing the constraint `pa + pd = 1` is exact (na+nd=ns
710-
after smoothing), so `pa > 0.5 ⟺ pd < 0.5` — the same tid cannot appear in
711-
both lists.
715+
Since `ns` counts all non-nil votes including PASS (ns ≥ na+nd),
716+
`pa + pd = (na+nd+PSEUDO_COUNT)/(ns+PSEUDO_COUNT) ≤ 1`, so pa and pd
717+
cannot both exceed 0.5 — the same tid cannot appear in both lists. (The
718+
equality pa+pd=1 holds only for PASS-free comments.)
712719
713720
Args:
714721
cons_stats: DataFrame indexed by tid with cols [na, nd, ns, pa, pd,
715722
pat, pdt], as produced by `consensus_stats_df`.
716723
717724
Returns:
718725
Dict shape `{'agree': [entries], 'disagree': [entries]}`. Each entry
719-
is `{comment_id, n_success, n_trials, p_success, p_test}` (Python
720-
convention key naming per S1; math-blob alignment with Clojure's
721-
hyphenated keys is a future PR).
726+
is `{tid, n-success, n-trials, p-success, p-test}` — EXACTLY the
727+
Clojure blob shape (repness.clj:181 + the ::consensus s/keys spec).
728+
This narrows the S1 deferral (2026-07-04): consensus entries flow
729+
raw into `result['consensus']` in to_dict / to_dynamo_dict, where
730+
server-helpers.ts:298-313 and client-report's
731+
majorityStrict.jsx:23-27 pluck `tid` — Python-convention keys broke
732+
both. Rep-comment entries keep `comment_id` until the deferred
733+
math-blob alignment PR.
722734
"""
723735
if cons_stats.empty:
724736
return {'agree': [], 'disagree': []}
@@ -735,20 +747,20 @@ def select_consensus_comments_df(
735747

736748
def _agree_entry(tid: Any, row: pd.Series) -> Dict[str, Any]:
737749
return {
738-
'comment_id': int(tid),
739-
'n_success': int(row['na']),
740-
'n_trials': int(row['ns']),
741-
'p_success': float(row['pa']),
742-
'p_test': float(row['pat']),
750+
'tid': int(tid),
751+
'n-success': int(row['na']),
752+
'n-trials': int(row['ns']),
753+
'p-success': float(row['pa']),
754+
'p-test': float(row['pat']),
743755
}
744756

745757
def _disagree_entry(tid: Any, row: pd.Series) -> Dict[str, Any]:
746758
return {
747-
'comment_id': int(tid),
748-
'n_success': int(row['nd']),
749-
'n_trials': int(row['ns']),
750-
'p_success': float(row['pd']),
751-
'p_test': float(row['pdt']),
759+
'tid': int(tid),
760+
'n-success': int(row['nd']),
761+
'n-trials': int(row['ns']),
762+
'p-success': float(row['pd']),
763+
'p-test': float(row['pdt']),
752764
}
753765

754766
return {
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
"""Benchmarks must stay importable as the production API evolves.
2+
3+
PR 14a deleted the scalar repness functions; `bench_repness.py` still
4+
imported `comment_stats`, so running any benchmark in that module crashed
5+
with ImportError. Plain import tests catch this class of drift at CI time
6+
(Copilot review 2026-07-04, g2).
7+
"""
8+
9+
import importlib
10+
11+
import pytest
12+
13+
14+
@pytest.mark.parametrize('module_name', [
15+
'polismath.benchmarks.bench_repness',
16+
'polismath.benchmarks.bench_pca',
17+
'polismath.benchmarks.bench_update_votes',
18+
'polismath.benchmarks.benchmark_utils',
19+
])
20+
def test_benchmark_module_imports(module_name):
21+
importlib.import_module(module_name)

0 commit comments

Comments
 (0)