Docs: state that the device log threshold is fixed at device init - #2117
Docs: state that the device log threshold is fixed at device init#2117ChaoWao wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR documents that host and simulated-AICPU log thresholds update immediately, while onboard AICPU latches ChangesOnboard AICPU logging lifetime
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This change documents the existing distinction between live host logging thresholds and the Worker-lifetime onboard AICPU threshold, including Worker recreation as the supported way to change device verbosity. No behavior changes are introduced, and the change is ready to merge. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue [ Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. (1 skipped: 1 unsupported.) 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 |
|
Since #2029 a Fixed in #2118, which covers all three. Once that lands I will rebase this one; re-running the job here would probably go green on its own, but the flake would stay. |
Two lifetimes exist and the docs described only one. Every host module reads the bound state's threshold per record, so a `set_level` reaches all of them immediately — sim AICPU included, since it runs in the host process as one of them. Onboard AICPU is different: `simpler_aicpu_init` latches `InitArgs.log_level` into a device-side flag, that entry runs once per Worker, and no later `set_level` reaches it. Nothing said so. `docs/logging.md` claimed "all bound host modules observe a native state update immediately instead of requiring threshold fan-out to every DSO" — correct as written and easy to read as "live everywhere" — while the configuration-flow table described the onboard AICPU row only in terms of CANN policy. `set_level` itself carried no note about its reach. This is the intended contract rather than a missing refresh, and it is now recorded as one: raising verbosity mid-run to chase a device-side problem is not a supported workflow, and recreating the Worker is what to do instead. A `Worker` is cheap to recreate, and device log volume is where an accidental `DEBUG` costs most — `codestyle.md` rule 7 forbids AICPU hot-path logging because `device_log` writes serialize on the single AICPU op and can trip the op-execute timeout, so a threshold that outside code can raise into that is a liability rather than a feature. Both ends of the one-shot path now say so at the line that creates it, in both architecture trees, and `set_level`'s declaration scopes its own liveness claim. Closes hw-native-sys#2109. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bd53230 to
4cb3cc4
Compare
1 participant
Summary
Closes #2109 by recording the contract rather than building a mechanism for it.
I filed #2109 as a defect. It is not one — the device threshold being fixed at device init is a design decision, and the actual defect was that nothing said so. This documents it.
Two lifetimes, and only one was described
set_leveltakes effectdlopened onestate()->thresholdper recordsimpler_aicpu_initlatchesInitArgs.log_levelinto a device-side flag, and that entry runs once per Worker.What the docs said before:
docs/logging.md— "all bound host modules observe a native state update immediately instead of requiring threshold fan-out to every DSO." Correct as written, and easy to read as "live everywhere".set_levelcarried no note about its reach at all.Why it stays one-shot
Raising verbosity mid-run to chase a device-side problem is not a workflow this runtime supports; recreate the Worker with the level you want.
Workeris cheap to recreate, so the workaround costs nothing a debugging session would notice.DEBUGis most expensive.codestyle.mdrule 7 forbids AICPU hot-path logging becausedevice_logwrites serialize on the single AICPU op and can trip the op-execute timeout — a threshold outside code can raise into that is a liability, not a feature.The doc also records what delivering it would cost, so the next person does not have to re-derive it: making it immediately live needs either a device launch per
set_level— the opposite direction from #2092, which exists to makesimpler_aicpu_initlaunch exactly once — or a new host-writable device-resident location polled outside any launch.I did start down the "make it work" path before this was settled, and found the threshold could ride an existing per-run payload (
KernelArgsis host-built per run and read bysimpler_aicpu_exec, and onboardset_log_levelis a single bool store, so a per-run refresh would be nearly free and give next-run granularity). That is recorded in the doc as the cheap option that still is not worth taking, since there is no use case asking for it. Better to write it down than to leave it as a discovery someone repeats.What changed
docs/logging.md— a new "The threshold is live on the host and fixed on the device" section with the table above and the rationale; the configuration-flow table's AICPU row now names the latch.src/common/log/include/host_log.h—set_levelscopes its own liveness claim and points at the doc.src/common/platform/onboard/host/device_runner_base.cpp— theInitArgs.log_levelfill says it is the only time it happens.src/a2a3/…/aicpu/kernel.cppandsrc/a5/…/aicpu/kernel.cpp— theset_log_levelcall says it is one-shot. Both trees, since they are near-duplicates and a contract note that lands in one is stale in the other.No behaviour change: comments and docs only.
Testing
The onboard sweep is the one that matters here: the two annotated lines are on the AICPU init path, and although the edit is comment-only, all four platforms were rebuilt and the sweep run rather than assuming a comment cannot break a build.