chore(devcontainer): helper script for driving the dev container from outside sessions#63
Conversation
… outside sessions Agent sessions without Remote-Containers support (e.g. running outside the dev container) need a reliable way to run commands in the existing GPU/uv environment. Add scripts/devcontainer.sh wrapping the devcontainer CLI: - up/status/down for lifecycle management - exec/shell for running commands with correct user, cwd, and remoteEnv (GPU/CUDA vars), which plain 'docker exec' silently drops - auto-resolves the main worktree checkout the container was built against, so it works correctly even when invoked from a linked 'git worktree' Document the workflow in AGENTS.md under Tooling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lper nvidia-smi succeeding inside a container doesn't guarantee a CUDA context can actually be created — after host-side kernel/driver churn (e.g. an unattended-upgrades kernel update rebuilding the NVIDIA DKMS module without a reboot), a long-running container can end up with stale device bindings that nvidia-smi doesn't catch. - gpu-check: nvidia-smi + a real TensorFlow CUDA context init. - gpu-recover: try a cheap 'docker restart' first (re-runs the NVIDIA container-runtime hook), fall back to a full stop + devcontainer up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a host-side helper script to reliably drive the existing shared dev container (including remoteEnv GPU/CUDA environment) from sessions running outside the container, and documents the workflow for agent usage in AGENTS.md.
Changes:
- Introduces
scripts/devcontainer.shwrapper fordevcontainer up/exec/shell/down/statusthat resolves the main worktree and targets the correct running container by ID. - Adds
gpu-check/gpu-recoversubcommands to detect and recover from stale GPU device bindings in long-running containers. - Updates
AGENTS.mdTooling section with recommended usage for outside-container agent sessions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/devcontainer.sh | Adds a devcontainer-driving helper script with worktree resolution and GPU check/recovery utilities. |
| AGENTS.md | Documents the new workflow for running commands in the dev container from outside sessions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if git -C "$(dirname "${BASH_SOURCE[0]}")" rev-parse --is-inside-work-tree >/dev/null 2>&1; then | ||
| git -C "$(dirname "${BASH_SOURCE[0]}")" worktree list --porcelain \ | ||
| | awk '/^worktree /{print $2; exit}' | ||
| fi |
| echo "-- CUDA context init via TensorFlow (real usability check) --" | ||
| if ! devcontainer exec --container-id "${id}" --workspace-folder "${MAIN_WORKSPACE}" -- \ | ||
| uv run python -c " | ||
| import tensorflow as tf | ||
| gpus = tf.config.list_physical_devices('GPU') | ||
| assert gpus, 'no GPU visible to TensorFlow' | ||
| with tf.device('/GPU:0'): | ||
| x = tf.constant([1.0, 2.0]) + tf.constant([3.0, 4.0]) | ||
| x.numpy() | ||
| print('CUDA context OK:', gpus) | ||
| " 2>&1 | tail -20; then |
Addresses findings from a dedicated code-review pass on PR #63: - resolve_main_worktree: the awk parser used field-splitting ($2) to extract the worktree path from 'git worktree list --porcelain' output, which silently truncates any workspace path containing spaces. Now strips only the 'worktree ' prefix and prints the rest of the line verbatim. - container_id(): now guards against multiple containers matching the same workspace-folder label (e.g. a leftover container from a previous 'up'), failing loudly instead of feeding a multi-line ID string into --container-id/docker stop and producing confusing downstream errors. - gpu-recover: the 'no running container' cold-start path now runs gpu-check after bringing the container up, matching the other two recovery paths. Previously it returned cmd_up's exit status directly, which only proves the container was created, not that the underlying GPU/CUDA issue gpu-recover exists to fix is actually resolved. Verified: bash -n syntax check, dogfooded up/status/id/exec/gpu-check against the real shared container, and isolated unit checks for the awk path-with-spaces fix and the multi-container guard's three cases (multi-line/single/empty). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7dc890204
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fi | ||
| echo "docker restart did not fix it; falling back to full stop + devcontainer up (recreates the container)..." | ||
| docker stop "${id}" >/dev/null 2>&1 || true | ||
| cmd_up |
There was a problem hiding this comment.
When gpu-recover reaches this fallback after docker restart did not restore CUDA, docker stop followed by devcontainer up does not force a fresh container. I checked the devcontainers/cli openDockerfileDevContainer path: findExistingContainer includes stopped containers, and startExistingContainer starts them with docker start, so this retries the same stale device bindings instead of the advertised full recreate; remove/down the container or pass up --remove-existing-container before checking again.
Useful? React with 👍 / 👎.
Codex review on PR #63 correctly identified that the 'full recreate' fallback in gpu-recover did not actually recreate anything: devcontainer up finds and restarts an existing (even stopped) container for a given workspace rather than creating a fresh one, so 'docker stop' + 'devcontainer up' just restarted the same container with its stale device bindings intact -- contradicting the script's own comment claiming a from-scratch recreate. Fix: docker rm the container (in addition to stop) before calling cmd_up, so devcontainer up has no existing container to find and must create a new one. Verified the normal up/gpu-check path still works correctly against the real shared container (including the ordinary restart-a-stopped-container case, which is the behavior this fix deliberately does NOT change for cmd_up itself -- only gpu-recover's last-resort fallback now forces a true recreate). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed all review findings:
Verified the Copilot comment about inline-Python indentation in All fixes dogfooded against the real shared dev container (up/status/exec/gpu-check), CI green. Merging. |
Summary
Adds
scripts/devcontainer.sh, a small helper for agent sessions (or any shell) that run outside the dev container but need to drive the existing shared GPU container reliably — e.g. Copilot CLI/agent sessions without VS Code Remote-Containers support.Why
devcontainer execlooks up a running container by matching the workspace-folder path baked into its labels at creation time. Run it from a linkedgit worktree(a different path than the one the container was created for) and it fails with "Dev container not found" even though the container is running.docker execworks from anywhere but silently dropsremoteEnvvalues fromdevcontainer.json(e.g.TF_FORCE_GPU_ALLOW_GROWTH, the CUDALD_LIBRARY_PATHaddition) since it has no knowledge of them — this matters for GPU/training workflows.nvidia-smisucceeding inside a container doesn't guarantee a real CUDA context can be created — after host-side kernel/driver churn (e.g. an unattended-upgrades kernel update rebuilding the NVIDIA DKMS module without a reboot), a long-running container can end up with stale device bindings thatnvidia-smidoesn't catch.What's included
scripts/devcontainer.sh {up|id|exec|shell|down|status}— always resolves the main worktree checkout (viagit worktree list) that the container was actually built against, and passes--container-idexplicitly, so it works correctly regardless of which worktree/directory it's invoked from.scripts/devcontainer.sh gpu-check—nvidia-smiplus a real TensorFlow CUDA context init, to catch the "stale device binding" failure mode nvidia-smi alone misses.scripts/devcontainer.sh gpu-recover— auto-heal: cheapdocker restartfirst (re-runs the NVIDIA container-runtime hook), falls back to a full stop +devcontainer up(recreate) if that doesn't fix it.## Toolingdocumenting this workflow for future agent sessions.Testing
up,status,exec(verified correct cwd/user/remoteEnv),gpu-check(verified both the healthy and OK-after-restart paths), used repeatedly to run the full test suite and training sweeps inside the container.bash -nsyntax check passes; no repo lint/test suite applies to this shell script (not part of the Python package).Not in this PR
docker.serviceordering againstnvidia-persistenced) were applied directly to this machine's host config with the user's approval — they're host-level system changes, not repo config, so there's nothing to commit for them here.