Skip to content

Commit cbf37fd

Browse files
committed
fix(sleep): keep the gate non-empty when every task hashes into test
assign_splits promises "Guarantee val (the gate) is non-empty when we have >=2 real tasks", but _promote_one only pulls from train (to top up val) or from val (to top up train), never from test. When every real task's hash bucket lands in [val_cut, test_cut) both train and val start empty, so both guarantee calls have nothing to promote and silently no-op. run_sleep_cycle then finishes with gate_action='reject' and edits=0: no error, no warning, and holdout_leaked does not flag it either. For a small nightly batch any test_fraction above roughly 0.5 makes this a matter of when, not if. Fall back to test for the promotion only when the preferred source is empty, so the stability guarantee #235 settled is untouched in the normal case and hash-assigned test tasks are only reassigned in the degenerate one. Topping up train now prefers test over a single-task val, so filling train cannot re-empty the gate it just filled. Borrowing spends a held-out task, so it is logged the way consolidate.py already logs holdout_leaked. Tests cover the degenerate split for 2..10 real tasks, the warning, dream tasks in train not masking an empty gate, the normal path not borrowing, and the single-real-task guard.
1 parent 79124b3 commit cbf37fd

2 files changed

Lines changed: 119 additions & 7 deletions

File tree

‎skillopt_sleep/mine.py‎

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -320,13 +320,18 @@ def _stable_key(task: TaskRecord) -> tuple[int, str]:
320320
bucket = int(hashlib.sha256((str(seed) + task.id).encode()).hexdigest(), 16)
321321
return bucket, task.id
322322

323-
def _promote_one(*, to: str, from_splits: set[str]) -> None:
324-
"""Promote one real task using hash order; never demote hash-assigned test."""
323+
def _promote_one(*, to: str, from_splits: set[str]) -> bool:
324+
"""Promote one real task using hash order; never demote hash-assigned test.
325+
326+
Returns True when a task was promoted, so degenerate splits that had to
327+
borrow from ``test`` can be reported rather than failing silently.
328+
"""
325329
candidates = [t for t in real if t.split in from_splits]
326330
if not candidates:
327-
return
331+
return False
328332
candidates.sort(key=_stable_key)
329333
candidates[0].split = to
334+
return True
330335

331336
for t in real:
332337
bucket = _stable_key(t)[0] % 100
@@ -338,12 +343,42 @@ def _promote_one(*, to: str, from_splits: set[str]) -> None:
338343
t.split = "train"
339344

340345
# Guarantee val (the gate) is non-empty when we have >=2 real tasks.
341-
# Only promote from train so hash-assigned test tasks stay untouched.
346+
# Prefer train so hash-assigned test tasks stay untouched. When every real
347+
# task hashed into test there is nothing in train to promote, and the old
348+
# code silently no-opped: the cycle then rejected with edits=0, no error,
349+
# and holdout_leaked did not flag it either. Fall back to test in that
350+
# degenerate case only, and log it since it spends a held-out task.
351+
borrowed_from_test = False
342352
if len(real) >= 2 and not any(t.split == "val" for t in real):
343-
_promote_one(to="val", from_splits={"train"})
344-
# Guarantee a train pool exists when possible; never borrow from test.
353+
if not _promote_one(to="val", from_splits={"train"}):
354+
borrowed_from_test = _promote_one(to="val", from_splits={"test"})
355+
356+
# Guarantee a train pool exists when possible. Prefer val, but never empty
357+
# the gate to do it: if val holds a single task, take from test first and
358+
# only fall back to val when test is exhausted (the pre-existing behavior
359+
# for splits that have no test slice at all).
345360
if not any(t.split == "train" for t in tasks) and len(real) >= 2:
346-
_promote_one(to="train", from_splits={"val"})
361+
spare_val = sum(1 for t in real if t.split == "val") > 1
362+
if spare_val:
363+
_promote_one(to="train", from_splits={"val"})
364+
elif _promote_one(to="train", from_splits={"test"}):
365+
borrowed_from_test = True
366+
else:
367+
_promote_one(to="train", from_splits={"val"})
368+
369+
if borrowed_from_test:
370+
import logging
371+
372+
logging.getLogger("skillopt_sleep").warning(
373+
"assign_splits: all %d real tasks hashed into test "
374+
"(val_fraction=%.2f, test_fraction=%.2f, seed=%d); "
375+
"borrowed from test so the gate has a val slice. "
376+
"Lower test_fraction to stop spending held-out tasks.",
377+
len(real),
378+
val_fraction,
379+
test_fraction,
380+
seed,
381+
)
347382
return tasks
348383

349384

‎tests/test_split_hardening_2x3.py‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,83 @@ def test_hash_assigned_test_not_demoted_for_val_top_up(self):
117117
if t.id in test_ids:
118118
self.assertEqual(t.split, "test")
119119

120+
def test_val_guaranteed_when_every_real_task_hashes_into_test(self):
121+
"""Degenerate split: nothing in train/val to promote from.
122+
123+
With a high test_fraction a small nightly batch can hash entirely into
124+
test. Both guarantees then had nothing to promote and silently no-opped,
125+
so the cycle rejected with edits=0 and no error. The gate must still get
126+
a val slice, borrowed from test as a last resort.
127+
"""
128+
for n in range(2, 11):
129+
with self.subTest(real_tasks=n):
130+
tasks = assign_splits(
131+
[_task(f"t{i}", f"task {i}") for i in range(n)],
132+
val_fraction=0.10,
133+
test_fraction=0.80,
134+
seed=42,
135+
)
136+
splits = [t.split for t in tasks]
137+
self.assertIn("val", splits, "gate must not be left empty")
138+
self.assertIn("train", splits, "train pool must not be empty")
139+
140+
def test_degenerate_split_warns_that_test_was_spent(self):
141+
"""Borrowing from test is reported, not silent."""
142+
with self.assertLogs("skillopt_sleep", level="WARNING") as captured:
143+
assign_splits(
144+
[_task(f"t{i}", f"task {i}") for i in range(5)],
145+
val_fraction=0.10,
146+
test_fraction=0.80,
147+
seed=42,
148+
)
149+
self.assertTrue(
150+
any("hashed into test" in line for line in captured.output),
151+
f"expected a degenerate-split warning, got {captured.output}",
152+
)
153+
154+
def test_dream_train_does_not_mask_an_empty_gate(self):
155+
"""Dream tasks fill train, but they may never stand in for val."""
156+
real = [_task(f"t{i}", f"task {i}") for i in range(4)]
157+
dream = [_task("d0", "dream variant", origin="dream")]
158+
tasks = assign_splits(
159+
real + dream,
160+
val_fraction=0.10,
161+
test_fraction=0.80,
162+
seed=42,
163+
)
164+
val_rows = [t for t in tasks if t.split == "val"]
165+
self.assertTrue(val_rows, "gate must be filled from real tasks")
166+
for t in val_rows:
167+
self.assertNotEqual(t.origin, "dream", "val must stay real-only")
168+
for t in tasks:
169+
if t.origin == "dream":
170+
self.assertEqual(t.split, "train")
171+
172+
def test_normal_split_does_not_borrow_from_test(self):
173+
"""The healthy path keeps its hash-assigned test slice intact."""
174+
with mock.patch("logging.getLogger") as get_logger:
175+
tasks = assign_splits(
176+
[_task(f"t{i}", f"task {i}") for i in range(12)],
177+
val_fraction=0.34,
178+
test_fraction=0.10,
179+
seed=7,
180+
)
181+
get_logger.assert_not_called()
182+
splits = [t.split for t in tasks]
183+
self.assertIn("val", splits)
184+
self.assertIn("train", splits)
185+
186+
def test_single_real_task_is_left_alone(self):
187+
"""The >=2 guard still applies; one task cannot fill val and train."""
188+
tasks = assign_splits(
189+
[_task("only", "task")],
190+
val_fraction=0.10,
191+
test_fraction=0.80,
192+
seed=42,
193+
)
194+
self.assertEqual(len(tasks), 1)
195+
self.assertIn(tasks[0].split, {"train", "val", "test"})
196+
120197

121198
class Pass1ApproachCFractionBoundaries(unittest.TestCase):
122199
"""Pass 1 / approach C: reject invalid fraction knobs early."""

0 commit comments

Comments
 (0)