Refresh bench tooling and Claude profiles#1
Conversation
|
@codex review |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates ROCm WSL configuration, mounts portable Claude profiles into the devcontainer, and refreshes bench tooling and container/runtime definitions to stay aligned with the parent workBenches setup. Flow diagram for updated ROCm WSL configuration preload variableflowchart TD
start[run configure-amd-rocm-wsl.sh] --> detectROCM[detect ROCM_ROOT]
detectROCM --> writeYAML[append ROCm config YAML]
writeYAML --> setPreload[set ROCM_HSA_PRELOAD in YAML]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- For the new ROCM_HSA_PRELOAD environment variable, consider adding a brief comment or helper script note explaining why LD_PRELOAD was avoided so future updates don’t inadvertently revert this change.
- The new .claude-profiles volume is mounted with :cached; if these profiles are not expected to be modified from inside the container, you may want to mount them read-only to avoid accidental edits.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the new ROCM_HSA_PRELOAD environment variable, consider adding a brief comment or helper script note explaining why LD_PRELOAD was avoided so future updates don’t inadvertently revert this change.
- The new .claude-profiles volume is mounted with :cached; if these profiles are not expected to be modified from inside the container, you may want to mount them read-only to avoid accidental edits.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Updates pyBench’s devcontainer/tooling setup to align with upstream workBenches changes, including portable Claude profile mounting and refreshed ROCm WSL handling.
Changes:
- Switch WSL ROCm preload handling from
LD_PRELOADto a dedicatedROCM_HSA_PRELOADvariable in the generated compose override and related diagnostics. - Refresh Python web stack tooling in
Dockerfile.layer2(usefastapi[standard]). - Mount
~/.claude-profilesinto the devcontainer (compose + devcontainer mounts) for portable Claude account/profile configuration.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/configure-amd-rocm-wsl.sh | Generates ROCm WSL docker-compose override; now exports ROCM_HSA_PRELOAD instead of setting LD_PRELOAD globally. |
| scripts/check-amd-rocm-wsl | Prints ROCM_HSA_PRELOAD and warns if LD_PRELOAD is globally set. |
| Dockerfile.layer2 | Updates pip-installed Python tooling to use fastapi[standard]. |
| .devcontainer/docker-compose.yml | Adds bind mount for ~/.claude-profiles into the container. |
| .devcontainer/devcontainer.json | Adds corresponding additionalMounts entry for ~/.claude-profiles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| HSA_ENABLE_DXG_DETECTION: "1" | ||
| HSA_ENABLE_SDMA: "0" | ||
| LD_PRELOAD: ${ROCM_ROOT}/lib/libhsa-runtime64.so.1 | ||
| ROCM_HSA_PRELOAD: ${ROCM_ROOT}/lib/libhsa-runtime64.so.1 | ||
| LD_LIBRARY_PATH: ${ROCM_ROOT}/lib:/usr/lib/x86_64-linux-gnu:/usr/lib |
| RUN python3 -m pip install --break-system-packages \ | ||
| pipx poetry black flake8 mypy pytest pytest-cov coverage pysonar ipython jupyter \ | ||
| django flask fastapi uvicorn pandas numpy | ||
| django flask 'fastapi[standard]' uvicorn pandas numpy |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acf0f50a15
ℹ️ 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".
| HSA_ENABLE_DXG_DETECTION: "1" | ||
| HSA_ENABLE_SDMA: "0" | ||
| LD_PRELOAD: ${ROCM_ROOT}/lib/libhsa-runtime64.so.1 | ||
| ROCM_HSA_PRELOAD: ${ROCM_ROOT}/lib/libhsa-runtime64.so.1 |
There was a problem hiding this comment.
Keep preloading HSA for ROCm containers
On WSL AMD ROCm hosts, this generated override now only exports ROCM_HSA_PRELOAD, but neither Docker Compose nor the dynamic linker consumes that variable; only LD_PRELOAD actually causes libhsa-runtime64.so.1 to be preloaded. Since there is no wrapper in the repo that maps ROCM_HSA_PRELOAD back to LD_PRELOAD, ROCm/Paddle commands launched normally in the devcontainer lose the preload that the previous generated config provided and can fail unless every invocation is manually prefixed.
Useful? React with 👍 / 👎.
| "source=${localEnv:HOME}/.agents,target=/home/${localEnv:USER}/.agents,type=bind,consistency=cached", | ||
| "source=${localEnv:HOME}/.claude,target=/home/${localEnv:USER}/.claude,type=bind,consistency=cached", | ||
| "source=${localEnv:HOME}/.claude.json,target=/home/${localEnv:USER}/.claude.json,type=bind,consistency=cached", | ||
| "source=${localEnv:HOME}/.claude-profiles,target=/home/${localEnv:USER}/.claude-profiles,type=bind,consistency=cached", |
There was a problem hiding this comment.
Avoid requiring optional Claude profile paths
For hosts that have the existing credential paths but have not created ~/.claude-profiles, this new Dev Containers bind mount makes startup fail because entries in mounts use Docker --mount semantics and the bind source must already exist. Since the profiles are only useful for users who opted into portable Claude profiles, create the host directory before startup or make this mount conditional rather than requiring every devcontainer user to have it.
Useful? React with 👍 / 👎.
Summary
Validation
git diff --checkbash -ndocker buildx build --checkSummary by Sourcery
Align bench tooling and devcontainer configuration with updated ROCm WSL setup and portable Claude profiles.
New Features:
Bug Fixes:
Enhancements: