From 1ee6e07be1c7e034e0d2ac97cb5a0350ac51baa0 Mon Sep 17 00:00:00 2001 From: Chao Wang <26245345+ChaoWao@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:30:57 -0700 Subject: [PATCH] Pin the C++ span names to the ladder, and stop writing them twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewing #1875 turned up how a rename can go silently wrong. That PR adds EXPECT_FALSE(captured_host_span("host.dispatch")); on a base that predates #1893, where the word became `node`. Rebasing produces no conflict — the line is new, not edited — so the assertion looks for a name nothing emits, `captured_host_span` answers false, and `EXPECT_FALSE(false)` **passes**. The test that exists to prove a disabled gate emits nothing would then pass whether the gate works or not. Ranked by when you find out, that is the worst of the three outcomes a rename can have: a conflict is immediate, a red test is one CI run, and a vacuously-true assertion is never. What makes it silent is the negated form — the same staleness under `EXPECT_TRUE` merely goes red. Two changes, and the first is not a new guard but the removal of what made the guard necessary. **The names come from the table the emitter uses.** Eight literals in `test_scheduler.cpp` are now `host_span_name(HostSpan::Dispatch)` and friends. These tests assert that a decision point emitted at all; the name is not their subject, so writing it out was a second copy of it. Taking it from the single source means a rename cannot leave them behind — verified by renaming the level word to `network9` and re-running: the suite stays green because the assertions follow. Under the old literals the same rename would have reddened the `EXPECT_TRUE` ones and silently passed an `EXPECT_FALSE` one. **The C++ pre-bind default is pinned to the ladder.** `host_span_names.h`'s `level_word()` defaults to `"node"`, which was a third hand-written copy of the L3 word: `WorkerLevel` is the source of truth, `strace_timing.py`'s `_NODE_WORDS` is pinned to it by its own test, and this one was pinned to nothing. A level renamed in Python would leave C++ emitting the old word for every span before a Worker binds the prefix, with no test noticing. The pin reads that default through the existing binding rather than adding a symbol: `set_level_prefix("")` returns early without binding, while the binding still reports what is in effect. It runs in a child process because the prefix freezes on first bind, so a Worker constructed anywhere in this process would leave the *bound* word behind instead of the default. `test_graph_failure_still_emits_graph_build_span` also spelled out `node.graph_build`; it now builds the expected name from the worker's own `_host_span_prefix`, which is the contract that assertion is about. Verification: - `pytest tests/ut/py` — 1657 passed, 0 failed. - `ctest -LE requires_hardware` — 107/107. - Negative control for the pin: setting the C++ default back to `"host"` fails `test_the_cpp_pre_bind_level_word_is_the_ladder_word_for_l3` and nothing else. - Negative control for the double-write removal: renaming the level word to `network9` keeps `test_scheduler` green, where a literal would have gone red or, worse, vacuously true. Not every literal span name is a double-write, and the distinction is which side of the test it sits on. A literal that **constructs input** — the `SimplerHostSpan` fed to the logger in `test_host_log_off.cpp`, the fake log lines in `test_strace_timing.py` — is right to be written out: the test owns its input, and it is exercising encoding or parsing over an arbitrary name. A literal that **asserts the implementation emitted a particular name** is the second copy, and that is the class both changes above remove. The eight in `test_scheduler.cpp` were the only ones of that kind. What this does not cover, stated because "renames are guarded now" would be too broad a claim: the pin ties the C++ *default* word to the ladder, not every place a name could be spelled out. `git grep -n '"\(node\|network[123]\)\.'` before landing a rename, and judge each hit by the input-versus-assertion test above. --- tests/ut/cpp/hierarchical/test_scheduler.cpp | 23 +++++++++++------ tests/ut/py/test_worker/test_host_worker.py | 27 +++++++++++++++++++- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/tests/ut/cpp/hierarchical/test_scheduler.cpp b/tests/ut/cpp/hierarchical/test_scheduler.cpp index ec1f9fdb58..327efe0f2b 100644 --- a/tests/ut/cpp/hierarchical/test_scheduler.cpp +++ b/tests/ut/cpp/hierarchical/test_scheduler.cpp @@ -39,6 +39,7 @@ #include "call_config.h" #include "common/host_span.h" +#include "common/host_span_names.h" #include "common/host_span_scope.h" #include "orchestrator.h" #include "ring.h" @@ -1210,13 +1211,13 @@ TEST(WorkerManagerTest, DispatchAndCompletionEmitHostSpans) { worker.dispatch(WorkerDispatch{slot, 0}); ASSERT_TRUE(endpoint_ptr->wait_submitted(1)); - EXPECT_TRUE(captured_host_span("node.dispatch")); + EXPECT_TRUE(captured_host_span(simpler::host_trace::host_span_name(simpler::host_trace::HostSpan::Dispatch))); const WorkerDispatch submitted = endpoint_ptr->submitted().front(); endpoint_ptr->emit(WorkerProgressKind::COMPLETED, submitted); worker.progress(); ASSERT_EQ(completed.size(), 1u); - EXPECT_TRUE(captured_host_span("node.complete")); + EXPECT_TRUE(captured_host_span(simpler::host_trace::host_span_name(simpler::host_trace::HostSpan::Complete))); worker.stop(); allocator.shutdown(); @@ -1474,8 +1475,8 @@ TEST(WorkerManagerTest, TwoFrameLeaseSlotsDoNotDefineFifoOrAcceptance) { ASSERT_TRUE(endpoint.poll_progress(progress)); EXPECT_EQ(progress.kind, WorkerProgressKind::ACCEPTED); EXPECT_EQ(progress.dispatch.dispatch_id, 42u); - EXPECT_TRUE(captured_host_span("node.frame_submit")); - EXPECT_TRUE(captured_host_span("node.activate")); + EXPECT_TRUE(captured_host_span(simpler::host_trace::host_span_name(simpler::host_trace::HostSpan::FrameSubmit))); + EXPECT_TRUE(captured_host_span(simpler::host_trace::host_span_name(simpler::host_trace::HostSpan::Activate))); allocator.shutdown(); } @@ -2042,10 +2043,13 @@ TEST_F(ProgressSchedulerFixture, GroupSubmitReportsNoSingleWorkerAndNoSingleInde ); orchestrator.close_run_submission(run); - ASSERT_TRUE(captured_host_span("node.submit")); + ASSERT_TRUE(captured_host_span(simpler::host_trace::host_span_name(simpler::host_trace::HostSpan::Submit))); std::ostringstream expected; expected << "run_id=" << run << " task_slot=" << group.task_slot << " group_index=-1 group_size=2 role=facade"; - EXPECT_EQ(captured_host_span_attrs("node.submit"), expected.str()); + EXPECT_EQ( + captured_host_span_attrs(simpler::host_trace::host_span_name(simpler::host_trace::HostSpan::Submit)), + expected.str() + ); } TEST_F(ProgressSchedulerFixture, SuccessorStagesButActivatesOnlyAfterFifoPromotion) { @@ -2515,11 +2519,14 @@ TEST_F(SchedulerFixture, NextLevelSubmitEmitsAPairableHostSpan) { auto submitted = orch.submit_next_level(C(0x42), single_tensor_args(0xCAFE, TensorArgType::OUTPUT), cfg, 0); - ASSERT_TRUE(captured_host_span("node.submit")); + ASSERT_TRUE(captured_host_span(simpler::host_trace::host_span_name(simpler::host_trace::HostSpan::Submit))); std::ostringstream expected; expected << "run_id=" << run_id << " task_slot=" << submitted.task_slot << " group_index=0 group_size=1 worker_id=0 role=facade"; - EXPECT_EQ(captured_host_span_attrs("node.submit"), expected.str()); + EXPECT_EQ( + captured_host_span_attrs(simpler::host_trace::host_span_name(simpler::host_trace::HostSpan::Submit)), + expected.str() + ); mock_worker.wait_running(); mock_worker.complete(); diff --git a/tests/ut/py/test_worker/test_host_worker.py b/tests/ut/py/test_worker/test_host_worker.py index 70fd1ab8bf..9c134f2379 100644 --- a/tests/ut/py/test_worker/test_host_worker.py +++ b/tests/ut/py/test_worker/test_host_worker.py @@ -19,6 +19,7 @@ import inspect import multiprocessing.shared_memory as shared_memory_mod import struct +import subprocess import sys import threading import time @@ -65,6 +66,7 @@ _mailbox_store_i32, _pack_py_callable_payload, ) +from simpler.worker_level import WorkerLevel from ._harness import chip_callable, fake_chip_l3, requires_sim_binaries @@ -2755,7 +2757,8 @@ def bad_graph(*_args): with pytest.raises(ValueError, match="bad graph"): worker._submit_l3_locked(bad_graph, None, cast(Any, object())) - assert emitted == [("node.graph_build", 1, 0, 0, 100, 175, "run_id=1 role=facade")] + expected_name = f"{worker._host_span_prefix}.graph_build" + assert emitted == [(expected_name, 1, 0, 0, 100, 175, "run_id=1 role=facade")] def test_unsettled_graph_cancellation_abandons_the_handle_before_close(self): worker, events = self._submission_failure_worker(failures=2) @@ -7655,3 +7658,25 @@ def test_unregister_removes_only_after_last_digest_ref(self): shm.unlink() payload_shm.close() payload_shm.unlink() + + +def test_the_cpp_pre_bind_level_word_is_the_ladder_word_for_l3(): + """`host_span_names.h` hand-writes the L3 word a third time, as its pre-bind default. + + `WorkerLevel` is the source of truth and `strace_timing.py`'s copy is pinned to + it by its own test, but the C++ default is pinned to nothing — so a level word + could be renamed in Python while C++ keeps emitting the old one until a Worker + binds the prefix. Every span emitted before that binding would carry the stale + word, and no test would notice. + + Read in a child process on purpose: the prefix freezes on first bind, so any + test in this process that constructed a Worker would leave the *bound* word + here instead of the default. Passing an empty word binds nothing (the setter + returns early) while the binding still reports what is currently in effect. + """ + source = "from _task_interface import _set_host_span_level_prefix as bind; print(bind(''), end='')" + completed = subprocess.run([sys.executable, "-c", source], capture_output=True, text=True, check=True, timeout=120) + + assert completed.stdout == WorkerLevel.node.name, ( + f"C++ pre-bind level word is {completed.stdout!r}, ladder says {WorkerLevel.node.name!r}" + )