Skip to content

fix(sleep): keep train and val splits non-empty - #272

Open
kaluli123123 wants to merge 1 commit into
microsoft:mainfrom
kaluli123123:fix/271-all-test-split-fallback
Open

fix(sleep): keep train and val splits non-empty#272
kaluli123123 wants to merge 1 commit into
microsoft:mainfrom
kaluli123123:fix/271-all-test-split-fallback

Conversation

@kaluli123123

Copy link
Copy Markdown

Summary

  • Preserve hash-assigned test tasks whenever the non-test pool can supply distinct train and validation tasks.
  • When a small batch cannot supply both pools, deterministically reassign the minimum number of test tasks and emit a warning with each affected task ID and destination split.
  • Cover both the reported all-test case and the adjacent val-plus-test/no-train case so the fallback cannot empty validation while creating train.

Why

assign_splits promised a non-empty validation gate for at least two real tasks, but an all-test hash assignment left both train and validation empty. A val-plus-test assignment had the same invariant gap: topping up train from the only validation task emptied validation.

The fallback now prefers spare train or validation tasks and reaches into test only when preserving both required pools is otherwise impossible.

Fixes #271

Validation

  • uv run --extra dev pytest -q tests/test_split_hardening_2x3.py tests/test_split_wiring.py — 32 passed.
  • Exhaustive local matrix over 6,300 combinations of seed, task count, split fractions, and input order — all invariants passed.
  • PATH=/Users/vam/.local/bin:/usr/bin:/bin:/usr/sbin:/sbin uv run --extra dev pytest -q — 1,499 passed, 11 skipped, 353 subtests passed.
  • uv run --extra dev ruff check skillopt_sleep/mine.py tests/test_split_hardening_2x3.py — passed.
  • git diff --check — passed.

Baseline notes

With the normal host PATH, the full suite reports one unrelated failure because the installed Pi CLI resolves to /opt/homebrew/bin/pi while tests/test_backend_pi.py::test_default_model_from_env expects the literal string pi. The same failure reproduces on pristine upstream main; excluding the installed Pi CLI from PATH gives the clean full-suite result above. Full-repository Ruff also reports pre-existing lint findings outside these two files; both changed files are clean.

@kaluli123123

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@Yif-Yang

Copy link
Copy Markdown
Contributor

Reviewed 58522d7fcadf. This is a useful, bounded correctness fix: it handles both the all-test assignment and the adjacent single-val/rest-test case without consuming the only validation task to populate train.

Local Linux/Python 3.11 verification: 34 passed, comprising the split-hardening/wiring suites and two independent all-test / val-plus-test invariant checks. I have not reproduced an additional runtime defect in this focused review.

For a reproducible acceptance record, please attach the command or small script used for the reported 6,300-case invariant sweep, with its seed/count/fraction/order ranges. That is an evidence request, not a request to add a large framework or perform paid-model experiments. Preserve the documented minimum-reassignment rule, warnings identifying the affected task IDs, and the distinction between stable hash assignment and the small-batch fallback.

Please keep this as one coherent solution to #271 rather than stacking overlapping split fixes. Official CI for this exact head is still action_required and awaits upstream approval; that is a maintainer-side gate, not a failed author test. This comment is a focused review receipt, not merge approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

assign_splits's own non-empty-val guarantee doesn't fire when every real task hashes into test

2 participants