Skip to content

Commit 40cccfd

Browse files
jucorjucor
authored
Fix stale group-k-smoother crash on large conversations (#2536)
* Fix stale group-k-smoother crash on large conversations Clamp smoothed-k to available group-clusterings keys when the base-cluster count shrinks between iterations, preventing a nil group-clusters lookup. Add an informative error in conv-repness for nil/empty group-clusters (instead of the cryptic 'Don't know how to create ISeq from transducer'). Add test coverage for both issues on synthetic data. * Address Copilot review: rename shadowed binding, clean up test names, revert unrelated package-lock changes
1 parent 9c506b2 commit 40cccfd

4 files changed

Lines changed: 147 additions & 2 deletions

File tree

‎math/src/polismath/math/conversation.clj‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -465,10 +465,17 @@
465465
; if seen > buffer many times, switch, OW, take last smoothed
466466
smoothed-k (if (>= this-k-count count-buffer)
467467
this-k
468-
(if smoothed-k smoothed-k this-k))]
468+
(if smoothed-k smoothed-k this-k))
469+
; Clamp: if smoothed-k no longer exists in this iteration's
470+
; clusterings (e.g. base-cluster count shrank), fall back to
471+
; the best available k by silhouette.
472+
clamped-smoothed-k
473+
(if (contains? group-clusterings smoothed-k)
474+
smoothed-k
475+
this-k)]
469476
{:last-k this-k
470477
:last-k-count this-k-count
471-
:smoothed-k smoothed-k}))
478+
:smoothed-k clamped-smoothed-k}))
472479

473480
; Pick the cluster corresponding to smoothed K value from group-k-smoother
474481
:group-clusters

‎math/src/polismath/math/repness.clj‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,9 @@
109109
n=#, p=prob, r=rep, t=test, a=agree, d=disagree, s=seen
110110
The :ids key maps to a vector of group ids in the same order they appear in the :stats sequence."
111111
[data group-clusters base-clusters]
112+
(when (empty? group-clusters)
113+
(throw (IllegalArgumentException.
114+
"conv-repness: group-clusters is nil or empty — investigate upstream why group-clusters is nil.")))
112115
{:ids (map :id group-clusters)
113116
:tids (nm/colnames data)
114117
:stats

‎math/test/conv_edge_cases_test.clj‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
;; Copyright (C) 2012-present, The Authors. This program is free software: you can redistribute it and/or modify it under the terms of the GNU Affero General Public License, version 3, as published by the Free Software Foundation. This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more details. You should have received a copy of the GNU Affero General Public License along with this program. If not, see <http://www.gnu.org/licenses/>.
2+
3+
(ns conv-edge-cases-test
4+
"Tests for edge cases in the conversation update pipeline that can cause
5+
crashes on large or evolving conversations."
6+
(:require [clojure.test :refer [deftest testing is]]
7+
[polismath.math.repness :as repness]
8+
[polismath.math.named-matrix :as nm]
9+
[polismath.math.conversation :as conversation]))
10+
11+
12+
;; ============================================================================
13+
;; Bug 1: conv-repness crashes with empty group-clusters
14+
;;
15+
;; When group-clusters is nil or empty, (apply map f []) returns a transducer
16+
;; (function object) instead of a sequence. Downstream code that tries to
17+
;; iterate over :stats gets:
18+
;; IllegalArgumentException: Don't know how to create ISeq from: clojure.core$map$fn__XXXX
19+
;;
20+
;; This happens in production when the k-smoother holds a stale smoothed-k
21+
;; that no longer exists in group-clusterings (see Bug 2 below).
22+
;; ============================================================================
23+
24+
(def test-rating-mat
25+
(nm/named-matrix
26+
[:p1 :p2 :p3 :p4]
27+
[:c1 :c2 :c3]
28+
[[-1 1 0]
29+
[ 1 -1 1]
30+
[-1 -1 0]
31+
[ 1 1 -1]]))
32+
33+
(def test-base-clusters
34+
[{:id :b1 :members [:p1 :p2]}
35+
{:id :b2 :members [:p3 :p4]}])
36+
37+
38+
(deftest conv-repness-with-empty-group-clusters
39+
(testing "conv-repness throws an informative error when group-clusters is empty"
40+
;; Without the guard, (apply map f []) silently returns a transducer that
41+
;; crashes downstream with a cryptic "Don't know how to create ISeq" error.
42+
;; The guard throws immediately with a message pointing to the root cause.
43+
(is (thrown-with-msg? IllegalArgumentException
44+
#"group-clusters is nil or empty"
45+
(repness/conv-repness test-rating-mat [] test-base-clusters))))
46+
47+
(testing "conv-repness throws an informative error when group-clusters is nil"
48+
(is (thrown-with-msg? IllegalArgumentException
49+
#"group-clusters is nil or empty"
50+
(repness/conv-repness test-rating-mat nil test-base-clusters)))))
51+
52+
53+
;; ============================================================================
54+
;; Bug 2: stale smoothed-k in group-k-smoother
55+
;;
56+
;; The k-smoother preserves the old smoothed-k for :group-k-buffer iterations
57+
;; to dampen oscillation. But it doesn't check whether the old k still exists
58+
;; in the current group-clusterings. When the number of non-empty base-clusters
59+
;; drops (e.g. due to PCA rotation after a batch of new votes), the k-range
60+
;; shrinks and (get group-clusterings old-smoothed-k) returns nil.
61+
;;
62+
;; We test this by feeding the smoother a conv with a previous smoothed-k=5
63+
;; and group-clusterings that only contain k=2,3.
64+
;; ============================================================================
65+
66+
(deftest stale-smoothed-k-is-clamped-to-available-group-clusters
67+
(testing "smoothed-k is clamped to an available range so group-clusters stays non-nil"
68+
;; Simulate: previous iteration had smoothed-k=5, but current base-cluster
69+
;; count only supports k=2,3
70+
(let [;; Minimal group clusterings for k=2 and k=3
71+
dummy-clustering-k2 [{:id 0 :members [:b1]} {:id 1 :members [:b2]}]
72+
dummy-clustering-k3 [{:id 0 :members [:b1]} {:id 1 :members [:b2]} {:id 2 :members []}]
73+
group-clusterings {2 dummy-clustering-k2
74+
3 dummy-clustering-k3}
75+
;; Old smoother state with smoothed-k=5 (stale!)
76+
old-smoother {:last-k 5 :last-k-count 1 :smoothed-k 5}
77+
;; Current smoother picks the best available k, but preserves old smoothed-k
78+
;; because buffer hasn't been exceeded
79+
group-clusterings-silhouettes {2 0.6, 3 0.8}
80+
smoother-fnk (:group-k-smoother conversation/small-conv-update-graph)
81+
new-smoother (smoother-fnk
82+
{:conv {:group-k-smoother old-smoother}
83+
:group-clusterings group-clusterings
84+
:group-clusterings-silhouettes group-clusterings-silhouettes
85+
:opts' {:group-k-buffer 4}})
86+
;; Now look up group-clusters using the smoother's smoothed-k
87+
group-clusters (get group-clusterings (:smoothed-k new-smoother))]
88+
89+
;; With the current (unfixed) code, smoothed-k stays at 5 and the lookup returns nil.
90+
;; After the fix, smoothed-k should be clamped to an available k.
91+
(testing "smoothed-k should be a key that exists in group-clusterings"
92+
(is (contains? group-clusterings (:smoothed-k new-smoother))
93+
(str "smoothed-k=" (:smoothed-k new-smoother)
94+
" not in " (keys group-clusterings))))
95+
96+
(testing "group-clusters should not be nil"
97+
(is (some? group-clusters)
98+
"group-clusters lookup must not return nil")))))
99+
100+
101+
;; ============================================================================
102+
;; Bug 3 (colleague's fix): agg-bucket-votes-for-tid with unknown pids
103+
;;
104+
;; When base-cluster members include pids not present in the rating matrix
105+
;; (e.g. after incremental updates where clusters lag behind the matrix),
106+
;; the pid-to-row lookup returns nil, and (get person-rows nil) can fail
107+
;; depending on the matrix implementation.
108+
;; ============================================================================
109+
110+
(deftest agg-bucket-votes-unknown-pid
111+
(testing "agg-bucket-votes-for-tid handles pids not in the rating matrix"
112+
(let [;; rating-mat only has :p1 and :p2
113+
rating-mat (nm/named-matrix
114+
[:p1 :p2]
115+
[:c1 :c2]
116+
[[-1 1]
117+
[ 1 0]])
118+
;; but bid-to-pid references :p3 which doesn't exist in rating-mat
119+
bid-to-pid [[:p1 :p3] [:p2]]
120+
result (conversation/agg-bucket-votes-for-tid
121+
bid-to-pid rating-mat number? :c1)]
122+
;; Should not throw; :p3's vote is nil, filtered out by number?
123+
(is (vector? result))
124+
;; bucket 0 has :p1 (voted) and :p3 (unknown → nil, filtered out) → count 1
125+
(is (= 1 (first result)))
126+
;; bucket 1 has :p2 (voted) → count 1
127+
(is (= 1 (second result))))))

‎math/test/test_runner.clj‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
(:require [cluster-tests]
55
[conv-man-tests]
66
[conversation-test]
7+
[conv-edge-cases-test]
78
[index-hash-test]
89
[named-matrix-test]
910
[pca-test]
@@ -14,6 +15,12 @@
1415
[clojure.test :as test]))
1516

1617

18+
;; Print each deftest name as it starts, so we can see progress and detect hangs.
19+
(defmethod test/report :begin-test-var [m]
20+
(let [v (:var m)]
21+
(println " " (-> v meta :name))
22+
(flush)))
23+
1724
(defn -main
1825
"Run all the pure tests for polisapp. The one integration test is in conv-man-tests, and should be run separately (and
1926
needs to be cleaned up to run on a separate poller system)"
@@ -22,6 +29,7 @@
2229
test/run-tests
2330
'[cluster-tests
2431
conversation-test
32+
conv-edge-cases-test
2533
index-hash-test
2634
named-matrix-test
2735
pca-test

0 commit comments

Comments
 (0)