Add vla_sim: prompt-driven cube stacking with a vision-language-action policy - #815
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a complete ChangesVLA simulation rollout
Possibly related issues
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (3 passed)
Comment |
|
Consider whether the change should land upstream in Overlapping files
|
fdavulcu
force-pushed
the
20588-vla-sim
branch
2 times, most recently
from
July 28, 2026 15:05
5d12643 to
1fae55a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
src/vla_sim/docker/test_serve_policy.py (2)
311-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlso close the server socket in cleanup.
shutdown()stops the serve loop but leaves the listening socket open; each test in this class leaks one FD.♻️ Suggested cleanup
threading.Thread(target=httpd.serve_forever, daemon=True).start() self.addCleanup(httpd.shutdown) + self.addCleanup(httpd.server_close) return http.client.HTTPConnection("127.0.0.1", httpd.server_address[1])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/docker/test_serve_policy.py` around lines 311 - 315, Update the cleanup registered in _start to close the HTTP server socket as well as stopping serve_forever, using the server’s existing shutdown and server_close lifecycle methods so each test releases its listening file descriptor.
207-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test reaches the Hugging Face code path.
"/nonexistent/checkpoint"isn't a directory and contains/, soload_checkpoint_fileroutes it tohf_hub_download. It passes today only because the repo id fails client-side validation, so theexcept Exceptionbranch produces the expectedValueError. A checkpoint string with no/(e.g."nonexistent-checkpoint") exercises the same assertion without depending on hub-client behavior or risking a network call in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/docker/test_serve_policy.py` around lines 207 - 210, Update test_unresolvable_fps_raises to use a checkpoint identifier without a slash, such as “nonexistent-checkpoint”, so resolve_fps exercises the local missing-checkpoint path without invoking Hugging Face hub handling or network-dependent behavior.src/vla_sim/docker/serve_policy.py (2)
460-472: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
stateof a non-sequence type yields a 500, not a 400.
payload["state"]is used directly inlen()and thentorch.tensor(...). A scalar or string state is a caller error but escapes asTypeError→ 500. A type check alongside the existingimagescheck keeps the 4xx/5xx split honest.🛡️ Suggested guard
if not isinstance(payload["images"], dict): raise ValueError("/infer payload 'images' must map camera names to images") + if not isinstance(payload["state"], list): + raise ValueError("/infer payload 'state' must be a list of joint positions")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/docker/serve_policy.py` around lines 460 - 472, Add an explicit sequence-type validation for payload["state"] alongside the existing payload["images"] check, before calling len() or converting it with torch.tensor. Raise the endpoint’s existing ValueError-style client error for scalar or string states, while preserving the expected-dimension validation for valid sequences.
236-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
rtc_schedulewith a message that names the valid values.A typo (
exp,Exp) surfaces as a bareKeyError: 'exp'in/healthdetail, which doesn't tell the operator what to write invla_serving.yaml— unlike every other knob here, which fails with an actionable message.♻️ Suggested validation
+ try: + schedule = RTCAttentionSchedule[rtc_schedule] + except KeyError: + valid = ", ".join(s.name for s in RTCAttentionSchedule) + raise ValueError( + f"rtc_schedule '{rtc_schedule}' is not a known RTC schedule; " + f"set rtc_schedule in vla_serving.yaml to one of: {valid}" + ) from None self.policy.config.rtc_config = RTCConfig( enabled=True, - prefix_attention_schedule=RTCAttentionSchedule[rtc_schedule], + prefix_attention_schedule=schedule, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/docker/serve_policy.py` around lines 236 - 240, Validate rtc_schedule before indexing RTCAttentionSchedule in the policy initialization flow, and raise an actionable error that includes the invalid value and the valid RTCAttentionSchedule names. Keep valid schedule handling unchanged and ensure the message directs operators to the expected vla_serving.yaml values.src/vla_sim/launch/agent_bridge.launch.xml (1)
16-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing
http_timeoutas a launch arg.The adapter declares
http_timeout(andservice_name) as parameters, and its own comment says the timeout should be sized toExecutePolicy'spolicy_call_timeout. As launched, it's pinned to the 10.0s built-in with no way to align the two without editing the script.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/launch/agent_bridge.launch.xml` around lines 16 - 24, Expose an `http_timeout` launch argument in the launch configuration and pass it to the `get_action_chunk_adapter` node as its parameter, using the adapter’s existing default unless overridden. This should allow the timeout to be aligned with `ExecutePolicy`’s `policy_call_timeout` without modifying the script.src/vla_sim/docker/Dockerfile.serve_policy (1)
28-32: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueNo
USERin the image (Trivy DS-0002).The compose service sets
user:, so the deployed path is non-root, but a baredocker run python serve_policy.py(documented in the README) runs as root. AddingUSER 1000keeps the default safe without affecting the compose override.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/docker/Dockerfile.serve_policy` around lines 28 - 32, Add a USER 1000 directive to the Dockerfile after the application setup and before ENTRYPOINT, ensuring bare-container execution of serve_policy.py defaults to a non-root user while preserving the compose service’s user override.Source: Linters/SAST tools
src/vla_sim/script/get_action_chunk_adapter.py (2)
114-125: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winElement count isn't checked against the declared layout.
The 2-dimension check passes, but
dim[0].size * dim[1].sizemay not equallen(prev.data). Thereshapethen raises a rawValueError, which falls through to the generic handler asadapter failed: ValueError: cannot reshape ...— losing the actionable framing the sibling layout error already has.🛡️ Suggested check
steps, width = prev.layout.dim[0].size, prev.layout.dim[1].size + if steps * width != len(prev.data): + raise RequestError( + f"previous_action_chunk carries {len(prev.data)} values but " + f"its layout declares {steps}x{width} = {steps * width}" + ) payload["prev_chunk_left_over"] = (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/script/get_action_chunk_adapter.py` around lines 114 - 125, In the previous action-chunk handling within the request adapter, validate that steps multiplied by width equals len(prev.data) after confirming the layout has two dimensions. Raise RequestError with the same actionable layout-mismatch framing before calling np.asarray(...).reshape(...), preserving the existing valid-payload behavior.
199-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
response.success = Trueis set before the RTC echo is built.If
action_chunk_rawis malformed, the failure lands in_on_request's generic handler, which does resetsuccess— so behavior is correct today, but it depends on that ordering. Building the echo before flippingsuccessmakes the invariant local rather than incidental.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/script/get_action_chunk_adapter.py` around lines 199 - 215, The response success flag is set before constructing the RTC echo, allowing malformed action_chunk_raw data to fail after marking success. In the response-building method containing response.policy_action_chunk, move response.success = True to after all raw echo validation and Float64MultiArray construction complete, while preserving the existing success and failure handling.src/vla_sim/description/picknik_kinova_gen3.xacro (2)
23-23: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDefault
mujoco_modelpoints to a file that doesn't exist in this package.
description/mujoco/scene.xmlisn't shipped; onlygen3_7dof.xmlandcube_stack_scene.xmlexist.config.yamlalways overrides this tocube_stack_scene.xmlin the shipped path, so this only bites a standalone/default xacro render.🩹 Proposed fix
- <xacro:arg name="mujoco_model" default="description/mujoco/scene.xml" /> + <xacro:arg name="mujoco_model" default="description/mujoco/cube_stack_scene.xml" />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/description/picknik_kinova_gen3.xacro` at line 23, Update the mujoco_model argument default in the xacro configuration to reference the shipped cube_stack_scene.xml file instead of the nonexistent description/mujoco/scene.xml path, while preserving config.yaml overrides.
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify these macro includes are still needed.
gen3_macro.xacroand the gripper macro are included, but none of the following link/joint definitions (lines 41-707) appear to invoke them — everything is hand-written directly instead. If no macro from these includes is actually instantiated anywhere in this file, they're likely leftover from before the manual/inlined rewrite (consistent with the "picknik_accessories dropped" note nearby) and could be removed, also dropping the implicit runtime dependency onkortex_description's macro path resolving for the givenarm/gripperargs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/description/picknik_kinova_gen3.xacro` around lines 26 - 27, Verify whether the included arm and gripper macros are instantiated anywhere in the xacro content; if neither include is used, remove both xacro:include directives while preserving the manually defined links and joints.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yaml`:
- Around line 72-79: Update the max_joint_velocity values for joints 5–7 in the
control configuration to stay within the declared 1.2218 rad/s joint limits used
by the selected PoseJog controller; preserve the existing limits for joints 1–4
and ensure the controller does not permit velocities above the MoveIt
configuration.
In `@src/vla_sim/description/picknik_kinova_gen3.xacro`:
- Around line 538-560: Update the world-fixed table geometry in the table_joint
definition to match the stacking scene’s table placement, specifically aligning
the table link with x=0.5 and y=0.0 while preserving its existing dimensions and
z placement unless the scene requires otherwise. Keep the table link name
unchanged so the SRDF disable_collisions references remain valid.
In `@src/vla_sim/docker/Dockerfile.serve_policy`:
- Around line 5-7: Update the apt installation command in the Dockerfile to
satisfy hadolint DL3008 by pinning libgl1 and libglib2.0-0 to explicit versions,
or add a narrowly scoped DL3008 ignore directive if stable pinning is
impractical for the Debian slim repository.
- Around line 17-26: Update the TORCH_VERSION used in the Dockerfile’s
dependency installation to a version below 2.11.0 that is supported by
lerobot==0.6.0, and align TORCHVISION_VERSION with the matching vision wheel.
Preserve the existing optional TORCH_INDEX installation flow and lerobot
dependency.
In `@src/vla_sim/docker/serve_policy.py`:
- Around line 1-23: Reformat all three new Python files with the repository’s
Black/pre-commit configuration: src/vla_sim/docker/serve_policy.py (lines 1-23),
src/vla_sim/docker/test_serve_policy.py (lines 1-36), and
src/vla_sim/test/test_get_action_chunk_adapter.py (lines 1-28). Run pre-commit
run --all-files from the workspace root and commit the resulting formatting
changes.
In `@src/vla_sim/objectives/execute_color_stack_policy.xml`:
- Line 35: Replace the approximate policy_call_timeout handling in the adapter
used by execute_color_stack_policy with an explicit hard wall-clock deadline for
/get_action_chunk, covering connection and response-read time together. Ensure
the single-threaded request path returns or aborts once the end-to-end limit is
reached rather than relying on requests’ separate timeout phases, while
preserving the configured 10.0-second budget.
---
Nitpick comments:
In `@src/vla_sim/description/picknik_kinova_gen3.xacro`:
- Line 23: Update the mujoco_model argument default in the xacro configuration
to reference the shipped cube_stack_scene.xml file instead of the nonexistent
description/mujoco/scene.xml path, while preserving config.yaml overrides.
- Around line 26-27: Verify whether the included arm and gripper macros are
instantiated anywhere in the xacro content; if neither include is used, remove
both xacro:include directives while preserving the manually defined links and
joints.
In `@src/vla_sim/docker/Dockerfile.serve_policy`:
- Around line 28-32: Add a USER 1000 directive to the Dockerfile after the
application setup and before ENTRYPOINT, ensuring bare-container execution of
serve_policy.py defaults to a non-root user while preserving the compose
service’s user override.
In `@src/vla_sim/docker/serve_policy.py`:
- Around line 460-472: Add an explicit sequence-type validation for
payload["state"] alongside the existing payload["images"] check, before calling
len() or converting it with torch.tensor. Raise the endpoint’s existing
ValueError-style client error for scalar or string states, while preserving the
expected-dimension validation for valid sequences.
- Around line 236-240: Validate rtc_schedule before indexing
RTCAttentionSchedule in the policy initialization flow, and raise an actionable
error that includes the invalid value and the valid RTCAttentionSchedule names.
Keep valid schedule handling unchanged and ensure the message directs operators
to the expected vla_serving.yaml values.
In `@src/vla_sim/docker/test_serve_policy.py`:
- Around line 311-315: Update the cleanup registered in _start to close the HTTP
server socket as well as stopping serve_forever, using the server’s existing
shutdown and server_close lifecycle methods so each test releases its listening
file descriptor.
- Around line 207-210: Update test_unresolvable_fps_raises to use a checkpoint
identifier without a slash, such as “nonexistent-checkpoint”, so resolve_fps
exercises the local missing-checkpoint path without invoking Hugging Face hub
handling or network-dependent behavior.
In `@src/vla_sim/launch/agent_bridge.launch.xml`:
- Around line 16-24: Expose an `http_timeout` launch argument in the launch
configuration and pass it to the `get_action_chunk_adapter` node as its
parameter, using the adapter’s existing default unless overridden. This should
allow the timeout to be aligned with `ExecutePolicy`’s `policy_call_timeout`
without modifying the script.
In `@src/vla_sim/script/get_action_chunk_adapter.py`:
- Around line 114-125: In the previous action-chunk handling within the request
adapter, validate that steps multiplied by width equals len(prev.data) after
confirming the layout has two dimensions. Raise RequestError with the same
actionable layout-mismatch framing before calling np.asarray(...).reshape(...),
preserving the existing valid-payload behavior.
- Around line 199-215: The response success flag is set before constructing the
RTC echo, allowing malformed action_chunk_raw data to fail after marking
success. In the response-building method containing
response.policy_action_chunk, move response.success = True to after all raw echo
validation and Float64MultiArray construction complete, while preserving the
existing success and failure handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4e824ca-dd74-4ccb-bc34-7b2433afec88
⛔ Files ignored due to path filters (14)
src/vla_sim/description/mujoco/assets/kinova/base_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.objis excluded by!**/*.obj
📒 Files selected for processing (62)
.dockerignore.gitignoredocker-compose.yamlsrc/vla_sim/CMakeLists.txtsrc/vla_sim/README.mdsrc/vla_sim/config/config.yamlsrc/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yamlsrc/vla_sim/config/initial_positions.yamlsrc/vla_sim/config/moveit/joint_jog.yamlsrc/vla_sim/config/moveit/joint_limits.yamlsrc/vla_sim/config/moveit/picknik_kinova_gen3_base.srdfsrc/vla_sim/config/moveit/pose_ik_distance.yamlsrc/vla_sim/config/moveit/pose_jog.yamlsrc/vla_sim/config/moveit/sensors_3d.yamlsrc/vla_sim/config/vla_serving.yamlsrc/vla_sim/description/mujoco/assets/kinova/base_link.STLsrc/vla_sim/description/mujoco/assets/kinova/base_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/end_effector_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.STLsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.mtlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/base.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_tip_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/pad.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/silicone_pad.stlsrc/vla_sim/description/mujoco/cube_stack_scene.xmlsrc/vla_sim/description/mujoco/gen3_7dof.xmlsrc/vla_sim/description/picknik_kinova_gen3.xacrosrc/vla_sim/docker/Dockerfile.serve_policysrc/vla_sim/docker/README.mdsrc/vla_sim/docker/serve_policy.pysrc/vla_sim/docker/test_serve_policy.pysrc/vla_sim/hf_cache/.gitkeepsrc/vla_sim/launch/agent_bridge.launch.xmlsrc/vla_sim/models/.gitkeepsrc/vla_sim/objectives/execute_color_stack_policy.xmlsrc/vla_sim/objectives/reset_simulation.xmlsrc/vla_sim/package.xmlsrc/vla_sim/script/get_action_chunk_adapter.pysrc/vla_sim/test/test_get_action_chunk_adapter.pysrc/vla_sim/waypoints/waypoints.yaml
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vla_sim/docker/vla_inference_server.py`:
- Around line 528-530: Update the image decoding flow around decode_image_b64 so
TypeError from non-string image values is handled alongside ValueError as a
malformed-request condition, returning HTTP 400; preserve the existing
binascii.Error handling for invalid base64 and avoid treating these client input
errors as server faults.
In `@src/vla_sim/script/get_action_chunk_adapter.py`:
- Around line 123-130: Update _build_payload to validate that
request.robot_state.name and request.robot_state.position have equal lengths
before constructing the payload, and raise the established validation error
identifying the mismatched state arrays. Keep the existing state serialization
and _validate_chunk behavior unchanged once the upfront validation passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 511d366a-b889-4385-80dd-f8daaa40323e
⛔ Files ignored due to path filters (14)
src/vla_sim/description/mujoco/assets/kinova/base_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.objis excluded by!**/*.obj
📒 Files selected for processing (61)
.dockerignore.gitignoredocker-compose.yamlsrc/vla_sim/CMakeLists.txtsrc/vla_sim/README.mdsrc/vla_sim/config/config.yamlsrc/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yamlsrc/vla_sim/config/moveit/joint_jog.yamlsrc/vla_sim/config/moveit/joint_limits.yamlsrc/vla_sim/config/moveit/picknik_kinova_gen3_base.srdfsrc/vla_sim/config/moveit/pose_ik_distance.yamlsrc/vla_sim/config/moveit/pose_jog.yamlsrc/vla_sim/config/moveit/sensors_3d.yamlsrc/vla_sim/config/vla_serving.yamlsrc/vla_sim/description/mujoco/assets/kinova/base_link.STLsrc/vla_sim/description/mujoco/assets/kinova/base_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/end_effector_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.STLsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.mtlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/base.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_tip_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/pad.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/silicone_pad.stlsrc/vla_sim/description/mujoco/cube_stack_scene.xmlsrc/vla_sim/description/mujoco/gen3_7dof.xmlsrc/vla_sim/description/picknik_kinova_gen3.xacrosrc/vla_sim/docker/Dockerfile.vla_inference_serversrc/vla_sim/docker/README.mdsrc/vla_sim/docker/test_vla_inference_server.pysrc/vla_sim/docker/vla_inference_server.pysrc/vla_sim/hf_cache/.gitkeepsrc/vla_sim/launch/agent_bridge.launch.xmlsrc/vla_sim/models/.gitkeepsrc/vla_sim/objectives/execute_color_stack_policy.xmlsrc/vla_sim/objectives/reset_simulation.xmlsrc/vla_sim/package.xmlsrc/vla_sim/script/get_action_chunk_adapter.pysrc/vla_sim/test/test_get_action_chunk_adapter.pysrc/vla_sim/waypoints/waypoints.yaml
🚧 Files skipped from review as they are similar to previous changes (43)
- src/vla_sim/description/mujoco/assets/robotiq_2f85/finger_link.stl
- src/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stl
- src/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.STL
- src/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtl
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_1.mtl
- src/vla_sim/description/mujoco/assets/kinova/shoulder_link.mtl
- src/vla_sim/description/mujoco/assets/robotiq_2f85/silicone_pad.stl
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtl
- src/vla_sim/description/mujoco/assets/kinova/shoulder_link.STL
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.STL
- src/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.mtl
- src/vla_sim/description/mujoco/assets/kinova/forearm_link.STL
- src/vla_sim/description/mujoco/assets/kinova/end_effector_link.STL
- src/vla_sim/description/mujoco/assets/kinova/base_link.mtl
- .dockerignore
- src/vla_sim/description/mujoco/assets/robotiq_2f85/base.stl
- src/vla_sim/config/moveit/joint_limits.yaml
- src/vla_sim/CMakeLists.txt
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.mtl
- src/vla_sim/launch/agent_bridge.launch.xml
- src/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.mtl
- src/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.STL
- src/vla_sim/objectives/reset_simulation.xml
- src/vla_sim/config/moveit/pose_jog.yaml
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_2.mtl
- src/vla_sim/description/mujoco/assets/kinova/base_link.STL
- src/vla_sim/description/mujoco/assets/robotiq_2f85/pad.stl
- src/vla_sim/config/moveit/joint_jog.yaml
- src/vla_sim/description/mujoco/assets/kinova/forearm_link.mtl
- src/vla_sim/README.md
- src/vla_sim/objectives/execute_color_stack_policy.xml
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.STL
- src/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.mtl
- src/vla_sim/config/moveit/picknik_kinova_gen3_base.srdf
- src/vla_sim/description/mujoco/assets/rafti_finger.mtl
- src/vla_sim/config/vla_serving.yaml
- src/vla_sim/config/moveit/pose_ik_distance.yaml
- src/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STL
- src/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yaml
- src/vla_sim/config/moveit/sensors_3d.yaml
- src/vla_sim/description/mujoco/gen3_7dof.xml
- .gitignore
- src/vla_sim/description/mujoco/cube_stack_scene.xml
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vla_sim/description/mujoco/assets/robotiq_2f85/knuckle_link.stl`:
- Around line 1-3: Ensure the MuJoCo startup or installation flow fetches Git
LFS objects before loading gen3_7dof.xml, including the robotiq_2f85 mesh
assets. Add a check or fetch step that detects unresolved LFS pointers and runs
the project’s established Git LFS retrieval mechanism before launch.
In `@src/vla_sim/docker/vla_inference_server.py`:
- Around line 562-573: Update the request validation in do_POST around
inference_delay, guidance_horizon, and payload["task"] so malformed non-scalar
delay or horizon values and non-string tasks are rejected as client errors
(400), alongside the existing image-value validation. Normalize conversion
failures into the existing ValueError handling rather than allowing TypeError or
deep policy errors to escape, while preserving the non-negative checks for valid
numeric inputs.
In `@src/vla_sim/script/get_action_chunk_adapter.py`:
- Around line 208-228: Harden action_chunk validation before the width and
finiteness checks: validate that every row is a list-like numeric sequence,
catching scalar and non-numeric entries without allowing TypeError or ValueError
to escape. Raise RequestError messages that explicitly identify the invalid
action_chunk, matching the precise behavior of the action_chunk_raw path while
preserving existing width and non-finite value validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be2038d3-e9f2-4c15-84b9-b404d03cd230
⛔ Files ignored due to path filters (14)
src/vla_sim/description/mujoco/assets/kinova/base_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.objis excluded by!**/*.obj
📒 Files selected for processing (61)
.dockerignore.gitignoredocker-compose.yamlsrc/vla_sim/CMakeLists.txtsrc/vla_sim/README.mdsrc/vla_sim/config/config.yamlsrc/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yamlsrc/vla_sim/config/moveit/joint_jog.yamlsrc/vla_sim/config/moveit/joint_limits.yamlsrc/vla_sim/config/moveit/picknik_kinova_gen3_base.srdfsrc/vla_sim/config/moveit/pose_ik_distance.yamlsrc/vla_sim/config/moveit/pose_jog.yamlsrc/vla_sim/config/moveit/sensors_3d.yamlsrc/vla_sim/config/vla_serving.yamlsrc/vla_sim/description/mujoco/assets/kinova/base_link.STLsrc/vla_sim/description/mujoco/assets/kinova/base_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/end_effector_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.STLsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.mtlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/base.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_tip_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/pad.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/silicone_pad.stlsrc/vla_sim/description/mujoco/cube_stack_scene.xmlsrc/vla_sim/description/mujoco/gen3_7dof.xmlsrc/vla_sim/description/picknik_kinova_gen3.xacrosrc/vla_sim/docker/Dockerfile.vla_inference_serversrc/vla_sim/docker/README.mdsrc/vla_sim/docker/test_vla_inference_server.pysrc/vla_sim/docker/vla_inference_server.pysrc/vla_sim/hf_cache/.gitkeepsrc/vla_sim/launch/agent_bridge.launch.xmlsrc/vla_sim/models/.gitkeepsrc/vla_sim/objectives/execute_color_stack_policy.xmlsrc/vla_sim/objectives/reset_simulation.xmlsrc/vla_sim/package.xmlsrc/vla_sim/script/get_action_chunk_adapter.pysrc/vla_sim/test/test_get_action_chunk_adapter.pysrc/vla_sim/waypoints/waypoints.yaml
🚧 Files skipped from review as they are similar to previous changes (52)
- src/vla_sim/description/mujoco/assets/rafti_finger.mtl
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.STL
- src/vla_sim/description/mujoco/assets/kinova/forearm_link.mtl
- src/vla_sim/config/moveit/pose_ik_distance.yaml
- src/vla_sim/description/mujoco/assets/robotiq_2f85/base.stl
- src/vla_sim/description/mujoco/assets/kinova/end_effector_link.STL
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.mtl
- src/vla_sim/config/moveit/joint_limits.yaml
- src/vla_sim/description/mujoco/assets/kinova/shoulder_link.STL
- src/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STL
- src/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.STL
- src/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.STL
- src/vla_sim/package.xml
- src/vla_sim/description/mujoco/assets/robotiq_2f85/silicone_pad.stl
- src/vla_sim/description/mujoco/assets/kinova/forearm_link.STL
- src/vla_sim/CMakeLists.txt
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_1.mtl
- src/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stl
- src/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtl
- src/vla_sim/description/mujoco/assets/kinova/base_link.STL
- src/vla_sim/waypoints/waypoints.yaml
- src/vla_sim/description/mujoco/assets/robotiq_2f85/finger_tip_link.stl
- src/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.mtl
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_4.mtl
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtl
- src/vla_sim/docker/Dockerfile.vla_inference_server
- src/vla_sim/description/mujoco/assets/robotiq_2f85/pad.stl
- src/vla_sim/README.md
- src/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.STL
- src/vla_sim/description/mujoco/assets/kinova/shoulder_link.mtl
- .gitignore
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.mtl
- src/vla_sim/description/mujoco/assets/robotiq_2f85/finger_link.stl
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.STL
- .dockerignore
- src/vla_sim/config/moveit/joint_jog.yaml
- src/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.mtl
- src/vla_sim/config/moveit/sensors_3d.yaml
- src/vla_sim/objectives/reset_simulation.xml
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_2.mtl
- src/vla_sim/launch/agent_bridge.launch.xml
- src/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.mtl
- src/vla_sim/objectives/execute_color_stack_policy.xml
- docker-compose.yaml
- src/vla_sim/config/moveit/picknik_kinova_gen3_base.srdf
- src/vla_sim/config/config.yaml
- src/vla_sim/config/vla_serving.yaml
- src/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yaml
- src/vla_sim/docker/README.md
- src/vla_sim/description/picknik_kinova_gen3.xacro
- src/vla_sim/description/mujoco/cube_stack_scene.xml
- src/vla_sim/description/mujoco/gen3_7dof.xml
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/vla_sim/script/get_action_chunk_adapter.py (1)
90-92: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNon-numeric
http_timeoutoverride raisesTypeErrorfromnp.isfinite, bypassing the intended message. If the parameter is declared/overridden as a string (e.g. from a launchvalue="fast"),resolve_http_timeoutcrashes with a numpyTypeErrorinstead of the actionableValueError. A type guard keeps the startup error self-explanatory.🛡️ Proposed guard
def resolve_http_timeout(value: float) -> float: """Reject a non-positive or non-finite http_timeout at startup, not per call.""" - if not np.isfinite(value) or value <= 0.0: + if ( + isinstance(value, bool) + or not isinstance(value, (int, float)) + or not np.isfinite(value) + or value <= 0.0 + ): raise ValueError( f"the http_timeout parameter must be a positive number of " f"seconds, got {value!r}" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/script/get_action_chunk_adapter.py` around lines 90 - 92, Update the http_timeout initialization around resolve_http_timeout and declare_parameter to validate that the resolved parameter value is numeric before it reaches np.isfinite, raising the intended actionable ValueError for string or other non-numeric overrides while preserving normal numeric timeout handling.src/vla_sim/docker/vla_inference_server.py (1)
347-362: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWarmup hardcodes 224x224 frames. If a checkpoint's image preprocessing does not resize (or expects a different native resolution), warmup fails and is only logged as a warning, so the first real request still pays the cold cost silently. Consider deriving the shape from
policy.config.input_features[...].shapeinstead of the constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/docker/vla_inference_server.py` around lines 347 - 362, The warmup implementation in warmup() hardcodes 224x224 image tensors, which can mismatch the checkpoint’s configured input resolution. Derive each camera tensor shape from the corresponding policy.config.input_features entry, preserving its channel and spatial dimensions before calling infer().src/vla_sim/description/picknik_kinova_gen3.xacro (1)
392-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead Gazebo/ODE
<surface>blocks in a MuJoCo cell.
<surface>,<friction><ode>, and<contact><ode>are SDF elements, not URDF children of<collision>; URDF parsers ignore them (often with warnings). Contact behavior for this cell comes fromdescription/mujoco/cube_stack_scene.xml, so these copy-paste artifacts have no effect and misleadingly suggest tuned fingertip friction. Consider dropping them.Also applies to: 427-444
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/description/picknik_kinova_gen3.xacro` around lines 392 - 409, Remove the unused Gazebo/ODE <surface> blocks, including their <friction><ode> and <contact><ode> contents, from both affected collision cells in the MuJoCo-related Xacro. Preserve the surrounding collision geometry and rely on the existing cube_stack_scene.xml contact configuration.src/vla_sim/CMakeLists.txt (1)
24-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ament_lint_auto_find_test_dependencies()will find no linters.
package.xmlonly declaresament_lint_autoandament_cmake_pytestas test deps, so no linter packages (e.g.ament_lint_common, orament_cmake_flake8/ament_cmake_xmllintfor this Python+XML package) are discoverable and the call is effectively a no-op. Either add the linter test deps or drop the call to avoid implying lint coverage.♻️ Suggested package.xml addition
<test_depend>ament_cmake_flake8</test_depend> <test_depend>ament_cmake_xmllint</test_depend>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/CMakeLists.txt` around lines 24 - 38, Update the BUILD_TESTING dependencies so ament_lint_auto_find_test_dependencies() discovers actual linters: add the appropriate Python and XML linter test dependencies, such as ament_cmake_flake8 and ament_cmake_xmllint, to package.xml. Keep the existing lint-discovery call in CMakeLists.txt.src/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yaml (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
twist_controllerandfault_controllerare declared but never spawned, andjoint: tcpdoesn't exist in this description.Neither controller appears in
controllers_active_at_startup,controllers_inactive_at_startup, norcontrollers_not_managedinconfig/config.yaml, so these blocks are dead configuration.twist_controller'sjoint: tcp(Line 24) also references a frame absent frompicknik_kinova_gen3.xacro(the EE frame here isgrasp_link), so it would fail if anyone later loads it. Drop them or fix the frame and register them inconfig.yaml.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yaml` around lines 8 - 14, Remove the unused twist_controller and fault_controller declarations from the ros2_control configuration, since neither is spawned or managed in config.yaml. Do not retain the invalid twist_controller joint reference to tcp; only keep or reintroduce these controllers if they are explicitly registered in the appropriate config.yaml startup or management list and the twist controller uses the existing grasp_link frame.src/vla_sim/description/mujoco/gen3_7dof.xml (1)
183-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
rafti_finger*meshes appear unused. Nogeomin this model references these five meshes (the gripper uses therobotiq_85_*meshes). Dropping them removes five asset files from the load-time dependency set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/description/mujoco/gen3_7dof.xml` around lines 183 - 187, Remove the five unused rafti_finger mesh declarations—rafti_finger and rafti_finger_collision_1 through rafti_finger_collision_4—from the model, and remove their corresponding asset files so no unnecessary load-time dependencies remain. Preserve the robotiq_85 mesh declarations and all referenced geometry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vla_sim/config/moveit/picknik_kinova_gen3_base.srdf`:
- Around line 15-26: Update the SRDF gripper group around the robotiq finger
joints to remove robotiq_85_left_finger_joint and robotiq_85_right_finger_joint
from the active joint list, marking them passive or defining them as
fixed/mimicked consistently with the robot description.
In `@src/vla_sim/description/mujoco/assets/kinova/end_effector_link.STL`:
- Around line 1-3: Replace the Git LFS pointer in end_effector_link.STL with the
actual valid STL mesh object, ensuring the file contains at least the required
80-byte header and 4-byte triangle count so the referenced MuJoCo asset loads
correctly.
---
Nitpick comments:
In `@src/vla_sim/CMakeLists.txt`:
- Around line 24-38: Update the BUILD_TESTING dependencies so
ament_lint_auto_find_test_dependencies() discovers actual linters: add the
appropriate Python and XML linter test dependencies, such as ament_cmake_flake8
and ament_cmake_xmllint, to package.xml. Keep the existing lint-discovery call
in CMakeLists.txt.
In `@src/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yaml`:
- Around line 8-14: Remove the unused twist_controller and fault_controller
declarations from the ros2_control configuration, since neither is spawned or
managed in config.yaml. Do not retain the invalid twist_controller joint
reference to tcp; only keep or reintroduce these controllers if they are
explicitly registered in the appropriate config.yaml startup or management list
and the twist controller uses the existing grasp_link frame.
In `@src/vla_sim/description/mujoco/gen3_7dof.xml`:
- Around line 183-187: Remove the five unused rafti_finger mesh
declarations—rafti_finger and rafti_finger_collision_1 through
rafti_finger_collision_4—from the model, and remove their corresponding asset
files so no unnecessary load-time dependencies remain. Preserve the robotiq_85
mesh declarations and all referenced geometry.
In `@src/vla_sim/description/picknik_kinova_gen3.xacro`:
- Around line 392-409: Remove the unused Gazebo/ODE <surface> blocks, including
their <friction><ode> and <contact><ode> contents, from both affected collision
cells in the MuJoCo-related Xacro. Preserve the surrounding collision geometry
and rely on the existing cube_stack_scene.xml contact configuration.
In `@src/vla_sim/docker/vla_inference_server.py`:
- Around line 347-362: The warmup implementation in warmup() hardcodes 224x224
image tensors, which can mismatch the checkpoint’s configured input resolution.
Derive each camera tensor shape from the corresponding
policy.config.input_features entry, preserving its channel and spatial
dimensions before calling infer().
In `@src/vla_sim/script/get_action_chunk_adapter.py`:
- Around line 90-92: Update the http_timeout initialization around
resolve_http_timeout and declare_parameter to validate that the resolved
parameter value is numeric before it reaches np.isfinite, raising the intended
actionable ValueError for string or other non-numeric overrides while preserving
normal numeric timeout handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7ee4197-5e40-4e38-b4d2-c14645cdc5cb
⛔ Files ignored due to path filters (14)
src/vla_sim/description/mujoco/assets/kinova/base_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.objis excluded by!**/*.obj
📒 Files selected for processing (61)
.dockerignore.gitignoredocker-compose.yamlsrc/vla_sim/CMakeLists.txtsrc/vla_sim/README.mdsrc/vla_sim/config/config.yamlsrc/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yamlsrc/vla_sim/config/moveit/joint_jog.yamlsrc/vla_sim/config/moveit/joint_limits.yamlsrc/vla_sim/config/moveit/picknik_kinova_gen3_base.srdfsrc/vla_sim/config/moveit/pose_ik_distance.yamlsrc/vla_sim/config/moveit/pose_jog.yamlsrc/vla_sim/config/moveit/sensors_3d.yamlsrc/vla_sim/config/vla_serving.yamlsrc/vla_sim/description/mujoco/assets/kinova/base_link.STLsrc/vla_sim/description/mujoco/assets/kinova/base_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/end_effector_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.STLsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.mtlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/base.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_tip_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/pad.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/silicone_pad.stlsrc/vla_sim/description/mujoco/cube_stack_scene.xmlsrc/vla_sim/description/mujoco/gen3_7dof.xmlsrc/vla_sim/description/picknik_kinova_gen3.xacrosrc/vla_sim/docker/Dockerfile.vla_inference_serversrc/vla_sim/docker/README.mdsrc/vla_sim/docker/test_vla_inference_server.pysrc/vla_sim/docker/vla_inference_server.pysrc/vla_sim/hf_cache/.gitkeepsrc/vla_sim/launch/agent_bridge.launch.xmlsrc/vla_sim/models/.gitkeepsrc/vla_sim/objectives/execute_color_stack_policy.xmlsrc/vla_sim/objectives/reset_simulation.xmlsrc/vla_sim/package.xmlsrc/vla_sim/script/get_action_chunk_adapter.pysrc/vla_sim/test/test_get_action_chunk_adapter.pysrc/vla_sim/waypoints/waypoints.yaml
🚧 Files skipped from review as they are similar to previous changes (19)
- src/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STL
- src/vla_sim/description/mujoco/assets/robotiq_2f85/finger_link.stl
- src/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.STL
- src/vla_sim/description/mujoco/assets/robotiq_2f85/base.stl
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.STL
- src/vla_sim/config/moveit/pose_ik_distance.yaml
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_4.mtl
- src/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.mtl
- src/vla_sim/description/mujoco/assets/robotiq_2f85/silicone_pad.stl
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.mtl
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtl
- src/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stl
- src/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.mtl
- src/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtl
- src/vla_sim/description/mujoco/assets/kinova/shoulder_link.mtl
- src/vla_sim/README.md
- src/vla_sim/docker/Dockerfile.vla_inference_server
- src/vla_sim/description/mujoco/assets/kinova/base_link.mtl
- .gitignore
fdavulcu
marked this pull request as ready for review
July 30, 2026 12:40
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/vla_sim/docker/README.md (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMention the
USER/HOMEenv for the mapped-uid dev run.The compose service sets
USER=vlaandHOME=/tmpprecisely because the image only has a passwd entry for uid 1000 (getpass.getuser()in torch's cache-dir setup). A baredocker run --user $(id -u):$(id -g)per this section lacks both, so the documented dev path can fail where compose succeeds.📝 Suggested doc addition
For development without compose, run the image directly with `--user $(id -u):$(id -g)` (the image defaults to uid 1000, so bind-mounted -files otherwise end up owned by that uid) and mount a Hugging Face cache +files otherwise end up owned by that uid), add `-e USER=vla -e HOME=/tmp` +(the image ships a passwd entry for uid 1000 only), and mount a Hugging Face cache (`-v "$PWD/../hf_cache:/hf" -e HF_HOME=/hf`) so multi-GB checkpoint downloads persist there instead of filling the container's writable layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/docker/README.md` around lines 52 - 56, Update the direct-development docker command documentation to include the same USER=vla and HOME=/tmp environment settings used by the compose service. Keep the existing mapped-UID and Hugging Face cache mounts intact so getpass.getuser() and torch cache setup work for non-default host UIDs.src/vla_sim/CMakeLists.txt (1)
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
CMAKE_CURRENT_SOURCE_DIRfor the test working directory.Equivalent today, but
CMAKE_SOURCE_DIRbreaks if this package is ever consumed viaadd_subdirectory.♻️ Proposed tweak
ament_add_pytest_test(test_get_action_chunk_adapter test/test_get_action_chunk_adapter.py TIMEOUT 60 - WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} + WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/CMakeLists.txt` around lines 31 - 35, Update the WORKING_DIRECTORY argument in the ament_add_pytest_test declaration for test_get_action_chunk_adapter to use CMAKE_CURRENT_SOURCE_DIR instead of CMAKE_SOURCE_DIR, preserving the existing test configuration.src/vla_sim/config/config.yaml (1)
106-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
twist_controllerandfault_controllerare configured but never spawned.Both are declared in
config/control/picknik_kinova_gen3.ros2_control.yaml(lines 8-9, 13-14) yet appear in neither startup list norcontrollers_not_managed, so their parameter blocks are dead config. Either list them as inactive-at-startup or drop them from the controller config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/config/config.yaml` around lines 106 - 117, Update the controller configuration so twist_controller and fault_controller are not left unspawned: add both to controllers_inactive_at_startup if they should remain available for later activation, or remove their parameter blocks from the controller configuration if they are unused. Keep the startup and management lists consistent with the intended controller lifecycle.src/vla_sim/package.xml (1)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ament_lint_auto_find_test_dependencies()will find no linters.
CMakeLists.txtline 37 calls it, but no linter packages are declared here, so linting silently no-ops. Addament_lint_common(or the specific linters you want) if lint coverage is intended.♻️ Proposed addition
<test_depend>ament_cmake_pytest</test_depend> <test_depend>ament_lint_auto</test_depend> + <test_depend>ament_lint_common</test_depend> <test_depend>python3-pytest</test_depend>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/package.xml` around lines 46 - 48, Add a test dependency on ament_lint_common in package.xml so ament_lint_auto_find_test_dependencies() in CMakeLists.txt discovers the standard linters; keep the existing pytest dependencies unchanged.src/vla_sim/description/mujoco/gen3_7dof.xml (2)
363-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnnamed body. The gripper mount body has no
name, so it can only be referenced by index and shows up anonymously in TF/debug output. Naming it (e.g.gripper_mount) costs nothing and helps when inspecting the tree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/description/mujoco/gen3_7dof.xml` at line 363, Add a descriptive name attribute, such as gripper_mount, to the gripper mount body declaration identified by childclass="2f85", preserving its existing hierarchy and configuration.
184-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused mesh declarations.
rafti_fingerandrafti_finger_collision_1..4are declared but nogeominworldbodyreferences them; the gripper uses therobotiq_85_*meshes. Consider dropping them (and their.mtlassets) unless a downstream include consumes them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/description/mujoco/gen3_7dof.xml` around lines 184 - 188, Remove the unused mesh declarations rafti_finger and rafti_finger_collision_1 through rafti_finger_collision_4 from the MuJoCo model, along with their corresponding .mtl assets if they are not consumed by downstream includes. Preserve the existing robotiq_85_* mesh declarations and references.src/vla_sim/description/mujoco/cube_stack_scene.xml (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoot-level
geom solrefalso retunes the robot's contacts. This unnamed<default>merges with the root default from the includedgen3_7dof.xml, so every robot geom that doesn't setsolref(all the arm collision meshes) also gets.004 1, not just the scene geoms. If only the table/floor/cubes were meant to be stiff, setsolrefon those geoms directly (the cubes already do at Lines 161, 174, 187).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/description/mujoco/cube_stack_scene.xml` around lines 27 - 29, Remove the root-level default geom solref override and apply the intended .004 1 solref directly to the table and floor geoms, leaving the existing cube solref settings unchanged. Ensure robot geoms inherited from gen3_7dof.xml retain their original contact parameters.src/vla_sim/launch/agent_bridge.launch.xml (1)
6-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing
http_timeoutas a launch arg. The README's troubleshooting row tells operators to raise the adapter'shttp_timeoutalongsidepolicy_call_timeout, but the only way to do that today is editing this file (the node declares the parameter with a 9.0 default). An arg mirrors theinfer_urlpattern.♻️ Proposed change
<arg name="infer_url" default="$(env INFER_URL 'http://127.0.0.1:8973/infer')" /> + <!-- Keep strictly below ExecutePolicy's policy_call_timeout. --> + <arg name="http_timeout" default="9.0" /> <include file="$(find-pkg-share moveit_studio_agent)/launch/studio_agent_bridge.launch.xml" /> @@ <param name="infer_url" value="$(var infer_url)" /> + <param name="http_timeout" value="$(var http_timeout)" /> </node>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vla_sim/launch/agent_bridge.launch.xml` around lines 6 - 24, Expose the adapter’s http_timeout as a launch argument alongside infer_url, using the adapter’s existing 9.0-second default. Pass that argument into the get_action_chunk_adapter node as its http_timeout parameter so operators can override it without editing the launch file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vla_sim/README.md`:
- Line 131: Update the timeout guidance in the README table to require the
adapter node’s http_timeout to be strictly below the objective’s
policy_call_timeout, matching get_action_chunk_adapter.py and preventing equal
budgets.
---
Nitpick comments:
In `@src/vla_sim/CMakeLists.txt`:
- Around line 31-35: Update the WORKING_DIRECTORY argument in the
ament_add_pytest_test declaration for test_get_action_chunk_adapter to use
CMAKE_CURRENT_SOURCE_DIR instead of CMAKE_SOURCE_DIR, preserving the existing
test configuration.
In `@src/vla_sim/config/config.yaml`:
- Around line 106-117: Update the controller configuration so twist_controller
and fault_controller are not left unspawned: add both to
controllers_inactive_at_startup if they should remain available for later
activation, or remove their parameter blocks from the controller configuration
if they are unused. Keep the startup and management lists consistent with the
intended controller lifecycle.
In `@src/vla_sim/description/mujoco/cube_stack_scene.xml`:
- Around line 27-29: Remove the root-level default geom solref override and
apply the intended .004 1 solref directly to the table and floor geoms, leaving
the existing cube solref settings unchanged. Ensure robot geoms inherited from
gen3_7dof.xml retain their original contact parameters.
In `@src/vla_sim/description/mujoco/gen3_7dof.xml`:
- Line 363: Add a descriptive name attribute, such as gripper_mount, to the
gripper mount body declaration identified by childclass="2f85", preserving its
existing hierarchy and configuration.
- Around line 184-188: Remove the unused mesh declarations rafti_finger and
rafti_finger_collision_1 through rafti_finger_collision_4 from the MuJoCo model,
along with their corresponding .mtl assets if they are not consumed by
downstream includes. Preserve the existing robotiq_85_* mesh declarations and
references.
In `@src/vla_sim/docker/README.md`:
- Around line 52-56: Update the direct-development docker command documentation
to include the same USER=vla and HOME=/tmp environment settings used by the
compose service. Keep the existing mapped-UID and Hugging Face cache mounts
intact so getpass.getuser() and torch cache setup work for non-default host
UIDs.
In `@src/vla_sim/launch/agent_bridge.launch.xml`:
- Around line 6-24: Expose the adapter’s http_timeout as a launch argument
alongside infer_url, using the adapter’s existing 9.0-second default. Pass that
argument into the get_action_chunk_adapter node as its http_timeout parameter so
operators can override it without editing the launch file.
In `@src/vla_sim/package.xml`:
- Around line 46-48: Add a test dependency on ament_lint_common in package.xml
so ament_lint_auto_find_test_dependencies() in CMakeLists.txt discovers the
standard linters; keep the existing pytest dependencies unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45415e94-1020-4436-a510-12adccdcad0b
⛔ Files ignored due to path filters (14)
src/vla_sim/description/mujoco/assets/kinova/base_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.objis excluded by!**/*.objsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.objis excluded by!**/*.obj
📒 Files selected for processing (60)
.dockerignore.gitignoredocker-compose.yamlsrc/vla_sim/CMakeLists.txtsrc/vla_sim/README.mdsrc/vla_sim/config/config.yamlsrc/vla_sim/config/control/picknik_kinova_gen3.ros2_control.yamlsrc/vla_sim/config/moveit/joint_jog.yamlsrc/vla_sim/config/moveit/joint_limits.yamlsrc/vla_sim/config/moveit/picknik_kinova_gen3_base.srdfsrc/vla_sim/config/moveit/pose_ik_distance.yamlsrc/vla_sim/config/moveit/pose_jog.yamlsrc/vla_sim/config/moveit/sensors_3d.yamlsrc/vla_sim/config/vla_serving.yamlsrc/vla_sim/description/mujoco/assets/kinova/base_link.STLsrc/vla_sim/description/mujoco/assets/kinova/base_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_no_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STLsrc/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/end_effector_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.STLsrc/vla_sim/description/mujoco/assets/kinova/forearm_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.STLsrc/vla_sim/description/mujoco/assets/kinova/shoulder_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.mtlsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.STLsrc/vla_sim/description/mujoco/assets/kinova/spherical_wrist_2_link.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_1.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_2.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtlsrc/vla_sim/description/mujoco/assets/rafti_finger_collision_4.mtlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/base.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/finger_tip_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/knuckle_link.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/pad.stlsrc/vla_sim/description/mujoco/assets/robotiq_2f85/silicone_pad.stlsrc/vla_sim/description/mujoco/cube_stack_scene.xmlsrc/vla_sim/description/mujoco/gen3_7dof.xmlsrc/vla_sim/description/picknik_kinova_gen3.xacrosrc/vla_sim/docker/Dockerfile.vla_inference_serversrc/vla_sim/docker/README.mdsrc/vla_sim/docker/test_vla_inference_server.pysrc/vla_sim/docker/vla_inference_server.pysrc/vla_sim/hf_cache/.gitkeepsrc/vla_sim/launch/agent_bridge.launch.xmlsrc/vla_sim/models/.gitkeepsrc/vla_sim/objectives/stack_cubes_with_the_vla_policy.xmlsrc/vla_sim/package.xmlsrc/vla_sim/script/get_action_chunk_adapter.pysrc/vla_sim/test/test_get_action_chunk_adapter.pysrc/vla_sim/waypoints/waypoints.yaml
🚧 Files skipped from review as they are similar to previous changes (13)
- src/vla_sim/description/mujoco/assets/robotiq_2f85/inner_knuckle_link.stl
- src/vla_sim/config/moveit/joint_jog.yaml
- src/vla_sim/description/mujoco/assets/robotiq_2f85/base.stl
- src/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.mtl
- src/vla_sim/description/mujoco/assets/rafti_finger_collision_3.mtl
- src/vla_sim/description/mujoco/assets/kinova/bracelet_with_vision_link.STL
- src/vla_sim/description/mujoco/assets/kinova/half_arm_1_link.STL
- src/vla_sim/description/mujoco/assets/rafti_finger.mtl
- src/vla_sim/description/mujoco/assets/kinova/shoulder_link.STL
- src/vla_sim/description/mujoco/assets/kinova/base_link.mtl
- .gitignore
- src/vla_sim/description/mujoco/assets/kinova/spherical_wrist_1_link.mtl
- src/vla_sim/description/mujoco/assets/kinova/half_arm_2_link.mtl
|
|
|
|
JWhitleyWork
requested changes
Aug 3, 2026
Member
|
Also, PR needs a rebase after we renamed |
danwahl
reviewed
Aug 4, 2026
|
1 similar comment
|
|
danwahl
approved these changes
Aug 5, 2026
There was a problem hiding this comment.
[written by AI]
Approving. I ran this branch end to end at da2ee41c against a freshly rebuilt core.
Two small things:
1. /joint_commands is only ever published by quest_oculus_teleop.
No controller publishes to it. I recorded 75 s of real arm motion driven by ExecutePolicy and the topic carried zero messages; a separate gripper-driven recording did the same.
2. The policy adapter only runs on the runtime path.
get_action_chunk_adapter.py is launched from launch/runtime.launch.xml, so /get_action_chunk is absent under agent_robot.app and Stack Cubes with the VLA Policy fails there until you start the adapter by hand.
One other small note on docker/README.md: the compose-free docker run fallback omits --gpus all, so following it verbatim on an NVIDIA machine gets you device: auto resolving to CPU.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
danwahl
dismissed
their stale review
August 5, 2026 19:47
Duplicate submission — see the identical approval above.
danwahl
previously approved these changes
Aug 5, 2026
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JWhitleyWork
force-pushed
the
20588-vla-sim
branch
from
August 6, 2026 16:48
417fdb3 to
011b7be
Compare
JWhitleyWork
approved these changes
Aug 6, 2026
JWhitleyWork
enabled auto-merge
August 6, 2026 16:49
|
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
4 participants
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.
[written (mostly) by AI]
Motivation
Adds
vla_sim: prompt-driven color cube stacking on a simulated Kinova Gen3, executed by a vision-language-action policy. Closes PickNikRobotics/moveit_pro#20588 and PickNikRobotics/moveit_pro#20587, part of epic PickNikRobotics/moveit_pro#20583. Requires the paired launcher change, which starts and stops the inference server with the stack. Both target 10.0.0.How it works
The policy is served by a LeRobot inference server in its own container (
src/vla_sim/docker/). Keeping it separate keeps torch and LeRobot out of the product image, lets the ML stack be pinned and upgraded on its own schedule, and avoids real conflicts. One concrete example: the product image needs numpy 1.x while LeRobot needs numpy 2.A small ROS node serves
/get_action_chunkand forwards requests to the server over HTTP.ExecutePolicyonly sees the ROS service, named by itspolicy_service_nameport, so any node implementingGetActionChunkcan back it, including one you write yourself. The adapter that ships here requires the server to be on the same machine: requests carry the deployment's shared key over plain HTTP, so it validatesinfer_urlat startup and accepts only a loopback address. Serving from another machine comes after the MVP.Model settings live in one file,
src/vla_sim/config/vla_serving.yaml: checkpoint, rate, device, RTC. Edits apply on the nextmoveit_pro run, with no rebuild. Checkpoints can be Hugging Face repos, cached undersrc/vla_sim/hf_cache/, or local folders dropped intosrc/vla_sim/models/. On NVIDIA machines the server uses the GPU automatically.The objective's
image_namesare the checkpoint's trained camera names, and there is no remapping layer. If the request's camera names don't exactly match the checkpoint's, the server rejects it and the objective shows a message listing the expected names. Loading, ready, and load errors surface the same way.