Refactor: make the examples tree internally consistent - #1547
Conversation
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3159fe9 to
dc5bf02
Compare
Three inconsistencies, each of which makes the tree harder to read than the code in it warrants. None changes behaviour. Test module names now match their directory. Four scene tests were named after a sibling example rather than their own: paged_attention_manual_scope and paged_attention_unroll_manual_scope, on both arches, each carried the plain paged_attention test name, so a failure report named a directory that was not the one running. scalar_data_test is renamed to scalar_data, since the directory suffix only duplicated what test_ already says; the file inside keeps its name and now matches. vector_add keeps two test modules on purpose: test_run_timing.py asserts the [STRACE] markers and is a second test, not a mismatched one. Four L3 example directories gained the __init__.py their siblings all have. Without it the package-relative `from .main import run` that every one of these tests uses depends on rootdir inference rather than being a package. allreduce had no test at all, so the only L3 example without regression protection was a collective. It now has one, scoped to a2a3: the demo's own README and CLI default document that platform, and while the algorithm is covered on a5 by tests/st/worker/collectives/allreduce, nobody has run this demo there. Widening it is a separate, verifiable step. The test documents that run() takes (device_ids, platform), the reverse of the single-device demos beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kernel sources carry two conventions. The collectives kernels qualify (pto::Stride<...>) and use no using-directive; the older scene-test kernels open with `using namespace pto;` and use bare names. Each is internally consistent, and nothing recorded which one new code should follow, so the split propagates by whichever file an author copied from. Rule 11 names qualification as the direction, because it is the only spelling that compiles whether or not a using-directive is in scope, and because a file-scope `using namespace` is what most style guides steer away from. New kernels qualify and do not add the directive; existing bare files are not defects and need no churn. The load-bearing part is the constraint that a file is qualified completely or not at all. Shape, Stride, Tile and GlobalTensor all live in namespace pto, so qualifying one and leaving its siblings bare yields adjacent lines that read worse than either consistent state. That is a real failure mode rather than a hypothetical: an earlier revision of PR #1547 rewrote Stride across 24 files and left Shape alone, and the resulting mixture is why it was reverted. A repo-wide sweep is ruled out with the measurement that rules it out: the 142 files carrying the directive hold roughly 3,200 bare uses, of which Tile alone is 2080. Rewriting them would touch every kernel, collide with every in-flight branch, and risk regex damage for no functional gain. Unifying the tree in one go needs its own decision plus a lint rule to hold the line.
1 participant
Why this instead of writing 27 READMEs
The ask was READMEs for the 27 example directories that lack one. Measuring the tree first changed the answer: documenting it as-is would have written the inconsistencies down rather than removed them, and would have copied the duplication into the docs. This is the cleanup that should come first. No behaviour changes.
What this fixes
Test module names did not match their directory.
paged_attention_manual_scopeandpaged_attention_unroll_manual_scope, on both arches, each carried the plainpaged_attentiontest name — so a failure report named a directory that was not the one running. Renamed to match.scalar_data_test→scalar_data: the directory suffix only duplicated whattest_already says. The file inside keeps its name and now matches.vector_addkeeps two test modules on purpose —test_run_timing.pyasserts the[STRACE]markers. That is a second test, not a mismatch, so it is left alone.Four L3 examples had no
__init__.pywhile every sibling did (allreduce,ffn_tp_parallel,l3_l2_message_queue,l3_l2_orch_comm_stream). Every one of these tests doesfrom .main import run, so without the marker that relative import leans on rootdir inference instead of being a package.allreducehad no test at all — the only L3 example without regression protection, and it is a collective. It now has one. I checked first whether it merely duplicatedtests/st/worker/collectives/allreduce: it does not. That one holds four algorithm variants; this one has its ownmain.pydemo and no test. It was an oversight.A
Stridenormalization was dropped from this PR after reviewAn earlier revision of this branch also rewrote bare
Stride<topto::Stride<across 24 a2a3 kernel files, on the reasoning that the qualified spelling was "the portable one".That was wrong, and it is reverted. The kernels carry
using namespace pto;, and there is exactly oneStridein the pto headers — no ambiguity — so the qualification is redundant. Worse,Shapelives in the same namespace and stayed bare, so the change produced adjacent lines like:Making it genuinely consistent would mean qualifying every
Shapetoo — 50 more files, purely stylistic, and not something to bundle into a consistency PR unasked. The.cppfiles are byte-identical tomainagain; the only.cppchanges remaining are the three that moved with thescalar_datarename, verified by checksum.What measuring the tree exposed, and did not fix
Deliberately not in this PR:
vector_exampleStridespelling, 1 ×platformsdeferred_notify_demoasync_notify_demosdma_async_completion_demopaged_attention×3 variantsRoughly 5,280 lines of near-duplicate example code, and
tests/st/has the same shape — 18 same-named directory pairs. Merging is technically feasible: arch comes from the platform string, not the path (KernelCompiler(platform=…)), andCASES[*].platformsexists precisely to declare several at once.Not proposed here for two reasons. It changes the arch-first directory convention, and it runs counter to in-flight work — #1450 is porting a2a3 tests to a5 by copying, adding 29 more files of exactly this duplication. That is a maintainer's call about direction.
READMEs are best written after that decision: merging the four trivially-duplicated pairs would cut the number needed roughly in half.
Verification
ruff check/ruff format --checkclean acrossexamples/gitrecords all 8 moves as renames, so history is preserved.cppcontent changes remain — confirmed by diffing everyexamples/**/*.cppagainstmainand by checksumming the three files that moved with the renamemulti_chip_dispatchsibling exactly_task_interfacein my venv — the repo's own build-stamp guard rejects it. Confirmed environmental, not introduced here: the pre-existingmulti_chip_dispatchtest fails collection identically. CI is the real check.allreduce's test is scoped toa2a3sim/a2a3rather than all four platforms: that is what the demo's own README and CLI default document. The algorithm runs on a5 (tests/st/worker/collectives/allreducedeclares all four), so widening is likely fine — but nobody has run this demo there, and I could not verify locally, so I did not assert it.