Repository navigation
Conversation
Signed-off-by: Xinjie Yao <xyao@nvidia.com>
Signed-off-by: Xinjie Yao <xyao@nvidia.com>
 [Medium impact] The changes reach the typed experiment-runner path for VLM-controlled DROID runs. Fix discovery of the new short policy names before merging. Findings
SummaryAdds YAML-driven construction for the three VLM execution strategies and a DROID pick-and-place example.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Experiment YAML] --> B[Resolve policy selector]
B -->|Dotted class path| C[Import VLM policy module]
B -->|Short name in fresh process| X[Name not discovered]
C --> D[Build typed policy configuration]
D --> E[Create observation and DROID action adapters]
E --> F[Create owned inference client]
F --> G[Execute step, chunk, or goal strategy]
G --> H[Close policy and environment]
Reviews (1) · Last reviewed commit: "Use registry_name for the DROID IK embod..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @register_policy | ||
| class VLMStepPolicy(VLMPolicy, PolicyBase[VLMStepPolicyCfg]): | ||
| """Predict one action per environment from fresh observations on every step.""" | ||
|
|
||
| name = "vlm_step" |
There was a problem hiding this comment.
A fresh runner cannot load vlm_step, vlm_chunk, or vlm_goal by name. ensure_assets_registered() never imports this module, so a YAML policy such as type: vlm_step fails the loader's dotted-path assertion before the experiment starts. The selector test misses this because it imports all three policies first.
Add lazy discovery for these names and test name-based loading without first importing the policies.
![arena-review-bot[bot]](https://avatars.githubusercontent.com/u/195794629?s=60&v=4){kind=small}
| return True | ||
|
|
||
|
|
||
| @pytest.mark.with_cameras |
There was a problem hiding this comment.
🟡 This camera test won't get cameras in CI
The "Run VLM agent policy tests" step runs pytest isaaclab_arena_vlm_agent/tests/ in one process with no -m filter (ci.yml:318-321, uv-ci.yml:198-203). So with_cameras doesn't move this test to a separate phase. test_droid_vlm_adapters_simulation.py runs first and starts the shared SimulationApp without cameras. After that, get_persistent_simulation_app prints a warning and ignores enable_cameras=True. This test then builds a camera environment on an app that was started without camera rendering.
Could this test run without cameras instead (policy.observation_modality=state, environment.enable_cameras=false)? The unit test test_configured_execution_and_partial_reset already covers finding camera terms. The other fix is to split the VLM CI step by marker, but that's a workflow change.
|
|
||
|
|
||
| @pytest.mark.with_cameras | ||
| @pytest.mark.parametrize("mode", ["step", "chunk", "goal"]) |
There was a problem hiding this comment.
🔵 Three full scene builds for one end-to-end check
Each case builds the maple-table scene with DROID, an HDR and cameras. Existing tests already cover most of what differs between the modes:
- Step vs chunk behavior runs without the simulator in
test_configured_execution_and_partial_resetandtest_droid_vlm_adapters.py::test_policy_execution_and_partial_resets. - Live absolute-pose decoding and gripper measurements run in
test_droid_absolute_pose_adapters.
The goal case covers the rest: a YAML run building the registered policy in build_and_run, the total_action_dim check, the Robotiq gripper measurements, and closing the policy's own client. Could we keep only that case?
| @pytest.mark.parametrize("mode", ["step", "chunk", "goal"]) | |
| @pytest.mark.parametrize("mode", ["goal"]) |
| from isaaclab_arena_vlm_agent.response_errors import InvalidAgentResponseError | ||
|
|
||
|
|
||
| class AbsoluteStepActionAdapter(AgentActionAdapter): |
There was a problem hiding this comment.
🔵 A second way to run single-step DROID control
In #1482, single-step absolute control was settled as AbsoluteChunkActionAdapter(1). Its docstring still says "use chunk_size=1 for one prediction per step", and test_droid_vlm_adapters.py::_policy runs "step" as VLMChunkPolicy with that adapter. This class brings a step adapter back, and its body matches AbsoluteGoalActionAdapter line for line.
Does vlm_step need a DROID preset? If not, the example could drop the step run, or use vlm_chunk with chunk_size: 1. If we keep it, could the three absolute adapters share the index lookup and decoding? Then the chunk_size=1 hint in the chunk docstring can go.
| vlm_embodiment_adapter: str | None = "droid" | ||
| """Named action and prompt preset.""" | ||
|
|
||
| chunk_size: int = 1 |
There was a problem hiding this comment.
🔵 Should chunk_size be required?
With the default of 1, vlm_chunk does the same thing as vlm_step, only with a different response schema. AbsoluteChunkActionAdapter requires chunk_size for that reason (from the #1482 review), and max_steps_per_goal in VLMGoalPolicyCfg is required the same way. The tests that build VLMChunkPolicyCfg without it would need to pass chunk_size.
| chunk_size: int = 1 | |
| chunk_size: int = field(kw_only=True) |
| vlm_embodiment_adapter: str | None = None | ||
| """Named action and prompt preset; droid supports the droid_abs_ik embodiment.""" | ||
|
|
||
| observation_modality: str = "image_state" |
There was a problem hiding this comment.
🔵 Enum for observation_modality?
The three allowed values appear in __post_init__ and again in the dict in VLMPolicy.__init__. Gr00tRemoteClosedloopPolicyCfg.scheduler uses a str Enum with a method that returns the class, and the typed loader converts YAML strings to enums. So the YAML can stay observation_modality: image_state:
class ObservationModality(str, Enum):
"""Observations included in each VLM request."""
IMAGE = "image"
STATE = "state"
IMAGE_STATE = "image_state"
def get_adapter_type(self) -> type[VLMObservationAdapter]:
"""Return the observation adapter class this modality selects."""
return {
ObservationModality.IMAGE: ImageObservationAdapter,
ObservationModality.STATE: StateObservationAdapter,
ObservationModality.IMAGE_STATE: ImageStateObservationAdapter,
}[self]|
|
||
| def __init__(self, embodiment: EmbodimentBase): | ||
| observation_cfg = embodiment.get_observation_cfg() | ||
| def __init__(self, embodiment: EmbodimentBase | None = None): |
There was a problem hiding this comment.
🔵 Two places to read camera terms from
VLMPolicy.__init__ now builds this adapter without an embodiment, so only tests still pass one. In #1482, StateObservationAdapter was changed to read the live managers, because callbacks can change the built environment. The embodiment path here can still disagree with the environment.
Could we drop the embodiment parameter here and in ImageStateObservationAdapter, and read env.unwrapped.cfg.observations on each call? That also removes the camera_keys value that is filled in on first use.
| ) | ||
| ) | ||
|
|
||
| def close(self) -> None: |
There was a problem hiding this comment.
🔵 close() can be smaller
build_and_run builds a new policy for each rebuild and closes it once through close_run_resources. Clearing pending commands and the _closed flag only guard a second close(), which nothing in Arena makes. The test's double close() and its _chunks/_goals checks after it could go too. Reaching into self._inference_backend.client also suggests InferenceBackend should have its own close().
def close(self) -> None:
"""Close the inference client when this policy created it."""
if self._owns_inference_backend:
self._inference_backend.client.close()|
|
||
| registry_name = "droid_abs_ik" | ||
|
|
||
| def get_action_cfg(self): |
There was a problem hiding this comment.
🔵 action_config and get_action_cfg() disagree
This is the only embodiment that overrides get_action_cfg, so self.action_config still has relative mode with scale=0.5 while the environment gets absolute targets. ArenaEnvBuilder calls configure_physics_backend for every backend before get_action_cfg() (arena_env_builder.py:334 and :401). So the switch could happen where the DROID classes already adjust their action config, after the Newton IK override. That keeps one config object and drops the copy:
def _configure_physics_backend(self, backend: PhysicsBackend) -> None:
"""Apply the backend's IK settings, then accept unscaled absolute pose targets."""
super()._configure_physics_backend(backend)
self.action_config.arm_action.controller.use_relative_mode = False
self.action_config.arm_action.scale = 1.0
🤖 Isaac Lab-Arena Review BotSummaryThis PR registers Design, Boundaries & Scope
Findings🟡 Warning: Test Coverage and CI Cost
VerdictMinor fixes needed |
Signed-off-by: Xinjie Yao <xyao@nvidia.com>
Add experiment-runner support for VLMStepPolicy, VLMChunkPolicy, and VLMGoalPolicy. Add a DROID pick-and-place experiment with flat policy configuration and one Run per execution strategy.
Why this change?
The existing VLM policies require callers to construct inference, observation, and action adapters in Python. Allow the experiment runner to construct these policies from typed YAML, using the DROID adapters introduced in #1482.
What changes?
vlm_embodiment_adapter: droidto supply absolute-pose action adapters, Robotiq gripper measurements for goal completion, and strategy-specific default prompts.droid_abs_ikfor absolute pose control and use the action manager's dimension when validating goal commands.Usage example
Before, construct the dependencies in Python:
After, declare the policy in an Experiment Definition:
Use
environment.embodiment: droid_abs_ikwith cameras enabled. The example YAML also includes step and chunk policies. SetNV_API_KEYfor the configured internal endpoint; credentials remain outside the Experiment Definition.Validation