Repository navigation
[Worker][Bug fix] Read the speculation knobs when the worker is built, not after warmup - #463
Conversation
|
/review |
Automated reviewThe worker now reads Findings: none. I checked the claim that "every value that worked before gives the same result" against how the values are used:
The test count of 13 matches the file. The description doesn't mention that Advisory, not a merge gate. Rules: |
Live test: [Worker][Bug fix] Read the speculation knobs when the worker is built, not after warmupWorks. On base a malformed knob kills the deployment after weight load and CUDA-graph capture; on the PR it warns at startup, uses the default, and serves. No CPU-overhead regression outside noise. 1x H200, Qwen3.5 0.8B · PR head Claims
PerformanceQwen3.5 0.8B, closed loop, base, PR, PR, base, text tok/s, two jobs:
Caveat: node load average 20–96. Client command
Other
|
merceod
left a comment
There was a problem hiding this comment.
Ran main 84fd4a6 + e98e280 (463m, the only conflict is test/cpu-core.txt) against main on a klone H200 (clock-limited node), Qwen3.5-0.8B.
- MSTAR_LAUNCH_WAIT_MS=abc. Main captures 76 CUDA graphs, the conductor counts the worker ready, and 322 s after the first log line the worker dies in run() with "could not convert string to float: 'abc'", both warmup requests fail and the deployment shuts down. 463m logs "Ignoring invalid MSTAR_LAUNCH_WAIT_MS='abc'; using 5.0" before "Engine loaded" and serves 8/8.
- -5 is raised to 0.0 with a warning, MSTAR_MAX_CONSECUTIVE_SPEC_STEPS=0 warns that it turns speculation off and serves, both messages name the variable.
- Throughput. Cap 0 against the default at c=16 costs about the same on both trees (main 2156 to 1888 out tok/s, -12%, 463m 1920 to 1754, -9%, ranges overlap), as it should since the PR only moves where the value is read. MSTAR_LAUNCH_WAIT_MS=20 at c=32, 3037 vs 3096, within noise. 0 errors in every launch.
- 13/13 new tests, 13 fail on main. worker.py has no main commits since your base. cpu-core failing set equals main's, ruff clean.
- Reading agrees, both knobs are read in init right after worker_id, device and NVTX, before the communicator and the engine build, run() uses the stored values, and _wait_ms rejects nan, inf and anything past TIMEOUT_MAX.
One note, inline. MSTAR_LAUNCH_WAIT_MS still has no row in docs/environment_variables.rst. Pre-existing, but since this PR changes how it parses and touches the sibling row, this is the cheap moment to add it.
Approving. Needs the rebase for cpu-core.txt.
| @@ -321,7 +321,8 @@ Worker scheduling | |||
| * - ``MSTAR_MAX_CONSECUTIVE_SPEC_STEPS`` | |||
There was a problem hiding this comment.
MSTAR_LAUNCH_WAIT_MS still has no row here. Pre-existing gap, but this PR changes how it parses bad values and already touches this sibling row, so adding it now would be cheap (default 5 ms, non-finite or past TIMEOUT_MAX falls back with a warning, below 0 is raised to 0).
There was a problem hiding this comment.
Thanks for running this. Added the row (default 5 ms; a non-number, nan, inf or a value past threading.TIMEOUT_MAX falls back to 5 with a warning; a negative value is raised to 0), and rebased on main for test/cpu-core.txt.
e98e280 to
113df46
Compare
Fixes #462
What does this PR do?
A malformed
MSTAR_LAUNCH_WAIT_MSorMSTAR_MAX_CONSECUTIVE_SPEC_STEPSraised only after weight load, warmup, capture and SETUP_DONE, and then took the deployment down. The worker now reads both when it is built, before anything loads, and handles a bad value the wayMSTAR_PY_SWITCH_INTERVAL_SECandMSTAR_RECOMPILE_LIMITalready do:nan,infor more thanthreading.TIMEOUT_MAXseconds): a warning, and the default is used.MSTAR_LAUNCH_WAIT_MSgets the row indocs/environment_variables.rstit never had (default, units and the handling above).Every value that worked before gives the same result.
Schematic, not to scale: both knobs are now read before anything loads, and a bad value warns and falls back instead of raising after SETUP_DONE.
How was it tested?
test/modular/test_worker_spec_knobs.py: 13 CPU tests, added totest/cpu-core.txt, running the realWorker.__init__; all 13 fail on main, which reads neither knob in__init__.ruff check .is clean and the file passes 13/13 on the head (rebased on main).@test/cpu-core.txtwith the GPU hidden: 1307 passed, 46 skipped, before the rebase.Not tested
@test/cpu-core.txtwas not rerun after the rebase: the rebase only mergedtest/cpu-core.txtand the new commit is docs only.Details
_env_number(name, default, parse, minimum)is a small module helper._wait_msrejectsnan,infand anything pastthreading.TIMEOUT_MAXseconds, whereEvent.waitoverflows.Worker.__init__right afterworker_id,deviceand NVTX are set, before the communicator, the graph runtime andEngineManager.build.run()uses the stored values.Tests
test/modular/test_worker_spec_knobs.py: 13 CPU tests, added totest/cpu-core.txt. They run the realWorker.__init__and stop it at the first call after the knobs, before anything is built. They cover:nan,inf,1e20,1e3and16.0;Worker.runno longer reading either variable.__init__.ruff check .is clean andtest/modular/test_worker_spec_knobs.pypasses 13/13 on the head (rebased on main).@test/cpu-core.txtwith the GPU hidden: 1307 passed, 46 skipped, before the rebase. Not rerun after it: the rebase only mergedtest/cpu-core.txtand the new commit is docs only.Checklist
ruff check .passesAI assistance: written with AI coding agent