fix(install): simplify DGX Spark setup, remove cgroup v2 workaround - #1394
Closed
zyang-dev wants to merge 3 commits into
Closed
fix(install): simplify DGX Spark setup, remove cgroup v2 workaround#1394zyang-dev wants to merge 3 commits into
zyang-dev wants to merge 3 commits into
Conversation
Signed-off-by: zyang-dev <267119621+zyang-dev@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThe pull request simplifies Docker configuration in the setup script by removing manual daemon.json modifications and Docker restart operations. Instead of handling cgroup namespace settings through Docker's daemon configuration, responsibility shifts to OpenShell's gateway container. Documentation is updated accordingly to reflect the streamlined setup process and architectural changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
kjw3
approved these changes
Apr 3, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-spark.sh`:
- Around line 64-69: The script prints a final success message even when the
earlier warn path for DOCKER_GROUP_ADDED was triggered; update the tailing
conditional so the info("DGX Spark Docker configuration complete.") is only
emitted when DOCKER_GROUP_ADDED is not true (i.e., when no warning was
previously issued/when the docker-group was already present or configuration
actually completed), and ensure the same DOCKER_GROUP_ADDED flag used earlier is
checked here so the warn branch cannot fall through to the info branch
(reference DOCKER_GROUP_ADDED, warn, and info).
In `@spark-install.md`:
- Around line 18-24: The Quick Start sequence runs setup-spark.sh then
immediately runs the NemoClaw installer, but if setup-spark.sh added your user
to the docker group the new group membership may not be active in the current
shell causing Docker permission errors; update the docs around the
setup-spark.sh step to add a short note instructing the user to open a new
terminal or run newgrp docker (or otherwise refresh their shell) if the script
reports they were added to the docker group before running the NemoClaw install
curl command.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46854f3f-62b3-421a-984d-b243e8303815
📒 Files selected for processing (2)
scripts/setup-spark.shspark-install.md
Comment on lines
+64
to
69
| echo "" | ||
| if [ "${DOCKER_GROUP_ADDED:-}" = true ]; then | ||
| warn "Docker group was just added. You must open a new terminal (or run 'newgrp docker') before continuing." | ||
| else | ||
| info "Creating Docker daemon config with cgroupns=host..." | ||
| mkdir -p "$(dirname "$DAEMON_JSON")" | ||
| echo '{ "default-cgroupns-mode": "host" }' >"$DAEMON_JSON" | ||
| NEEDS_RESTART=true | ||
| info "DGX Spark Docker configuration complete." | ||
| fi |
Contributor
There was a problem hiding this comment.
Avoid reporting full success when no non-root user was detected
If the warning path at Line 45 is hit, Line 68 still reports completion even though no docker-group configuration was applied. That can create a false-success path before install.
Suggested patch
echo ""
-if [ "${DOCKER_GROUP_ADDED:-}" = true ]; then
+if [ -z "$REAL_USER" ]; then
+ warn "Setup finished, but no non-root user was detected; docker group was not configured."
+elif [ "${DOCKER_GROUP_ADDED:-}" = true ]; then
warn "Docker group was just added. You must open a new terminal (or run 'newgrp docker') before continuing."
else
info "DGX Spark Docker configuration complete."
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| echo "" | |
| if [ "${DOCKER_GROUP_ADDED:-}" = true ]; then | |
| warn "Docker group was just added. You must open a new terminal (or run 'newgrp docker') before continuing." | |
| else | |
| info "Creating Docker daemon config with cgroupns=host..." | |
| mkdir -p "$(dirname "$DAEMON_JSON")" | |
| echo '{ "default-cgroupns-mode": "host" }' >"$DAEMON_JSON" | |
| NEEDS_RESTART=true | |
| info "DGX Spark Docker configuration complete." | |
| fi | |
| echo "" | |
| if [ -z "$REAL_USER" ]; then | |
| warn "Setup finished, but no non-root user was detected; docker group was not configured." | |
| elif [ "${DOCKER_GROUP_ADDED:-}" = true ]; then | |
| warn "Docker group was just added. You must open a new terminal (or run 'newgrp docker') before continuing." | |
| else | |
| info "DGX Spark Docker configuration complete." | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-spark.sh` around lines 64 - 69, The script prints a final
success message even when the earlier warn path for DOCKER_GROUP_ADDED was
triggered; update the tailing conditional so the info("DGX Spark Docker
configuration complete.") is only emitted when DOCKER_GROUP_ADDED is not true
(i.e., when no warning was previously issued/when the docker-group was already
present or configuration actually completed), and ensure the same
DOCKER_GROUP_ADDED flag used earlier is checked here so the warn branch cannot
fall through to the info branch (reference DOCKER_GROUP_ADDED, warn, and info).
Comment on lines
18
to
24
| ```bash | ||
| # Install OpenShell: | ||
| curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/main/install.sh | sh | ||
|
|
||
| # Clone NemoClaw: | ||
| git clone https://github.com/NVIDIA/NemoClaw.git | ||
| cd NemoClaw | ||
|
|
||
| # Spark-specific setup (fixes cgroup v2 and Docker permissions — see Troubleshooting for details) | ||
| sudo ./scripts/setup-spark.sh | ||
| # Spark-specific setup (requires sudo) | ||
| curl -fsSL https://raw.githubusercontent.com/NVIDIA/NemoClaw/main/scripts/setup-spark.sh | sudo bash | ||
|
|
||
| # Install NemoClaw: | ||
| ./install.sh | ||
|
|
||
| # Alternatively, you can use the hosted install script: | ||
| # Install NemoClaw | ||
| curl -fsSL https://www.nvidia.com/nemoclaw.sh | bash | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add an explicit shell-refresh note between Quick Start commands
After Line 20, docker group membership may not be active in the current shell yet. Running Line 23 immediately (especially via full-block paste) can fail with Docker permission errors.
Suggested doc tweak (keeps Quick Start to the same two commands)
```bash
# Spark-specific setup (requires sudo)
curl -fsSL https://raw.githubusercontent.com/NVIDIA/NemoClaw/main/scripts/setup-spark.sh | sudo bash
-
-# Install NemoClaw
-curl -fsSL https://www.nvidia.com/nemoclaw.sh | bash+If setup-spark.sh reports your user was just added to the docker group, open a new terminal (or run newgrp docker) before continuing.
+
+bash +# Install NemoClaw +curl -fsSL https://www.nvidia.com/nemoclaw.sh | bash +
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spark-install.md` around lines 18 - 24, The Quick Start sequence runs
setup-spark.sh then immediately runs the NemoClaw installer, but if
setup-spark.sh added your user to the docker group the new group membership may
not be active in the current shell causing Docker permission errors; update the
docs around the setup-spark.sh step to add a short note instructing the user to
open a new terminal or run newgrp docker (or otherwise refresh their shell) if
the script reports they were added to the docker group before running the
NemoClaw install curl command.
cv
approved these changes
Apr 3, 2026
Contributor
Author
|
Closing — the equivalent changes were merged in #1368. |
wscurran
added
bug-fix
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.
Summary
Related Issue
Changes
scripts/setup-spark.sh: Remove cgroup v2 daemon.json workaround and Docker restart logic. Keep docker group setup only.spark-install.md: Replace multi-step Quick Start with two curl commands. Update troubleshooting and architecture sections to reflect the cgroup fix is now in OpenShell.Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Signed-off-by: zyang-dev 267119621+zyang-dev@users.noreply.github.com
Summary by CodeRabbit
Chores
Documentation