Repository navigation
refactor: retire motrix_legacy — owner-layer decomposition - #194
Conversation
|
这版不能合并。#177 要求退役的是跨层 compatibility bag，本 PR 只是把 按仓库当前架构，这个 PR 的核心问题有四个：
正确执行方案不要再做 rename-only，而是按 owner layer 拆：
结论：这版不是 partial fix，而是 rename-only。请按上面的 owner-layer 方案重做，不建议在这个 PR 上继续修补。 |
14bcede to
d7b6919
Compare
TATP-233
changed the title
Decompose the `motrix_legacy` compatibility bag into three owner-layer
mechanisms, following the existing `reward_motrix` sidecar pattern:
**Reward** — route all backend-specific rewards through `reward@reward_motrix`
Hydra defaults. Motrix reward YAML files already existed; task YAMLs now
reference them via defaults list instead of embedding reward_config in
the bag. `extract_reward_config()` remains the single reward resolution path.
**Algo** — new `algo_motrix` sidecar config group (4 files under
`conf/{ppo,offpolicy}/algo_motrix/`). `BackendAdapter.resolve_algo_dict()`
merges the sidecar over `cfg.algo` and returns a new dict — cfg.algo is
never mutated. Training scripts read from the resolved dict. Supports
`applies_to.algo` filtering (e.g., SAC-only overrides for Go1 offpolicy)
and CLI explicit-key precedence.
**Env** — backend-specific env overrides in `backend_profile` section of
task YAML, using flat fields that match env cfg structure (e.g.,
`commands.vel_limit`, `control_config.action_scale`, `gait_phase_init_mode`).
Applied via `registry.make` env_cfg_override — no `apply_*()` methods.
Removed:
- `motrix_legacy` config key from all 5 task YAMLs
- `LegacyMotrixProfile` dataclass + `apply_legacy_motrix_profile()` from Go1/Go2
- `backend_overrides: dict` + `apply_backend_overrides()` from G1
- `_use_legacy_motrix_baseline()` — replaced by direct `backend_type` check
- `adapt_legacy_go1_motrix_obs_if_needed()` from off-policy worker (dead code)
- Runtime mutation of `cfg.algo` by BackendAdapter
Updated:
- Commands.vel_limit → proper dataclass field (enables registry.make override)
- Training scripts use resolved algo dict instead of cfg.algo
- Tests assert resolved config values, not bag structure
- Architecture docs updated
d7b6919 to
4511aa3
Compare
|
补一条更具体的阻塞意见：这版不仅还有 backend leak，而且已经引入了最终生效超参数偏差。
我这里的要求很明确：
在这个问题解决之前，这个 PR 还不能算达到要求。 |
Address review feedback on rev 2:
- Delete conf/{ppo,offpolicy}/algo_motrix/ directories entirely
- Delete BackendAdapter.resolve_algo_dict() — training code no longer
interprets backend-specific algo config; cfg.algo comes straight from
Hydra compose
- Fold all old motrix_legacy.algo_overrides into the base algo section
of each task YAML (Go2 pattern), preserving the exact pre-refactor
resolved hyperparams for the Motrix path:
* Go1 PPO: empirical_normalization, policy, algorithm overrides
* G1 PPO: max_iterations=151 (was drifted to 220), empirical_norm,
obs_groups.actor, policy, algorithm overrides
* Go1 offpolicy SAC: num_envs=4096, max_iterations=2000
* G1 SAC: use_symmetry=true (resolve_sac_use_symmetry in script
still disables it for motrix at the one-liner level)
- Revert training scripts to read cfg.algo directly
- Rewrite tests to assert resolved hyperparam equivalence with the old
Motrix behavior, not the internal bag structure
- Update docs to remove algo_motrix references
|
这版还是不能过，原因不是 这违反了你这轮整改里最关键的约束：重构后最终生效的超参数不能偏离原本已存在的行为。 我确认到的具体回退有：
我这里的结论很明确：
如果团队决定要正式改变 MuJoCo 的默认超参数，那应该是单独 issue / 单独 PR / 单独验证，不应该混在这次 compatibility-layer 退役里。 |
b2898ca to
0b06fad
Compare
|
这版还有一个明确的 contract blocker，所以我这边仍然不能放行。 问题是：你现在把 证据：
但这和当前仓库自己声明的 config contract 是冲突的。你刚改过的
也就是说，现有 contract 是“显式 override 要被尊重”，不是“显式 override 一律非法”。 这不是实现偏好问题，而是一个 user-facing breaking change：
在这版里都被改成了 hard failure，而 issue #177 本身并没有授权顺手废掉这个输入 contract。 我的结论：
在这个点修正之前，我这边仍然认为 PR #194 没达到要求。 |
|
我按这个方向补了一版，commit: 2e7606a。 这次没有把
已同步更新：
验证：
|
|
已把分支同步到最新 处理点：
验证：
|
|
只看当前这版修改本身，我认为还需要做几处定点收口，问题主要不在超参数内容，而在整体设计边界还不够干净。
补一句平衡判断：我 spot check 了 Go1 offpolicy / Go1 PPO / G1 PPO，这版 owner YAML 基本已经把我之前指出的超参数漂移收回来了。所以现在不建议再大改 task/algo/reward 的组织方式，重点就是把上面这几处“隐藏身份字段 + runtime mutation + backend 名称泄漏 + 测试固化内部实现”收干净。 |
|
已按最新 review 做定点收口，commit: 152e838。 处理点：
验证：
|
|
这次我重新按最新提交看了一遍，前面几轮卡住的设计问题基本都已经收干净了，这版我这边没有新的 blocker。 我确认到的关键点是：
现在还能看到的 所以按当前这版修改本身来看，我这边可以过。 |
…gacy refactor: retire motrix_legacy — owner-layer decomposition
…gacy refactor: retire motrix_legacy — owner-layer decomposition
…gacy refactor: retire motrix_legacy — owner-layer decomposition
Summary
Closes #177. Decomposes the
motrix_legacycompatibility bag into three owner-layer mechanisms:reward@reward_motrixHydra defaults. No morereward_configembedded in the bag.algo_motrixsidecar config group (4 YAML files).BackendAdapter.resolve_algo_dict()merges the sidecar overcfg.algo— cfg.algo is never mutated.backend_profilewith flat typed fields. Applied byregistry.make— noapply_*()methods.Removed
motrix_legacyconfig key from all 5 task YAMLsLegacyMotrixProfile+apply_legacy_motrix_profile()from Go1/Go2backend_overrides: dict+apply_backend_overrides()from G1_use_legacy_motrix_baseline()— replaced by directbackend_typecheckadapt_legacy_go1_motrix_obs_if_needed()from off-policy workercfg.algoAdded
conf/{ppo,offpolicy}/algo_motrix/*.yamlsidecar configsBackendAdapter.resolve_algo_dict()— pure function, returns resolved dictCommands.vel_limitas proper dataclass field21 files changed, 348 insertions(+), 434 deletions(-).
Test plan
make test— 344 passed, 12 skipped, 0 failuresmotrix_legacy,legacy_motrix,adapt_legacy,apply_backend_overridescfg.motrix_legacy.*structurecfg.algois NOT mutated afterresolve_algo_dict()