fix(docs): declare frac in strategy README's 'Register your own' basis() hook (Fixes #274)#300
Merged
Merged
Conversation
…s() hook Fixes zeokin#274. multiply_subspace forwards the run's vram_fraction only to a basis() that declares frac (an inspect.signature gate whose else-branch is a legacy shim), so a contributor who copies a snippet without it lands in the legacy branch and silently streams at the default fraction instead of --vram-fraction -- the bug zeokin#211 fixed for rsvd. CONTRIBUTING.md, the template and run_example.py were all brought in line, but the README's 'Register your own (the updatable hook)' -- the copy a contributor meets first -- still showed the old signature. It survived because the guard test added for exactly this never covered strategy/README.md: its docstring enumerated only 'the two remaining copies'. Fix the snippet and close the hole: the markdown copies now run through one parametrized table, so a rollback or a newly added doc cannot quietly skip the check. Also assert the FULL parameter list against Transform.basis (defaults stripped), not just the presence of frac, so a snippet cannot drift on any other argument either. Credit: zeokin#275 (luciferlive112116) reported the same gap; it was closed unmerged by the 12h merge-conflict rule after zeokin#253 landed the test file underneath it.
zeokin
approved these changes
Jul 17, 2026
zeokin
left a comment
Owner
There was a problem hiding this comment.
Fixes #274: strategy/README's 'Register your own' hook still showed basis() without frac, so a copied snippet hits multiply_subspace's legacy branch and silently ignores --vram-fraction. Also extends the drift test to cover the README (the hole that let it drift).
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
Fixes #274.
Summary
multiply_subspaceforwards the run'svram_fractiononly to abasis()that declaresfrac— aninspect.signaturegate whoseelsebranch is a legacy-compat shim. So a contributor who copies a snippet withoutfraclands in the legacy branch and silently streams their basis at the default fraction instead of--vram-fraction(the bug #211 fixed forrsvd).CONTRIBUTING.md,
transform_template.pyandrun_example.pywere all brought in line.strategy/README.md's "Register your own (the updatable hook)" — the copy a contributor meets first — still showed the old signature:Why it survived
tests/test_contributor_docs_basis_signature.pywas added for exactly this class of drift, but never covered the README. Its own docstring scoped it out:So the one uncovered snippet is precisely the one that stayed stale — and #274 notes it has now regressed twice.
Fix
strategy/README.md: the hook declaresfrac, with a note on what to do with it (forward tostream_gemm_*; declare it even if unused, because the gate keys off the signature).tests/…basis_signature.py: close the hole. The markdown copies now run through one parametrized table (CONTRIBUTING.md,strategy/README.md), so a rollback — or a newly added doc — cannot quietly skip the check. Also asserts the full parameter list againstTransform.basis(defaults stripped), not merely the presence offrac, so a snippet cannot drift on any other argument either.Proof the guard actually bites — reverting just the README makes it fail:
Credit
#275 (@luciferlive112116) reported the same gap. It was closed unmerged by the 12h merge-conflict rule after #253 landed the test file underneath it — not on merits. This is an independent implementation against current main, scoped to the one snippet that is still stale (CONTRIBUTING.md and run_example.py are already correct).
Touches only
strategy/README.mdandtests/— no protected paths. (docs/whitepaper.mdhas the same stale snippet but is maintainer-owned, so it is left alone.)Validation
Pure text/AST parsing — no GPU needed.
Checklist
Co-authored-byfooters for coding agents such as Cursor, Codex, Claude, Copilot, or similar tools.featPR — no scorecard required.