assign_splits's own non-empty-val guarantee doesn't fire when every real task hashes into test
Maintainers usually reply within 1 day
Nobody has claimed this yet.
Assessment
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Newbie friendliness
- 65/100
Research direction
Start in skillopt_sleep/mine.py at assign_splits and _promote_one, reproducing the issue with the cited val_fraction=0.10 and test_fraction=0.80 case. Read consolidate.py's holdout_leaked logging for the expected warning style. Done means a batch with empty train and val receives the intended non-empty-val handling without changing normal hash assignments, and the pathological case is observable.
Written by the indexing model from the issue text.
Description
assign_splits in skillopt_sleep/mine.py says right in its own comment "Guarantee val (the gate) is non-empty when we have >=2 real tasks", but there's a gap: _promote_one only pulls from train (to top up val) or from val (to top up train), never from test. If 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. A nightly batch mines N real tasks, all go to test, and run_sleep_cycle finishes with gate_action='reject', edits=0, no error, no warning, holdout_leaked doesn't flag it either.
Not contrived: for a small nightly batch (2-5 tasks is realistic for a solo user's session), any test_fraction above roughly 0.5 makes this a matter of when, not if. Confirmed on 79124b37: 5 tasks with val_fraction=0.10, test_fraction=0.80 all land in test, val and train both come back empty.
I think I see why it's shaped this way. #235 replaced the old unconditional real[-1].split = "val" fallback (append-order-unstable, could demote an already hash-assigned test task) with the current from-train/from-val-only _promote_one, to stop reassigning hash-assigned test tasks per that review. That fixed the instability and reopened this as a side effect.
An additive fix, only reach into test for the promotion when train and val are both empty, and log it the way consolidate.py already logs holdout_leaked, would keep the stability guarantee for the normal case and only touch the pathological one. Wanted to check that's the right shape before sending a PR, since it's touching the val/test tradeoff #235 just settled.
- Dominant language
- Python
- Stars
- 17.3k
- Forks
- 1.6k
- Avg merge
- 2d 23h
- Merged PRs (30d)
- 9
Getting set up
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from microsoft/SkillOpt
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
microsoft/SkillOpt#284 · 2 comments ·
Maintainers usually reply within 1 day
-
项目还在迭代嘛？Open
Difficulty 5/5 Over a week Newbie friendliness 10/100
Maintainers usually reply within 1 day
-
Difficulty 5/5 Over a week Newbie friendliness 42/100
Maintainers usually reply within 1 day
-
Difficulty 3/5 1-2 days Newbie friendliness 72/100
microsoft/SkillOpt#286 · 2 comments ·
Maintainers usually reply within 1 day
-
Difficulty 5/5 Over a week Newbie friendliness 28/100
Maintainers usually reply within 1 day
All issues in microsoft/SkillOpt
Similar issues
-
needs triage
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
Maintainers usually reply within 2 days
-
Difficulty 2/5 1-3 hours Newbie friendliness 82/100
openvinotoolkit/openvino_notebooks#3665 ·
Maintainers usually reply within 1 day
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 86/100
Maintainers usually reply within 1 day
-
docs
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
Maintainers usually reply within 1 day
-
benchmark-gap
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
Maintainers usually reply within 1 day