Repository navigation
Conversation
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ 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 |
`task_args.h` bundled two things at different layers. The generic argument
container — `TaskArgsTpl`, its tag mixin, and the `ChipStorageTaskArgs` alias whose
element is `ChipTensor` — is needed by every runtime and orchestration translation
unit. Alongside it sat the L3+ form, whose element is the self-describing `Tensor`
from buffer.h: the `TaskArgs` builder, `TaskArgsView`, the mailbox blob codec, and
`validate_submit_args`. Because those need buffer.h, the header included it, and
`buffer.h` declares a `Tensor` at **global scope**.
buffer.h has exactly one includer in src/ — that line — so the effect was that every
orchestration translation unit saw the L3+ wire type. Measured on the pre-change
tree, this chain:
orchestration_api.h -> runtime/graph_cache.h -> common/{hbg}/graph_cache.h
-> runtime/types.h -> task_args.h -> task_interface/buffer.h
`types.h` needs the container because `Arg : TaskArgsTpl<TensorRef, …>` is the submit
surface orchestration builds on; it has no use for the wire form.
The L3+ half moves to `task_args_wire.h`, which includes `task_args.h` and buffer.h.
Its consumers are the layer that actually handles that form: the host orchestrator
under `src/common/hierarchical/`, the Python bindings, the platform host code that
decodes a blob into `ChipStorageTaskArgs`, and the cpput case that round-trips a blob.
Eleven includes change; nothing else moves and no declaration changes. `task_args.h`
keeps its name, so the fourteen runtime and orchestration includers are untouched, and
carries a pointer to where `TaskArgs` went — a reader grepping the obvious file for the
obvious name should not come up empty.
`ChipStorageTaskArgs` stays on the generic side by the same criterion that drove the
split: its element is `ChipTensor` from task_interface/tensor.h, not the wire
`Tensor`, so it needs no buffer.h. It is also what `runtime.h` and
`orchestration_api.h` include the header for.
The consequence is that `Tensor` is no longer taken inside a kernel or orchestration
source. hw-native-sys#2032 established that for kernels; this closes the orchestration half, which
is the larger one — `using Tensor = simpler::hbg::Tensor;` in an orchestration
translation unit failed with a conflicting declaration before this change and
compiles after. Verified in both directions: a control that pulls in
`task_args_wire.h` by hand on the changed tree still fails, so the check answers to
the include edge rather than to the alias. Both runtimes' orchestration paths were
probed with the include flags taken from `compile_commands.json` — `build_config.py`'s
`include_dirs` are relative to the runtime directory and do not carry `src/common`,
which the platform CMakeLists adds, so reading the build config alone gives the wrong
answer.
Also verified with the full product build across both arches x both runtimes x
sim/onboard, cpput 119/119 from a clean build directory, both sim scene-test sweeps in
the shape `_st-sim-{a2a3,a5}.yml` runs them (a2a3sim 60 passed / 8 skipped, a5sim 53
passed, each reconciling against `--collect-only`), and the whole scene-test corpus's
incores compiled with `--manual include` on both arches — zero failures, where the same
sweep reported six per arch before hw-native-sys#2027 landed, so it is sensitive rather than vacuous.
Clean is the operative word for cpput: an incremental build reuses object files and
dependency information, so it does not recompile a test whose include set this change
invalidates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChaoWao
force-pushed
the
split-task-args-wire-from-the-generic-container
branch
from
August 27, 2026 01:26
f85fff6 to
a120819
Compare
ChaoWao
added a commit
that referenced
this pull request
Aug 27, 2026
…on sources (#2044) Both runtime umbrellas exported their working tensor as `TaskTensor`, and 693 files in the scene-test corpus named it. The prefix carried no meaning: a kernel has exactly one tensor type, the one its payload elements are, and the umbrella exists precisely so a source compiled under both runtimes need not pick one. The name was a workaround — `Tensor` was taken inside those translation units by `task_interface/buffer.h`, which declares the address-free L3+ wire form at global scope, so the obvious alias was a conflicting redeclaration. Nothing takes it any more. #2032 lifted `EntryArgsStorage` out of the kernel-facing `common/{host_build_graph,tensormap_and_ringbuffer}/tensor.h`, cutting the kernel path to buffer.h; #2038 split `TaskArgs` and the blob codec into `task_args_wire.h`, cutting the orchestration path, which was the larger one. So the four umbrellas now export using Tensor = simpler::{hbg,tmr}::Tensor; and the 5657 occurrences across 693 files follow. The sweep is word-boundary anchored, so `ChipTensor` and `GlobalTensor` are untouched, and it ran over `git grep -Il` output so build artifacts were not rewritten. Verified beforehand that no C++ source under examples/ or tests/st/ already used a bare `Tensor`: the pattern finds 81 lines when Python and Markdown are included — `torch.Tensor`, the Python API, prose — and zero in `*.cpp`/`*.h`/`*.hpp`. The 234 files clang-format then wanted to change are a consequence of the rename, not pre-existing drift: a four-character-shorter name lets it rejoin lines that were wrapped. Checked by restoring one file's pre-rename content in place and confirming it is format-clean, where the renamed one is not. Formatting them makes the diff smaller overall. Two under `kernels/vendor/` are left alone, matching the clang-format hook's own exclusion. `tests/lint/check_kernel_wire_isolation.py` now holds the separation the name depends on. It checks the include *edge* rather than reachability, because pre-commit sees only the staged set and the file that would break reachability is typically a shared header such as `runtime/types.h`, not something under a kernel path: any file outside an allowlist that includes `task_interface/buffer.h` or `task_args_wire.h` fails. The allowlist is the twelve places that genuinely handle the L3+ form — the host orchestrator, the Python bindings, the platform host code that decodes a mailbox blob, and their unit tests — so it also documents who they are. Adding an entry stays possible and is a reviewable diff; adding one to a header kernels reach is the defect this surfaces. Exercised three ways: zero findings over all 1560 tracked C++ files, one finding on a probe that adds the include, and no finding on a probe naming `ring_buffer.h`, `aligned_buffer.h`, `kernel_pop_stack_buffer.h` and `task_args.h`. Verified with the full product build across both arches x both runtimes x sim/onboard, cpput 122/122 from a clean build directory, both sim scene-test sweeps in the shape `_st-sim-{a2a3,a5}.yml` runs them (a2a3sim 61 passed / 8 skipped, a5sim 54 passed, zero failures), and — the gate that actually covers a corpus-wide kernel rename — every scene test's incores compiled with `--manual include` on both arches, zero failures. That sweep reported six failures per arch before #2027 landed its fixes, so it is sensitive rather than vacuous. All of it re-run after the formatting pass.
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
1 participant
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.
The defect
task_args.hbundled two layers. The generic container —TaskArgsTpl, its tag mixin, and theChipStorageTaskArgsalias whose element isChipTensor— is needed by every runtime and orchestration translation unit. Sitting next to it was the L3+ form, whose element is the self-describingTensorfrombuffer.h: theTaskArgsbuilder,TaskArgsView, the mailbox blob codec,validate_submit_args.Because that half needs
buffer.h, the header included it — andbuffer.hdeclares aTensorat global scope.buffer.hhas exactly one includer insrc/, that line, so the effect was that every orchestration TU saw the L3+ wire type. Measured on the pre-change tree:types.hneeds the container, becauseArg : TaskArgsTpl<TensorRef, …>is the submit surface orchestration builds on. It has no use for the wire form.The change
The L3+ half moves to
task_args_wire.h, which includestask_args.h+buffer.h. Its consumers are the layer that actually handles that form:src/common/hierarchical/{types,orchestrator}.hTaskArgsand callsvalidate_submit_argssrc/common/platform/{onboard,sim}/host/{c_api_shared,device_runner_base}.cppChipStorageTaskArgspython/bindings/task_interface.cppTaskArgs/Tensortests/ut/cpp/types/test_child_memory.cppwrite_blob/read_blobEleven includes change. Nothing else moves and no declaration changes — 13 files, +281/−226, of which 226 deletions and 241 insertions are the move itself.
Two deliberate choices:
task_args.hkeeps its name, so the fourteen runtime and orchestration includers are untouched (they are near-duplicate a2a3/a5 trees, and a smaller diff there is worth more than a tidier filename). It carries a pointer to whereTaskArgswent — a reader grepping the obvious file for the obvious name should not come up empty.ChipStorageTaskArgsstays on the generic side, by the same criterion that drove the split: its element isChipTensorfromtask_interface/tensor.h, not the wireTensor, so it needs nobuffer.h. It is also the reasonruntime.handorchestration_api.hinclude the header at all.What this unblocks
Tensoris no longer taken inside a kernel or an orchestration source. #2032 established that for kernels; this closes the orchestration half, which is the larger one. Concretely,using Tensor = simpler::hbg::Tensor;in an orchestration TU:That matters because the umbrellas currently export the awkward
TaskTensorprecisely becauseTensorwas unavailable, and kernels and orchestration share onetensor.h. This PR does not rename anything — it removes the second of the two reasons the rename was impossible.Testing
_st-sim-a2a3.ymlmain sweep, verbatim shape--collect-only's 68 selected_st-sim-a5.ymlmain sweep, verbatim shape--manual include, both archesThat last row is worth a word, because a zero is only evidence if the check can be non-zero: the same sweep reported six failures per arch two commits ago, and reports zero now that #2027 has landed the fixes. It is sensitive, not vacuous.
"Clean build dir" in the cpput row is load-bearing, and the first CI round on this PR is why:
utwent red atBuild and run C++ unit testswithexit code 2while an incremental local cpput reported 119/119. An incremental build reuses object files and dependency information, so it never recompiledtest_child_memory.cpp— the one test whose include set this change invalidates.rm -rf tests/ut/cpp/buildfirst and it reproduces immediately. The macOSutin that round was matrix fail-fast collateral (step 7 cancelled, 9 and 10 skipped), not a second failure.The acceptance check probes both runtimes' orchestration paths with the include flags taken from
compile_commands.json, not frombuild_config.py— that file'sinclude_dirsare relative to the runtime directory and do not carrysrc/common, which the platformCMakeListsadds, so reading the build config alone gives the wrong answer. A control that pulls intask_args_wire.hby hand on the changed tree still fails with the error above, so the check answers to the include edge rather than to the alias.Docs
docs/buffer-abi.md's invariant paragraph was added by #2032 and this change makes it inaccurate — a kernel TU now does seetask_args.h, just not the wire half. Corrected, and extended to name the orchestration side.docs/task-flow.mdanddocs/remote-l3-worker-design.mdpointed attask_args.hfor the blob codec; both now point attask_args_wire.h, and a staletask_args.h:157line reference is dropped rather than renumbered.🤖 Generated with Claude Code