fix: Dockerfile HuggingFace cache and generations directory permissions - #1017
fix: Dockerfile HuggingFace cache and generations directory permissions#1017virtuallyunknown wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe Dockerfile now creates the HuggingFace cache and voice-generation directories before application setup. It assigns both directories to the ChangesDocker runtime directory permissions
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
81-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate generations setup.
Lines 103-105 already create
/app/data/generationsand assign/app/datatovoicebox. This block adds an unnecessary image layer and does not improve permissions for mounted paths.Proposed cleanup
-# Create voice generations directory for the named volume -RUN mkdir -p /app/data/generations \ - && chown -R voicebox:voicebox /app/data/generations🤖 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 `@Dockerfile` around lines 81 - 83, Remove the duplicate generations directory setup block from the Dockerfile, including its mkdir and chown commands. Keep the existing setup later in the Dockerfile that creates /app/data/generations and assigns ownership of /app/data to voicebox.
🤖 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 `@Dockerfile`:
- Around line 77-83: Move runtime ownership initialization for
/home/voicebox/.cache/huggingface and /app/data/generations from the Dockerfile
into scripts/rocm-entrypoint.sh, after GPU setup and before the gosu voicebox
handoff. Ensure the entrypoint creates missing mount paths and migrates existing
bind mounts or named volumes to the voicebox UID/GID before starting the
application, including handling volumes that retain incorrect ownership.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 81-83: Remove the duplicate generations directory setup block from
the Dockerfile, including its mkdir and chown commands. Keep the existing setup
later in the Dockerfile that creates /app/data/generations and assigns ownership
of /app/data to voicebox.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # Create HuggingFace cache directory for the named volume | ||
| RUN mkdir -p /home/voicebox/.cache/huggingface \ | ||
| && chown -R voicebox:voicebox /home/voicebox/.cache/huggingface | ||
|
|
||
| # Create voice generations directory for the named volume | ||
| RUN mkdir -p /app/data/generations \ | ||
| && chown -R voicebox:voicebox /app/data/generations |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^Dockerfile$|docker-compose|^scripts/rocm-entrypoint\.sh$)'
echo
echo "== Dockerfile relevant sections =="
if [ -f Dockerfile ]; then
nl -ba Dockerfile | sed -n '1,140p'
fi
echo
echo "== compose mounts =="
for f in docker-compose.yml docker-compose.yaml; do
[ -f "$f" ] && { echo "--- $f"; nl -ba "$f" | sed -n '1,180p'; }
done
echo
echo "== entrypoint relevant snippets =="
for f in scripts/rocm-entrypoint.sh; do
[ -f "$f" ] && { echo "--- $f"; nl -ba "$f" | sed -n '1,220p'; }
done
echo
echo "== search ownership/mount hints =="
rg -n "generations|huggingface|voi(cebox|ce)|chown|mkdir -p|rocm-entrypoint|compose" .Repository: jamiepine/voicebox
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== line ranges for relevant files =="
for f in Dockerfile docker-compose.sh docker-compose.yml docker-compose.rocm.yml scripts/rocm-entrypoint.sh; do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
echo "== searches =="
rg -n "generations|huggingface|voicebox|huggingface-cache|output|chown|mkdir -p|rocm-entrypoint|entrypoint" . || true
echo "== script executable bits =="
find . -maxdepth 3 \( -name 'Dockerfile' -o -name 'docker-compose*.yml' -o -name 'rocm-entrypoint.sh' \) -exec ls -la {} \; 2>/dev/null || trueRepository: jamiepine/voicebox
Length of output: 50375
Initialize runtime-owned mount paths as voicebox before applying the volume migrations.
The image ownership at lines 77-83 and 104-105 does not apply to bind-mounted host directories like ./output:/app/data/generations, and existing named volumes can retain wrong ownership after the chown migrations. Run the runtime mount path initialization in scripts/rocm-entrypoint.sh after GPUs are handled but before gosu voicebox, and ensure existing volumes are migrated or recreated with the voicebox UID/GID.
🤖 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 `@Dockerfile` around lines 77 - 83, Move runtime ownership initialization for
/home/voicebox/.cache/huggingface and /app/data/generations from the Dockerfile
into scripts/rocm-entrypoint.sh, after GPU setup and before the gosu voicebox
handoff. Ensure the entrypoint creates missing mount paths and migrates existing
bind mounts or named volumes to the voicebox UID/GID before starting the
application, including handling volumes that retain incorrect ownership.
It's following the existing Dockerfile conventions of having a comment before each significant instruction. And the code/fix itself is taken from #542. No AI was involved in the making of this. |
1 participant
Fixes #542
Summary by CodeRabbit