Repository navigation
fix(fastsac): preserve final observations and stabilize actor - #120
Conversation
b840c72 to
b00ecd4
Compare
8f47ac2 to
d53ace6
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It makes a broad, high-risk change spanning an RL algorithm semantics shift (Q-min aggregation), a new cross-process sparse-payload transport, and replay-buffer/env-contract changes that warrant human validation.
1 open finding
What changed in this PR
This PR hardens the FastSAC trainer in two ways. First, it introduces a backend-neutral final-observation contract: ArrayEnv captures pre-reset observations of time-limit-truncated rows (ArrayEnvState.final_obs, implemented by ManagerEnv), which the async single-collector path threads through a new sparse FinalObsStorage transport and replay-buffer terminal-row storage so timeout bootstrapping uses the true episode-ending observation instead of the next episode's reset observation. Second, it changes the actor objective to use min-over-critic-heads Q aggregation (keeping Gaussian means unclamped), and replaces the per-call metrics dict / CUDA-graph _own machinery with a WindowMetrics accumulator drained at logging boundaries.
Changes:
- Add
FinalObs/_capture_final_obsenv contract and wire sparse final-observation payloads through ring transport, replay buffer, collector, and learner (async single-collector + multi-shard merge). - Refactor FastSAC env wrappers to return
FastSacStepResult; makeFastSacAgent.update()returnNoneand accumulateWindowMetricsinstead of a metrics dict. - Switch actor Q aggregation from mean to min across critic heads; rename ring cursors to
*_completed_count/*_issued_count; add design docs and extensive regression tests.
| File | Description |
|---|---|
| motrix_env_core/src/.../array/env.py | Adds FinalObs, _capture_final_obs hook, and pre-reset capture in step lifecycle |
| motrix_env_core/src/.../numba/manager/env.py | ManagerEnv final-obs capture via dedicated observe slot |
| motrix_env_core/src/.../numba/manager/observations.py | ObsTerm docstring update (contains RNG-restore inaccuracy) |
| motrix_rl_builtin/.../fastsac/wrap*.py | FastSacStepResult dataclass; np/torch wrappers return it; adds max_episode_steps |
| motrix_rl_builtin/.../fastsac/buffer.py | Sparse terminal-row storage + final-obs bootstrap in sampling |
| motrix_rl_builtin/.../fastsac/async_impl/transport/ring.py, ipc_ring.py | FinalObsStorage/FinalObsPayload; cursor renames; row-capacity backpressure |
| motrix_rl_builtin/.../fastsac/async_impl/collector.py, learner.py, worker.py, train.py | Plumb final payloads through collect/drain/ingest and store allocation |
| motrix_rl_builtin/.../fastsac/agent.py, window_metrics.py | Q-min aggregation; remove _own/metrics dict; WindowMetrics |
| motrix_rl_builtin/.../fastsac/sync/train.py | Consume step result and window metrics |
| motrix_rl_builtin/tests/*, motrix_env_core/tests/* | New/updated regression tests for final-obs, transport, buffer, actor |
| wiki/design/*.md | New final-observation design doc + transport payload section |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Summary
ArrayEnvState.final_obs; expose non-consuming adapter access. No RNG backup/restore: noisy final-observation capture consumes the shared random stream normally.Validation
prek run --all-filespassed.Completed ablation experiments
Task:
g1-wbt-dance; 2,048 environments; seed 1; single async collector; strict UTD approximately 4; gradient clipping disabled. CPU environment rollout uses Numba TBB; actor inference and learning use separate GPUs for the two concurrent runs.Definitions:
Both new runs use the same Env refactor baseline. A1 disables F1/F2; A4 disables F2 only. The PR now adopts A4's algorithm settings: Q min + final-observation bootstrap, without a hard mean clamp.
Returns are training log-window statistics, not a separate held-out policy evaluation. Deep drops are grouped relative to a rolling 20-point median baseline; counts describe numerical depth, not a definitive causal classification.
A1 troughs occurred at approximately 78.9k, 91.4k and 119.2k iterations (returns -1.54, 2.60 and -2.51); recovery to 90% of the pre-drop baseline took approximately 8.3k, 2.0k and 7.3k iterations. Its mature-period Q peak was 17.35 and minimum episode length 19.45. A4 had a Q peak of 10.94 and minimum episode length 382.30. A1's deep drops did not require positive entropy, so a positive-entropy-only pathology detector would miss them.
Interpretation and limitations
Remaining risks / scope
The independent final-event queue is removed. Sparse final payloads are attached to transition-ring slots and share publication/acknowledgement; collector owns one pending completed step on backpressure. Learner ingests transition + final rows together, including during warmup, without stamp joins or replay private-field access. Replay reclaims rows by live transition references and grows geometrically rather than assuming episode-length spacing.
Refactor validation: 105 builtin tests passed, including spawned producer/slow-consumer ownership, both wrap boundaries, sparse/main-ring backpressure, no duplicate step on retries, CPU/CUDA warmup ingest, replay growth/reuse and n-step bootstrap. Host and CUDA-IPC short-episode training both completed 300 iterations (64 envs, 20-step limit, learning_starts=100) with nonzero final-observation diagnostics. Hooks passed. Full 200k training was not rerun after transport refactoring.
Remaining scope: sync/multi-collector final-observation support is not implemented. Sparse replay can reach dense worst-case memory under every-step truncation; rare growth synchronizes CUDA. Diagnostic clipping limitations and single-seed causal limitations remain documented.
Details: final-observation contract, slot payload transport.
Public text omits internal addresses, accounts and absolute server/workspace paths.