Conversation
…ac Lab develop support Assisted with Codex
WalkthroughThis PR extends Isaac Sim and Isaac Lab support from binary installations (versions 5.1.0 and 2.3.2) to source builds via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
docker_modules/install_isaac_sim.sh (2)
97-104: Clarify the comment about compression/extraction.The comment on lines 98-99 mentions "source build" but appears in the
5.1.0binary install block. This could confuse future maintainers. The comment may have been intended to explain the consistent extraction approach, but currently reads as if it applies to source builds.📝 Suggested clarification
elif [ "$ISAAC_SIM_VERSION" = "5.1.0" ]; then - # It's a bit unfortunate that we are currently manually compressing the source build and then extracting - # it again to install, but without this process, the build will not be standalone (depends on `.cache`). + # Extract the pre-built archive to ISAACSIM_PATH (same destination as the source build path for consistency). echo "Extracting standalone package to $ISAACSIM_PATH..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker_modules/install_isaac_sim.sh` around lines 97 - 104, The comment in the ISAAC_SIM_VERSION == "5.1.0" block incorrectly refers to a "source build" and should be clarified to reflect that this branch extracts a prebuilt standalone binary archive; update the comment above the extraction (near the echo "Extracting standalone package...") to explain that we compress the built standalone package elsewhere and here we extract the prebuilt standalone archive (referencing ISAAC_SIM_STANDALONE_ARCHIVE and ISAACSIM_PATH) so maintainers understand this is a binary/archive extraction path for version 5.1.0 rather than a source build step.
119-120: Update comment to reflect both supported versions.The condition on line 119 now covers both
developand5.1.0, but the comment on line 120 still says "Isaac Sim 5.1.0 specific directories". Consider updating it to avoid confusion.📝 Suggested comment update
if [ "$ISAAC_SIM_VERSION" = "develop" ] || [ "$ISAAC_SIM_VERSION" = "5.1.0" ]; then - echo "Creating Isaac Sim 5.1.0 specific directories with correct ownership to avoid permission issues after volume mount..." + echo "Creating Isaac Sim cache directories with correct ownership to avoid permission issues after volume mount..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker_modules/install_isaac_sim.sh` around lines 119 - 120, Update the echo comment that currently reads "Creating Isaac Sim 5.1.0 specific directories..." to reflect both supported values checked by the condition (ISAAC_SIM_VERSION being "develop" or "5.1.0"); locate the conditional that checks ISAAC_SIM_VERSION and the echo statement and change the message to mention both "develop" and "5.1.0" or to use a generic phrase like "Isaac Sim specific directories" so it accurately describes the behavior.orbslam3_ws/docker/compose.yaml (1)
117-123: Device passthrough change is acceptable givenprivileged: true.Switching from selective device mounts (
/dev/dri,/dev/snd,/dev/shm) to broad/dev:/devaccess is acceptable since the container already runs withprivileged: true(line 68). The TODO comment on line 122 is now outdated.📝 Consider updating the TODO comment
- # TODO: Uncomment the line below and comment out the three entries above to enable USB support. + # Mounting /dev:/dev for full device access (requires privileged: true). - /dev:/dev🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@orbslam3_ws/docker/compose.yaml` around lines 117 - 123, Update the outdated TODO comment next to the device mount change: since the service already uses privileged: true, replace or remove the TODO that suggests commenting/uncommenting the selective mounts and the broad '- /dev:/dev' line; specifically edit the compose.yaml comment block containing the selective mounts (/dev/dri, /dev/snd, /dev/shm), the TODO line, and the '- /dev:/dev' entry so the comment accurately reflects that broad device passthrough is intentional when privileged: true (or document the security implications and alternative of selective mounts if you want to keep that option).vlp_ws/docker/compose.yaml (1)
117-123: Stale TODO comment after device mount switch.The TODO at line 122 reads "Uncomment the line below and comment out the three entries above to enable USB support" but now
/dev:/devis already uncommented and the three entries are already commented. The comment is misleading in its current state.📝 Suggested comment update
# Mount shared memory for ROS2 communication. # - /dev/shm:/dev/shm - # TODO: Uncomment the line below and comment out the three entries above to enable USB support. + # Note: /dev:/dev provides full device access including USB, DRI, sound, and shared memory. + # For minimal device access, comment the line below and uncomment /dev/dri, /dev/snd, /dev/shm above. - /dev:/dev🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vlp_ws/docker/compose.yaml` around lines 117 - 123, The TODO comment is now stale because /dev:/dev is already uncommented and the three device mount lines are commented; update or remove the TODO in the docker/compose.yaml block so it accurately reflects the current configuration (either remove the instruction or change it to explain that /dev:/dev enables USB support and the former individual mounts are disabled). Locate the comment lines referencing "/dev/dri:/dev/dri", "/dev/snd:/dev/snd", "/dev/shm:/dev/shm" and the active "- /dev:/dev" entry and adjust the TODO text accordingly.h1_ws/docker/compose.yaml (1)
117-123: Same stale TODO comment pattern across all workspace files.Consider a bulk update to fix the misleading TODO comment in all affected compose.yaml files for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@h1_ws/docker/compose.yaml` around lines 117 - 123, The TODO comment is stale and misleading: update the commented block around the device mounts so it accurately reflects current behavior (explain that /dev is mounted for full device/USB support and the other individual mounts are optional examples) and remove the "TODO: Uncomment..." instruction; apply the same change consistently across all compose.yaml files that contain the same commented entries (look for the comment lines containing "/dev/dri", "/dev/snd", "/dev/shm", the "TODO: Uncomment" text, and the "- /dev:/dev" entry) so the documentation matches the actual configuration.turtlebot3_ws/docker/compose.yaml (1)
117-123: Same stale TODO comment issue as other workspace files.The TODO comment at line 122 is now misleading since
/dev:/devis active and the individual device mounts are commented out. Consider updating to reflect the current state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@turtlebot3_ws/docker/compose.yaml` around lines 117 - 123, The TODO comment is stale because the catch‑all mount "- /dev:/dev" is active while the individual device mounts are commented out; update or remove the TODO so it accurately reflects current behavior by either deleting the TODO and explaining that "/dev:/dev" is enabled (and why), or change it to instruct maintainers to comment out "- /dev:/dev" if they prefer fine‑grained mounts like "- /dev/dri:/dev/dri", "- /dev/snd:/dev/snd", and "- /dev/shm:/dev/shm" (referencing the "- /dev:/dev" line and the three commented mount lines).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docker_modules/install_isaac_sim.sh`:
- Around line 97-104: The comment in the ISAAC_SIM_VERSION == "5.1.0" block
incorrectly refers to a "source build" and should be clarified to reflect that
this branch extracts a prebuilt standalone binary archive; update the comment
above the extraction (near the echo "Extracting standalone package...") to
explain that we compress the built standalone package elsewhere and here we
extract the prebuilt standalone archive (referencing
ISAAC_SIM_STANDALONE_ARCHIVE and ISAACSIM_PATH) so maintainers understand this
is a binary/archive extraction path for version 5.1.0 rather than a source build
step.
- Around line 119-120: Update the echo comment that currently reads "Creating
Isaac Sim 5.1.0 specific directories..." to reflect both supported values
checked by the condition (ISAAC_SIM_VERSION being "develop" or "5.1.0"); locate
the conditional that checks ISAAC_SIM_VERSION and the echo statement and change
the message to mention both "develop" and "5.1.0" or to use a generic phrase
like "Isaac Sim specific directories" so it accurately describes the behavior.
In `@h1_ws/docker/compose.yaml`:
- Around line 117-123: The TODO comment is stale and misleading: update the
commented block around the device mounts so it accurately reflects current
behavior (explain that /dev is mounted for full device/USB support and the other
individual mounts are optional examples) and remove the "TODO: Uncomment..."
instruction; apply the same change consistently across all compose.yaml files
that contain the same commented entries (look for the comment lines containing
"/dev/dri", "/dev/snd", "/dev/shm", the "TODO: Uncomment" text, and the "-
/dev:/dev" entry) so the documentation matches the actual configuration.
In `@orbslam3_ws/docker/compose.yaml`:
- Around line 117-123: Update the outdated TODO comment next to the device mount
change: since the service already uses privileged: true, replace or remove the
TODO that suggests commenting/uncommenting the selective mounts and the broad '-
/dev:/dev' line; specifically edit the compose.yaml comment block containing the
selective mounts (/dev/dri, /dev/snd, /dev/shm), the TODO line, and the '-
/dev:/dev' entry so the comment accurately reflects that broad device
passthrough is intentional when privileged: true (or document the security
implications and alternative of selective mounts if you want to keep that
option).
In `@turtlebot3_ws/docker/compose.yaml`:
- Around line 117-123: The TODO comment is stale because the catch‑all mount "-
/dev:/dev" is active while the individual device mounts are commented out;
update or remove the TODO so it accurately reflects current behavior by either
deleting the TODO and explaining that "/dev:/dev" is enabled (and why), or
change it to instruct maintainers to comment out "- /dev:/dev" if they prefer
fine‑grained mounts like "- /dev/dri:/dev/dri", "- /dev/snd:/dev/snd", and "-
/dev/shm:/dev/shm" (referencing the "- /dev:/dev" line and the three commented
mount lines).
In `@vlp_ws/docker/compose.yaml`:
- Around line 117-123: The TODO comment is now stale because /dev:/dev is
already uncommented and the three device mount lines are commented; update or
remove the TODO in the docker/compose.yaml block so it accurately reflects the
current configuration (either remove the instruction or change it to explain
that /dev:/dev enables USB support and the former individual mounts are
disabled). Locate the comment lines referencing "/dev/dri:/dev/dri",
"/dev/snd:/dev/snd", "/dev/shm:/dev/shm" and the active "- /dev:/dev" entry and
adjust the TODO text accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f721681-8605-47be-9dd4-ff10c10d55ef
📒 Files selected for processing (19)
README.mdaloha_ws/docker/compose.yamldelto_gripper_ws/docker/compose.yamldocker_modules/install_isaac_lab.shdocker_modules/install_isaac_sim.shdocs/docker-modules/isaac-lab.mddocs/docker-modules/isaac-sim.mdgazebo_world_ws/docker/compose.yamlgo2_ws/docker/compose.yamlh1_ws/docker/compose.yamlhusky_ws/docker/compose.yamlkobuki_ws/docker/compose.yamlorbslam3_ws/docker/compose.yamlstretch3_ws/docker/compose.yamltemplate_ws/docker/compose.yamltests/diff_base/docker/compose.yamlturtlebot3_ws/docker/compose.yamlur5_ws/docker/compose.yamlvlp_ws/docker/compose.yaml
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.
Closes: #115
Only tested basic examples:
Summary by CodeRabbit
New Features
developbranch.Documentation
Chores