Repository navigation
Conversation
7d91854 to
4a6da52
Compare
4a6da52 to
9f561c6
Compare
994dae9 to
84e177a
Compare
c76e2ad to
c77a643
Compare
0da7a6f to
85910f0
Compare
c77a643 to
f77291a
Compare
85910f0 to
c7357bc
Compare
f77291a to
ce4c383
Compare
c7357bc to
222faf8
Compare
e63242c to
939f249
Compare
222faf8 to
072b971
Compare
RyanPCo
left a comment
There was a problem hiding this comment.
Automated adversarial review (read-only; findings were checked by a second skeptical pass before posting): 1 medium, 2 low, 4 nit.
| # The MAD spike detector in ModelWrapper.on_after_backward is log-only and never | ||
| # touches gradients; this is the only thing that clips. null = no clipping (the | ||
| # value is picked from the grad-norm curve of a log-only baseline run). | ||
| gradient_clip_val: null |
There was a problem hiding this comment.
medium (scope): The grad-clipping change has nothing to do with deterministic eval, and it changes training defaults. enable_grad_norm defaults to True, so HPT runs used to clip MAD-flagged spikes to the median. With gradient_clip_val: null they now get no clipping at all. Split this (the on_after_backward/on_before_optimizer_step changes, the trainer yaml, and the section 3-4 tests) into its own PR so it can be reviewed and reverted on its own, and say in it that the default goes from spike-clipping to none.
|
|
||
| if not grad_norm_flagged: | ||
| self.grad_norm_history.append(grad_norm_val) | ||
| if grad_norm_flagged and self.trainer.is_global_zero: |
There was a problem hiding this comment.
low (behavior-change): With the MAD clip turned log-only and trainer.gradient_clip_val: null, every HPT config that inherits trainer/default.yaml now trains with no gradient clipping. Before this PR they clipped to the rolling median on spikes. That is a training-dynamics change inside an eval-determinism PR. Either ship a non-null gradient_clip_val for the HPT configs, or state the change prominently in the PR description so reviewers know existing recipes lose spike protection.
| # Advance the pass counter once per forward_eval call (after the batch, | ||
| # so the prompts built for this batch and the noise drawn for it share a | ||
| # seed). ``reset_eval_pass_counter`` rewinds it at on_validation_start. | ||
| self._eval_pass_counter = int(getattr(self, "_eval_pass_counter", 0)) + 1 |
There was a problem hiding this comment.
low (naming): _eval_pass_counter advances once per forward_eval call (per batch) and is reset once per validation pass. Meanwhile reset_eval_pass_counter's docstring uses "pass" for the whole validation pass. Rename it to count batches (e.g. _eval_step, _eval_step_seed(), reset_eval_step()) and update the EVAL_BASE_SEED module comment so readers don't assume one seed per pass.
| return ( | ||
| self.eval_base_seed | ||
| + self._eval_rank() * EVAL_RANK_STRIDE | ||
| + int(getattr(self, "_eval_pass_counter", 0)) |
There was a problem hiding this comment.
nit (simplification): _eval_pass_counter is never initialised in HPT.init, so both reads go through int(getattr(self, "_eval_pass_counter", 0)). Set it to 0 in init (and in the test stub) and use the attribute directly.
| self._eval_pass_counter = 0 | ||
|
|
||
| @staticmethod | ||
| def _eval_rank() -> int: |
There was a problem hiding this comment.
nit (simplification): The RANK/LOCAL_RANK/SLURM_PROCID fallback only runs when no process group exists, which under Lightning means only in tests (test_eval_determinism monkeypatches RANK). Checking LOCAL_RANK before SLURM_PROCID would also give two nodes the same seed if the fallback ever ran. Return get_rank() when initialized, else 0, and have the test monkeypatch _eval_rank.
|
|
||
| # The MAD detector is log-only: it never rescales gradients (actual | ||
| # clipping is Lightning's fixed ``trainer.gradient_clip_val``), so every | ||
| # step goes into the rolling history. The old code excluded flagged |
There was a problem hiding this comment.
nit (comments): This comment narrates the previous implementation ("The old code excluded flagged steps..."). Trim it to the reason, e.g. # Log-only detector: every step enters the rolling window; clipping is trainer.gradient_clip_val.
| @@ -0,0 +1,435 @@ | |||
| """Deterministic evaluation (lane B). | |||
There was a problem hiding this comment.
nit (comments): Drop "(lane B)": it refers to an internal plan that means nothing to repo readers.
stem input_dim, infer_ac_dims, act_dim and enc-dec output_dim now interpolate
robomimic_model.dims.{<emb>.proprio,<emb>.action,action_width}. Resolved values
are unchanged (pinned by tests/unit/test_model_dims.py). Pi configs untouched.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MWtR1x5tjREwoxQZAnUGrh
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01BbQNNaMAbTNMypegmJkv4s
…only grad-norm MAD Validating one checkpoint twice now gives identical prompts, actions and metrics. * HPT gets a per-pass RNG seed, base + rank * stride + pass counter, where the counter advances once per forward_eval call and is rewound by ModelWrapper.on_validation_start. forward_eval builds a torch.Generator from it and threads it down HPTModel.forward -> DenoisingPolicy.forward -> sample_action, and forks/seeds the global RNG around the BC val-loss call (DenoisingPolicy.predict draws its own noise and timestep, out of reach of any generator kwarg). Every generator kwarg defaults to None, so training and all other callers are untouched. The val-loss seed is offset by EVAL_LOSS_STRIDE rather than by 1: a torch.Generator seeded with S draws the same stream as a global manual_seed(S), so +1 would have handed batch i's val loss exactly the noise batch i+1 samples with. eval_base_seed is a real __init__ kwarg, so a second eval pass can be made to draw different noise on purpose. * _build_prompts picks annotations with that same seeded RNG whenever the algo is in eval mode, regardless of annotation_sampling_mode, so eval keeps prompt variety instead of collapsing to sample[0]. Train mode is unchanged. * The MAD grad-norm spike detector in on_after_backward is now log-only: it still logs the raw norm, the threshold, the flag and the [GRAD_NORM_SPIKE] line, but never rescales gradients, and every step (flagged or not) goes into the rolling median/MAD window. Fixed clipping moves to Lightning: trainer/default.yaml gains gradient_clip_val (null for now) and gradient_clip_algorithm: norm. The on_before_optimizer_step hook goes with it: Precision._after_closure calls that hook BEFORE _clip_gradients, so "policy_grad_norms_clipped" was the pre-clip norm under another name -- bit-identical to policy_grad_norms_raw, and no more informative once gradient_clip_val is set. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01VBmhpVy7ZcMtsBJWh38cJp
072b971 to
fba6218
Compare
|
Review findings: status after the fixup pass. The fixes are squashed into this PR's commits, and the whole stack was force-pushed. Fixed
Not fixed
|

Validating one checkpoint twice now gives identical prompts, actions and
metrics.
the counter advances once per forward_eval call and is rewound by
ModelWrapper.on_validation_start. forward_eval builds a torch.Generator
from it and threads it down HPTModel.forward -> DenoisingPolicy.forward ->
sample_action, and forks/seeds the global RNG around the BC val-loss call
(DenoisingPolicy.predict draws its own noise and timestep, out of reach of
any generator kwarg). Every generator kwarg defaults to None, so training
and all other callers are untouched.
algo is in eval mode, regardless of annotation_sampling_mode, so eval keeps
prompt variety instead of collapsing to sample[0]. Train mode is unchanged.
still logs the raw norm, the threshold, the flag and the [GRAD_NORM_SPIKE]
line, but never rescales gradients, and every step (flagged or not) goes
into the rolling median/MAD window. Fixed clipping moves to Lightning:
trainer/default.yaml gains gradient_clip_val (null for now) and
gradient_clip_algorithm: norm.
Co-Authored-By: Claude Opus 5 (1M context) [email protected]
🤖 Generated with Claude Code
https://claude.ai/code/session_01HjCQBqQCr2igFEMR7yUovw