Repository navigation
feat(bench): measure whole-machine throughput with NUMA workers - #125
Conversation
6189db9 to
bb75d3e
Compare
bb75d3e to
2f4058a
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It combines spawn-based multiprocessing orchestration, cross-process timing, and ctypes libnuma syscalls, and refactors the shared NUMA module used by the FastSAC async training path, which warrants human verification despite solid test coverage.
2 open findings
What changed in this PR
This PR reworks the environment benchmark (bench/bench_env.py) to measure whole-machine throughput. It defaults to a 10-second sustained-throughput window with one spawn-started worker per usable NUMA node, binding CPU affinity and future memory allocations before any heavy runtime import, and aggregating actual completed env.step() transitions across workers over multiprocessing Pipes. The legacy single-process --steps latency path, comparisons, batch sweeps, and JSONL output are preserved. NUMA discovery/placement is extracted into a new stdlib-only motrix_rl/numa.py shared by both the benchmark and the FastSAC async trainer (whose fastsac/async_impl/numa.py and topology.py now delegate to it). This partially addresses issue #124 (auto/GPU-local node selection and bench_physics unification are explicitly out of scope).
Changes:
- Add shared
motrix_rl/numa.py(NUMA discovery, CPU affinity, libnumaset_mempolicy-based memory policy) and refactor FastSAC async NUMA code to use it. - Add
bench/env_benchmarkpackage (runner.py,workload.py) implementing synchronized multi-worker duration measurement and the preserved step-latency path; slim downbench/bench_env.pyto CLI/reporting. - Add behavior tests (bench CLI/validation, NUMA discovery/placement, duration window/aggregation, spawn-worker orchestration) and a
bench/README.md.
| File | Description |
|---|---|
| motrix_rl/src/motrix_rl/numa.py | New stdlib-only NUMA discovery/placement module (docstring slightly overstates import-safety). |
| motrix_rl_builtin/src/.../fastsac/async_impl/numa.py | Removes local NUMA helpers; delegates to shared system_numa; spawn_placement now snapshots/restores exact memory policy. |
| motrix_rl_builtin/src/.../fastsac/async_impl/topology.py | Switches node/CPU lookups to system_numa. |
| bench/env_benchmark/runner.py | Window measurement, spawn-worker orchestration, timeouts/cleanup, and legacy step mode. |
| bench/env_benchmark/workload.py | Lazy runtime import and shared env/action preparation with comparison checks. |
| bench/env_benchmark/__init__.py | New package marker/docstring. |
| bench/bench_env.py | CLI reworked for --duration/--steps/--numa, validation, and duration/step reporting. |
| bench/README.md | Documents usage, counting semantics, and multi-NUMA validation. |
| tests/test_bench_*.py, motrix_rl_builtin/tests/test_numa.py | New behavior tests for CLI, NUMA, duration windows, and spawn orchestration. |
| motrix_rl_builtin/tests/test_fastsac_async_multi.py | Updated imports/monkeypatches to the shared numa module. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Low-level ctypes set_mempolicy syscalls and multi-process spawn/window orchestration depend on real multi-NUMA hardware behavior that cannot be fully verified from the diff, so final human review is warranted.
2 open findings
2 resolved since last review
🧠 Review effort: Balanced

Summary
Scope
Related to #124. This PR intentionally expands beyond placement-only benchmarking to include cross-NUMA environment sharding. Auto/GPU-local node selection and unifying bench_physics placement are not included; this does not fully close #124.
Validation
G1 WBT dance sustained throughput
Each case: 10 seconds, 30 warmup calls, fixed random actions; num_envs is the total batch. Single run per configuration, not a repeated statistical benchmark.
CPU and memory binding succeeded for both workers. These measurements cover env.step(), not policy inference or training data transport.