Dtingdahl/datagen collector port - #1248
Draft
david-tingdahl-nvidia wants to merge 11 commits into
Draft
david-tingdahl-nvidia wants to merge 11 commits into
david-tingdahl-nvidia wants to merge 11 commits into
Conversation
Add num_rerenders_on_reset field to IsaacLabArenaManagerBasedRLEnvCfg with default value of 5 to prevent stale camera frames after reset. Implement CallbackRecorderTerm, a generic IsaacLab RecorderTerm subclass that forwards lifecycle callbacks (post-step, pre-reset, close) to externally-supplied handler functions. This provides correctly-timed hooks without requiring a new RecorderTerm subclass for each use case. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
isaaclab.managers/isaaclab.envs need a running SimulationApp (omni.timeline is only importable once Kit has booted), so these tests were not actually sim-free as originally written. Rewrite both to use the codebase's established run_function_with_persistent_simulation_app pattern (see isaaclab_arena/tests/test_task_registry.py).
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
…SON loading Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
…ers params Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
…t_runner.main Annotate the datagen_collector_factory parameter with the same type signature used in run_execution.execute_experiment: Callable[[ArenaRunCfg, gym.Env], DatagenCollectorBase] | None. Add required imports to TYPE_CHECKING block to support the annotation, matching the import convention already used in run_execution.py. This fixes the inconsistency flagged in code review (same issue class as Task 5). Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
Task 1 and Task 2's test files were never run through pre-commit as their own diffs; running it across everything this branch touched (Task 7's verification step) caught black reformatting and an isort ordering issue. No behavior change -- all 10 affected tests still pass. Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
…r, MISSING default Restore the 7 pre-existing build_and_run/execute_experiment/_seed_cfg_for_rebuild tests that a prior datagen task accidentally dropped when it replaced test_run_execution.py, wrapping all 9 tests (7 restored + 2 datagen) in the _test_/test_ + run_function_with_persistent_simulation_app pattern now required because run_execution imports isaaclab.managers.recorder_manager at module level, and updating fakes for _build_environment_from_cfg/build_and_run to accept the datagen_collector_factory parameter added since. Fix _with_datagen_recorder_term to merge recorders_cfg after datagen_recorders_cfg so an already-configured run's recorder settings win over datagen_recorders_cfg's inherited base-class defaults, and add a regression test asserting a pre-set field survives the merge. Change CallbackRecorderTermCfg.build_handlers's default from None to MISSING so an unset field fails with IsaacLab's own clear error instead of a TypeError deep inside RecorderTerm construction. Signed-off-by: David Tingdahl <dtingdahl@nvidia.com>
| decimation: int = 8 | ||
| wait_for_textures: bool = False | ||
|
|
||
| # Force extra RTX sensor refreshes after every reset. IsaacLab's own default (0) |
Collaborator
Author
There was a problem hiding this comment.
Let's remove. try to set it downstream.
| """Variation values applied when the environment is compiled.""" | ||
|
|
||
| datagen: dict[str, Any] | None = field(default=None) | ||
| """Per-run datagen collection config (output_dir, cameras, ...), or None to disable |
Collaborator
Author
There was a problem hiding this comment.
shorten this comment. other comments are single line. keep it 1-2
| from isaaclab_arena.recording.callback_recorder_term import CallbackRecorderTermHandlers | ||
|
|
||
|
|
||
| class DatagenCollectorBase(ABC): |
Collaborator
Author
There was a problem hiding this comment.
This is more general than datagen. let's find a more generic name. this applies to all usages of "datagen"
| if env_cfg.recorders is not None: | ||
| env_cfg.recorders.dataset_filename = f"dataset_{cfg.name}" | ||
| if datagen_collector_factory is not None and cfg.datagen is not None: | ||
| # ArenaEnvBuilder only sets env_cfg.recorders when mimic is disabled, so a |
Collaborator
Author
There was a problem hiding this comment.
remove comment. assert string is enough
| # ArenaEnvBuilder only sets env_cfg.recorders when mimic is disabled, so a | ||
| # requested datagen collector would silently never be invoked in mimic mode. | ||
| assert not cfg.environment_builder.mimic, ( | ||
| f"Run '{cfg.name}' requests datagen collection but mimic mode never sets env_cfg.recorders," |
Collaborator
Author
There was a problem hiding this comment.
can't we directly check env_cfg.recorders somewhere instead of dragging mimic into this?
| build_handlers time -- see run_execution.py's usage). | ||
| """ | ||
|
|
||
| def on_pre_reset(pre_reset_env: Any, env_ids) -> None: |
Collaborator
Author
There was a problem hiding this comment.
why is this defined inline? why not a static function in DatagenCollectorBase?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Short description of the change (max 50 chars)
Detailed description