Skip to content

fix(strategy): declare frac in every contributor-facing basis() snippet (Fixes #274)#275

Closed
luciferlive112116 wants to merge 1 commit into
zeokin:mainfrom
luciferlive112116:fix/contributor-docs-basis-frac-v2
Closed

fix(strategy): declare frac in every contributor-facing basis() snippet (Fixes #274)#275
luciferlive112116 wants to merge 1 commit into
zeokin:mainfrom
luciferlive112116:fix/contributor-docs-basis-frac-v2

Conversation

@luciferlive112116

Copy link
Copy Markdown
Contributor

Fixes #274. Re-lands #253 (rolled back with its whole merge window) and extends it
to the copy #253 missed.

PR kind

  • fix
  • feat

Summary

multiply_subspace forwards the VRAM budget only to a basis() that declares
frac
— and its else is a compatibility shim for legacy transforms:

# Pass frac only if the transform accepts it, so custom transforms with the
# older basis(...) signature keep working.
if "frac" in inspect.signature(transform.basis).parameters: ... frac=frac
else:                                                        ... # no frac

So any snippet a contributor copies that omits frac yields a transform that lands
in the legacy branch and silently streams its basis at _DEFAULT_ROW_BLOCK_FRACTION
(0.3) instead of Config.vram_fraction — the bug #211 fixed for rsvd,
re-introduced once per contribution.

Three copies contributors read are currently on the pre-#211 signature:

copy before
CONTRIBUTING.md:73 — "What you actually change"
strategy/README.md:54 — "Register your own (the updatable hook)" never covered by #253
strategy/examples/run_example.py:45FirstAxes

The authoritative surfaces already declare it: Transform.basis, transforms.py's
module-docstring template, rsvd (#211), and examples/transform_template.py
(#251 — still on main).

Why two of them regressed

#253 fixed CONTRIBUTING + run_example, but was rolled back with the entire
9058d49..f35482a merge window: 1dc8fce..e42c525 reverted all seven PRs of
that batch, LIFO, across five authors (#235, #232, #253, #266, #267, #268, #255).
This re-lands that content rebased on current main. strategy/README.md's hook
was never covered by #253 and is genuinely new here — it's the copy sitting
directly under "The transform is the pluggable core tech", so the most likely one
to be copied.

Changes

  • CONTRIBUTING.md, strategy/README.md, strategy/examples/run_example.py
    declare frac=None and say what it's for. FirstAxes is a fixed basis that
    streams nothing, so it notes the parameter is unused there but must stay, since
    the forwarding gate is signature-based. +13/−5.
  • tests/test_contributor_docs_basis_signature.py (new) — CPU-only: regex-checks
    both markdown snippets and AST-parses the example (it can't be imported —
    it runs a real subspace_matmul at module scope, which needs a GPU), asserting
    each declares frac and that the example matches Transform.basis exactly.

No shipped engine code changed.

Validation

$ python -m pytest tests/test_contributor_docs_basis_signature.py -v
  3 passed

$ python -m pytest tests/ strategy/tests/ eval/tests/ -q     # CI-equivalent (no torch → GPU tests skip)
  380 passed, 24 skipped

No protected paths touched — root CONTRIBUTING.md, strategy/README.md and
strategy/examples/ are outside eval/, docs/, .github/, dashboard/.

Result

Contributor-facing docs/example + parsing tests; no shipped code path changed, so no
accuracy / latency / VRAM delta (it stops future transforms from silently losing
--vram-fraction in their basis stage).

metric value
accuracy unchanged (docs/example + tests only)
time complexity unchanged
latency N/A
VRAM usage N/A here; prevents copied transforms ignoring vram_fraction

Re-lands zeokin#253, which was rolled back with the whole 9058d49..f35482a merge
window (1dc8fce..e42c525 reverted all seven of that batch's PRs, LIFO, across
five authors), and extends it to the copy zeokin#253 missed.

multiply_subspace forwards the VRAM budget only to a basis() that declares frac:

    if "frac" in inspect.signature(transform.basis).parameters:
        Q = transform.basis(n, m, backend, cdt, A=A, B=B, frac=frac)
    else:
        Q = transform.basis(n, m, backend, cdt, A=A, B=B)

That else-branch is a compatibility shim for legacy transforms, so a contributor
copying a snippet without frac lands there and silently streams the basis at the
default fraction rather than Config.vram_fraction -- the bug zeokin#211 fixed for rsvd,
re-introduced once per contribution.

Three copies a contributor reads were on the pre-zeokin#211 signature; strategy/README's
'Register your own' hook was never covered by zeokin#253 at all:

  * CONTRIBUTING.md          'What you actually change'
  * strategy/README.md       'Register your own (the updatable hook)'   <- new
  * strategy/examples/run_example.py  FirstAxes (fixed basis; frac unused but
    kept, since the forwarding gate is signature-based)

The base class, transforms.py's module docstring, rsvd and transform_template
(zeokin#251, still on main) already declare it.

Tests parse the markdown snippets and AST-parse the example -- run_example.py
runs a real subspace_matmul at module scope and cannot be imported without a GPU.
@github-actions github-actions Bot added area:strategy Smart strategies / transforms (strategy/) area:tests Correctness gates and test suites (tests/) area:docs README, CONTRIBUTING, BENCHMARKS and other docs status:needs-review Awaiting maintainer review type:bug Something is incorrect or broken status:ready-non-gpu Fix/docs PR cleared non-GPU triage; maintainer review only labels Jul 16, 2026
@github-actions

Copy link
Copy Markdown
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.

@github-actions github-actions Bot removed the status:ready-non-gpu Fix/docs PR cleared non-GPU triage; maintainer review only label Jul 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Closing this PR because the reported merge conflict was not resolved within 12 hours of the bot reminder.

@github-actions github-actions Bot closed this Jul 17, 2026
zeokin pushed a commit that referenced this pull request Jul 17, 2026
…s() hook

Fixes #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
#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: #275 (luciferlive112116) reported the same gap; it was closed unmerged
by the 12h merge-conflict rule after #253 landed the test file underneath it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:docs README, CONTRIBUTING, BENCHMARKS and other docs area:strategy Smart strategies / transforms (strategy/) area:tests Correctness gates and test suites (tests/) status:needs-review Awaiting maintainer review type:bug Something is incorrect or broken

Projects

None yet

1 participant