Replace the skills' guessed machine numbers with measured ones - #291
Merged
Conversation
Two skills carried performance guidance that was never measured on this machine. Both are now backed by benchmarks run against the current stack, and the corrections went in the opposite direction from what I expected. nematode-run-evolution told sessions to divide wall-clock by the worker count: "parallel 1 = 240 episodes ~ 24 s. With --parallel 4 ~ 6 s". The serial estimate is good — measured 25.9 s — but the division is not. Real speedup saturates at 2.5-3x and is essentially reached by 6-8 workers: 800 MLPPPO episodes take 31.9 s serial, 12.0 s at 6 workers, and 11.1 s at 18. Each generation is a barrier and the optimiser step between them is serial, so extra workers buy fork overhead. On small runs more workers are actively slower — 80 episodes cost 5.8 s at --parallel 1 and 7.2 s at --parallel 18. The skill now carries the measured table, recommends --parallel 8, and the example config uses 8 instead of 4. Per-episode costs were re-measured and the old figures were close enough to keep: ~40 ms MLPPPO, ~110 ms LSTMPPO. nematode-run-experiments claimed "max 16 concurrent sessions — machine handles this without degradation". Sixteen turns out to be right, but for a reason worth writing down: it is where throughput peaks (~1.8 sessions/s, against 1.77 at 24 and 1.68 at 32), not a hard ceiling. Rounds needing six groups or six seeds can use 24. The skill also implied memory might be the constraint; it is not, at ~0.5 GB per session, so a matrix should never be cut out of memory worry — only because sessions compete for cores. Added thread pinning to the launch snippet, which is worth ~6% throughput at 24 sessions and more above that. Also corrected the evolution smoke-test timing (~2.5 s, not ~4 s) and verified every file and config path referenced across all seven skills still resolves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Two skills carried performance guidance that was never measured on this machine. Both are now backed by benchmarks against the current stack (Python 3.13, qiskit 2.5.2, torch 2.13), and both corrections went the opposite way from what I expected.
nematode-run-evolution:--paralleldoes not divide wall-clockThe skill told sessions to compute runtime as
... / parallel_workers, with the example "parallel 1 = 240 episodes ≈ 24 s. With--parallel 4≈ 6 s". The serial estimate is good — measured 25.9 s. The division is not.800 MLPPPO episodes:
--parallelSpeedup saturates at 2.5-3x, essentially reached by 6-8 workers. Each generation is a barrier and the optimiser step between generations is serial, so beyond that you pay fork overhead for nothing. I confirmed it is not population-bound by re-running with population 36 — same plateau.
On small runs more workers are actively slower: 80 episodes cost 5.8 s at
--parallel 1and 7.2 s at--parallel 18.The skill now carries the measured table, recommends
--parallel 8, and its example command uses 8 instead of 4. Per-episode costs were re-measured and the existing figures were close enough to keep (~40 ms MLPPPO, ~110 ms LSTMPPO).nematode-run-experiments: 16 was right, for the wrong reasonThe claim was "Max 16 concurrent sessions — machine handles this without degradation". Sixteen is indeed the sweet spot, but it is where throughput peaks, not a ceiling:
So a round that genuinely needs six groups or six seeds can use 24 for ~2% less throughput. The skill previously implied the group limit was machine capacity; it is really about how many arms a comparison can be read with, so that is what it now says.
It also read as though memory might be a constraint. It is not — a session peaks at ~0.5 GB, so even 32 at once is a small fraction of what is available. The skill now says explicitly never to cut an experiment matrix out of memory worry, only because sessions compete for cores.
Added
OMP_NUM_THREADS=1 MKL_NUM_THREADS=1to the launch snippet — each session otherwise spawns a full BLAS thread pool and they oversubscribe each other. Worth ~6% throughput at 24 sessions and more above that. (Same lever that fixed CI in #288.)Also
No chip or model names appear in the skills — the guidance is expressed as measured behaviour and thresholds.
🤖 Generated with Claude Code