Feature/nvblox next datagen slim#883
Conversation
![arena-review-bot[bot]](https://avatars.githubusercontent.com/u/195794629?s=60&v=4){kind=small}
| # leaves camera buffers stale on the first frame of every episode after the | ||
| # first, so the previous episode's final rendered frame leaks in (corrupting | ||
| # RGB/depth/flow during datagen). See IsaacLab-Arena #339. | ||
| num_rerenders_on_reset: int = 5 |
There was a problem hiding this comment.
🟡 Warning — This flips the reset behavior for every Arena environment, not just datagen. IsaacLabArenaManagerBasedRLEnvCfg is the shared base for all RL/eval envs, so every reset now triggers 5 extra RTX rerenders for everyone — a non-trivial per-reset cost on any camera-enabled run — even though the comment scopes the motivation entirely to datagen ("corrupting RGB/depth/flow during datagen").
The blast-radius/conservative-defaults invariant is that new behavior built for a narrow use case should be opt-in rather than silently changing the default path. Could this instead be applied only on the datagen cfg (e.g. inside prepare_env_cfg_for_datagen, which already mutates the cfg for exactly this purpose) rather than the global base?
Also, the comment says "Force at least one RTX sensor refresh" but the value is 5 — is 5 needed, or would a smaller number (1?) suffice? Worth a NOTE explaining why 5.
🤖 Isaac Lab-Arena Review BotSummaryThis PR wires optional datagen collection into the eval runner via a duck-typed Design, Boundaries & Scope🟡 Findings🟡 Warning: environments/isaaclab_arena_manager_based_env_cfg.py:88 — global Test Coverage
VerdictMinor fixes needed — the datagen wiring is well-structured; please just confirm the global |
Greptile SummaryThis PR wires a datagen collection pipeline into the eval runner, allowing dedicated camera-based HDF5 recording during policy rollouts. It disables the environment's auto-reset during datagen to prevent stale frames leaking across episodes, adds a
Confidence Score: 3/5The rollout and cleanup paths are well-structured, but two gaps in the datagen lifecycle could silently corrupt or drop collected data. The
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant M as main()
participant CP as collector_provider
participant LE as load_env()
participant RP as rollout_policy()
participant DR as _run_datagen_rollout()
participant C as collector
M->>CP: "start_run() [only if datagen_defaults != None]"
loop Per Job
M->>LE: "load_env(disable_auto_reset=is_datagen)"
LE->>LE: prepare_env_cfg_for_datagen() stash reset_terms
LE-->>M: (env, datagen_reset_terms)
M->>CP: create(job_name, job_datagen, env) collector
M->>RP: rollout_policy(collector, datagen_reset_terms)
RP->>DR: _run_datagen_rollout(...)
loop Per Step
DR->>DR: env.step()
DR->>C: on_step()
alt Episode end / cap reached
DR->>C: end_episode(outcome)
DR->>C: resample_cameras()
DR->>DR: env.reset()
end
end
DR-->>RP: return
RP->>C: finalize(env)
M->>C: close(env)
M->>CP: on_job_finished()
end
M->>CP: "finish_run() [only if datagen_defaults != None]"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant M as main()
participant CP as collector_provider
participant LE as load_env()
participant RP as rollout_policy()
participant DR as _run_datagen_rollout()
participant C as collector
M->>CP: "start_run() [only if datagen_defaults != None]"
loop Per Job
M->>LE: "load_env(disable_auto_reset=is_datagen)"
LE->>LE: prepare_env_cfg_for_datagen() stash reset_terms
LE-->>M: (env, datagen_reset_terms)
M->>CP: create(job_name, job_datagen, env) collector
M->>RP: rollout_policy(collector, datagen_reset_terms)
RP->>DR: _run_datagen_rollout(...)
loop Per Step
DR->>DR: env.step()
DR->>C: on_step()
alt Episode end / cap reached
DR->>C: end_episode(outcome)
DR->>C: resample_cameras()
DR->>DR: env.reset()
end
end
DR-->>RP: return
RP->>C: finalize(env)
M->>C: close(env)
M->>CP: on_job_finished()
end
M->>CP: "finish_run() [only if datagen_defaults != None]"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if collector_provider is not None and datagen_defaults is not None: | ||
| collector_provider.start_run(eval_jobs_config, args_cli.datagen_description, args_cli.device) |
There was a problem hiding this comment.
start_run() / finish_run() skipped for per-job-only datagen configs
Both lifecycle hooks are guarded by datagen_defaults is not None, but is_datagen (line 309) can be True when only a per-job datagen block exists and the top-level datagen key is absent. In that scenario collector_provider.create() and collector_provider.on_job_finished() run, but start_run() is never called — skipping any run-level initialization (output directories, manifest creation, etc.) — and finish_run() is never called, leaving the run incomplete.
| if num_steps is not None: | ||
| pbar.update(1) | ||
| if num_steps_completed >= num_steps: | ||
| break | ||
|
|
||
| ended_by = _manual_episode_done(env, reset_terms) | ||
| hit_cap = steps_in_episode >= max_episode_length | ||
| if ended_by is not None or hit_cap: | ||
| collector.end_episode(env, outcome=classify_outcome(ended_by)) | ||
| num_episodes_completed += 1 | ||
| if num_episodes is not None: | ||
| pbar.update(1) | ||
| if num_episodes_completed >= num_episodes: | ||
| break |
There was a problem hiding this comment.
num_steps limit is reached
In _run_datagen_rollout, the num_steps check (line 142-143) breaks out of the loop before the episode-end check (line 145). If the step limit is reached mid-episode, collector.on_step() has been called for each frame of the current episode but collector.end_episode() is never called — only collector.finalize(env) runs later via rollout_policy(). Unless finalize internally closes the in-flight episode, the last partial episode's data is silently dropped or left in an inconsistent state.
498d3bf to
89c379a
Compare
Inject nvblox_next datagen collection into eval_runner and policy_runner via an optional collector_provider argument. The provider builds a per-job collector from a job's datagen config block; rollouts then record per-step data, per-episode outcomes, and a run-level provenance manifest. Adds episode outcome classification, a --datagen-description CLI flag, and a max_recorded_frames cap. Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
89c379a to
5afe2b1
Compare
Define the collector interface the evaluation runners drive (on_step, on_episode_end, finalize, close) as an ABC in evaluation/datagen_collector.py, and type rollout_policy's collector argument against it. Implementations live in the collecting package; the runners no longer rely on duck typing. on_episode_end doubles as the pre-reset hook for preparing the next episode's cameras, since the runner calls it before the explicit env.reset() whose RTX rerenders flush new camera poses. Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
eval_runner now takes a datagen_run_manager typed against the new DatagenRunManagerBase ABC. Injecting the manager enables collection for every job (start_run asserts the config configures it), so the per-job is_enabled hook is gone. The manager owns the collector it creates per job: on_job_finished(job, env) closes and records it, so the runner no longer manages collector lifetime or passes the collector back. The per-episode recording cap moves onto the collector interface as cap_episode_length with a no-cap default, and episode outcomes are typed as the EpisodeOutcome literal that classify_outcome produces. Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
- Collapse the datagen rollout onto the standard auto-reset loop; drop _run_datagen_rollout, _manual_episode_done, and the termination stashing. On a done step the finished episode is closed before the post-reset obs is recorded, so the leaked reset frame lands in the next episode where the collector's skip_initial_frames drops it. - Add NoOpDatagenCollector / NoOpDatagenRunManager and default to them, so the runners no longer special-case the no-collection path. - Move env-cfg prep (drop metric recorders) and the camera requirement onto the run manager (prepare_env_cfg, needs_cameras); remove the unused cap_episode_length hook. - Delete episode_outcome (classify_outcome unused after the rewrite). Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
49029f8 to
282e47d
Compare
1 participant
Summary
Short description of the change (max 50 chars)
Detailed description