fix(strategy): reject a degenerate n in runner before building a Backend#276
Closed
statxc wants to merge 1 commit into
Closed
fix(strategy): reject a degenerate n in runner before building a Backend#276statxc wants to merge 1 commit into
statxc wants to merge 1 commit into
Conversation
matmul.runner.run rejects a non-positive / non-integer n up front, but strategy.runner.run and .compare did not: they constructed a Backend (demanding a real GPU) and only then failed inside storage -- ZeroDivisionError from the `// (n * 8)` block-size math for n=0, or numpy's negative-dimensions error for n<0. Mirror matmul's boundary check in both entry points. This check landed once in zeokin#268 and was lost when that PR was reverted (de2e149) to untangle a conflict with zeokin#232; zeokin#232 was re-landed as 4c44b45 but zeokin#268 never was, and no test covered the strategy side. Add tests/test_runner_n_parity.py: a parity table driving matmul.run, strategy.run and strategy.compare through the same boundary cases, so the two standalone packages cannot drift apart here again. A stub Backend that raises on construction proves validation happens before any device work, so it runs on CPU with no GPU.
Contributor
|
This PR currently has merge conflicts with the base branch. Please resolve them within 12 hours or the bot will close the PR automatically. |
Contributor
|
Closing this PR because the reported merge conflict was not resolved within 12 hours of the bot reminder. |
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.
PR kind
Summary
matmul.runner.runrejects a degeneratenbefore touching the GPU, butstrategy.runner.run/.comparedo not — they construct aBackend(demanding a real GPU) and only then fail deep insidestorage:n=0→ZeroDivisionErrorfrom the// (n * 8)block-size mathn<0→ numpynegative dimensions are not allowedReproduced on CPU (no GPU needed — the asymmetry is visible in which error you get):
Why it regressed (full transparency)
This check landed once in #268 and was lost when that PR was reverted in
de2e149to untangle a conflict with #232. #232's content was re-landed as4c44b45(expose validated spectral alpha), but #268 was never re-landed — and its strategy-side test went with it, so nothing caught the drop. Credit for the original fix belongs to #268; this restores the boundary and adds the coverage that would have caught the revert.Changes
strategy/runner.py: reject a non-positive / non-integerninrun()andcompare()beforeBackend(...), mirroringmatmul.runner.run's existing check (boolis excluded explicitly, sinceboolsubclassesint).tests/test_runner_n_parity.py(new): a parity table drivingmatmul.run,strategy.runandstrategy.comparethrough the same boundary cases, so the two standalone packages can't drift apart here again — plus ann=1off-by-one guard. A stubBackendthat raises on construction proves validation runs before any device work, so it runs on CPU.Touches only
strategy/andtests/— no protected paths.Validation
After the fix,
strategy.runner.run(0)/.compare(0)raiseValueError: n must be a positive integer, got 0with no GPU required.Checklist
Co-authored-byfooters for coding agents such as Cursor, Codex, Claude, Copilot, or similar tools.featPR — no scorecard required.