fix: validate n >= 1 in the MMLU/GSM8K slice loaders (fixes #8)#12
Merged
Merged
Conversation
n=0 or n<0 previously yielded an empty slice that scored as a misleading 0% accuracy. Both loaders now raise a clear ValueError at the top of the function, before the cache check and before the gated datasets import, so the guard needs no download or network. Co-Authored-By: Kimi K3 <noreply@kimi.com>
Owner
|
Thanks Peter — verified locally: ruff, mypy, and the full metrics suite all green, and the guard fires before any cache/datasets access as your test asserts. The One small follow-up if you're up for it: an accept-path test (n=1 against a pre-seeded tmp cache_dir) would pin the happy path without needing network — nice complement to the reject cases here. Not blocking. Appreciate the fix! |
bamdadd
pushed a commit
that referenced
this pull request
Jul 17, 2026
Add accept-path tests for load_mmlu_slice / load_gsm8k_slice: seed a tmp cache_dir with the exact JSON file each loader looks for, call with n=1, and assert one item of the right dataclass with the promised field shapes. Network is blocked via a socket monkeypatch so a cache miss fails loudly rather than downloading; the cache hit means the gated datasets import is never reached. Complements the reject-path tests from #12. Follow-up to #12.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8.
Both
load_mmlu_sliceandload_gsm8k_slicenow raiseValueError("n must be >= 1, got {n}")at the very top — before_cache_path(...)(whichmkdirs the cache root) and before the function-localfrom datasets import load_dataset— so the guard is reachable with no download, no network, and no side effects, per the issue's ordering requirement.Tests: four parametrized cases in
tests/test_metrics.py(n=0andn=-3for each loader),pytest.raises(ValueError), no existing test touched.Verification
ruff check .,ruff format --check .,mypy src,pytest -qall green (56 tests, 2 pre-existing unrelated skips — torch/matplotlib extras).ModuleNotFoundError: No module named 'datasets'atmetrics.py:584, i.e. without the guard the call sails past the cache check into the gated import, which is precisely the no-network property the guard exists to protect. gsm8k tests unaffected by the mmlu mutation; restore → all green.One boundary note: the guards reject
n < 1but deliberately not non-int types (out of #8's stated scope), andscore_mmlu([], [])still returns0.0— the issue fixes the misleading-0% at the loader boundary, which is what this does.🤖 This PR was written with AI assistance (implementation: Kimi K3; verification and review: Claude), directed by a human-supervised workflow.