Adds support for Linux Docker. - #1
Conversation
|
Please update this pr |
6c895e7 to
1760e9f
Compare
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📝 SummarySummary by CodeRabbit
WalkthroughAdds a reproducible CUDA 12.9.1 GPU Docker image with pinned dependencies, build-context exclusions, test execution, Linux runtime configuration, and a cross-platform separator output move operation. ChangesContainer runtime and portability
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Updated. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Dockerfile (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the Docker BuildKit syntax directive.
The Dockerfile uses the heredoc syntax (
RUN <<EOF), which is a BuildKit feature introduced in Dockerfile 1.4. While newer Docker versions default to BuildKit, explicitly declaring the syntax directive ensures compatibility across all build clients and resolves linting errors (like the Hadolint parsing error on line 64).🛠️ Proposed fix
+# syntax=docker/dockerfile:1 # =============================================================================🤖 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` at line 1, Add the Dockerfile BuildKit syntax directive for Dockerfile version 1.4 or newer before all existing content, so the heredoc RUN instructions parse correctly across build clients.Source: Linters/SAST tools
.dockerignore (1)
4-4: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIgnore virtual environment directories to optimize build context.
It's highly recommended to add virtual environment directories (like
venv/and.venv/) to.dockerignore. If a local virtual environment exists, it will be copied into the Docker build context, which can drastically slow down the build process and bloat the build context size.🚀 Proposed fix
__pycache__ +venv/ +.venv/🤖 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 @.dockerignore at line 4, Add virtual environment directory patterns such as venv/ and .venv/ to .dockerignore alongside __pycache__, ensuring local Python environments are excluded from the Docker build context.
🤖 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 64-80: Update the Dockerfile test stage to install pytest and
pytest-cov only for the test command, then remove the test dependencies and
assets after pytest completes. Combine test execution and cleanup in one RUN
instruction, removing pytest.ini and tests/ so the final image retains only
production files and dependencies.
In `@modules/transcription.py`:
- Line 49: Update the test covering the move operation in
test_transcription_behavior.py to patch modules.transcription.shutil.move
instead of os.rename, and assert that the mocked call receives the expected
destination path.
- Around line 47-49: Update the replacement flow around shutil.move in the
transcription output handling to preserve the existing dst_path until the new
file is fully staged. Copy or move src_path into a temporary path within
target_dir, then atomically install it with os.replace() only after staging
succeeds; remove the preemptive os.remove(dst_path) and retain the existing
output if staging fails.
---
Nitpick comments:
In @.dockerignore:
- Line 4: Add virtual environment directory patterns such as venv/ and .venv/ to
.dockerignore alongside __pycache__, ensuring local Python environments are
excluded from the Docker build context.
In `@Dockerfile`:
- Line 1: Add the Dockerfile BuildKit syntax directive for Dockerfile version
1.4 or newer before all existing content, so the heredoc RUN instructions parse
correctly across build clients.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dd3f6db-19cc-47ea-a42e-82b0e2e7fd00
📒 Files selected for processing (3)
.dockerignoreDockerfilemodules/transcription.py
📜 Review details
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile
[error] 64-64: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
🪛 Trivy (0.69.3)
Dockerfile
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🔇 Additional comments (2)
Dockerfile (1)
70-71: 🎯 Functional CorrectnessThe
sedreplacement is safe here — alldtype=dtype,occurrences inmodules/models.pyarefrom_pretrained(...)arguments, so this change doesn’t affect unrelated variables.> Likely an incorrect or invalid review comment.modules/transcription.py (1)
6-6: LGTM!
| if os.path.exists(dst_path): | ||
| os.remove(dst_path) | ||
| os.rename(src_path, dst_path) | ||
| shutil.move(src_path, dst_path) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' modules/transcription.py
printf '\n--- docs/instructions.md ---\n'
sed -n '1,120p' docs/instructions.mdRepository: ventura8/Auto-Subtitle-Generator
Length of output: 7143
🏁 Script executed:
python3 - <<'PY'
import os, shutil, tempfile, pathlib
# Read-only probe of Python semantics: show that shutil.move falls back to copy+remove
# for cross-filesystem moves by monkeypatching os.rename and observing the fallback.
calls = []
orig_rename = os.rename
orig_copy2 = shutil.copy2
orig_rmtree = shutil.rmtree
def fake_rename(src, dst):
calls.append(("rename", src, dst))
raise OSError("cross-device link")
def fake_copy2(src, dst, *, follow_symlinks=True):
calls.append(("copy2", src, dst))
return dst
def fake_rmtree(path, *args, **kwargs):
calls.append(("rmtree", path))
return None
os.rename = fake_rename
shutil.copy2 = fake_copy2
shutil.rmtree = fake_rmtree
with tempfile.TemporaryDirectory() as td:
src = pathlib.Path(td) / "src.txt"
dst = pathlib.Path(td) / "dst.txt"
src.write_text("hello")
try:
shutil.move(str(src), str(dst))
except Exception as e:
calls.append(("exception", type(e).__name__, str(e)))
print(calls)
PYRepository: ventura8/Auto-Subtitle-Generator
Length of output: 239
🏁 Script executed:
rg -n "atomic|separator output|shutil.move|transcription" -S docs modulesRepository: ventura8/Auto-Subtitle-Generator
Length of output: 2524
🏁 Script executed:
sed -n '1,120p' modules/transcription.pyRepository: ventura8/Auto-Subtitle-Generator
Length of output: 4091
🏁 Script executed:
python3 - <<'PY'
import inspect, shutil
print(inspect.getsource(shutil.move))
PYRepository: ventura8/Auto-Subtitle-Generator
Length of output: 2867
🏁 Script executed:
python3 - <<'PY'
import os, shutil, tempfile, pathlib, inspect
print("shutil.move source:")
print(inspect.getsource(shutil.move))
print("\n--- behavior probe ---")
calls = []
orig_rename = os.rename
orig_copy2 = shutil.copy2
orig_copytree = shutil.copytree
orig_rmtree = shutil.rmtree
def fake_rename(src, dst):
calls.append(("rename", os.path.basename(src), os.path.basename(dst)))
raise OSError(18, "Invalid cross-device link")
def fake_copy2(src, dst, *, follow_symlinks=True):
calls.append(("copy2", os.path.basename(src), os.path.basename(dst)))
# emulate successful overwrite
pathlib.Path(dst).write_text("new")
return dst
os.rename = fake_rename
shutil.copy2 = fake_copy2
with tempfile.TemporaryDirectory() as td:
td = pathlib.Path(td)
src = td / "src.txt"
dst = td / "dst.txt"
src.write_text("source")
dst.write_text("existing")
try:
result = shutil.move(str(src), str(dst))
calls.append(("result", os.path.basename(result)))
except Exception as e:
calls.append(("exception", type(e).__name__, str(e)))
print(calls)
print("dst content:", dst.read_text() if dst.exists() else "<missing>")
print("src exists:", src.exists())
os.rename = orig_rename
shutil.copy2 = orig_copy2
shutil.copytree = orig_copytree
shutil.rmtree = orig_rmtree
PYRepository: ventura8/Auto-Subtitle-Generator
Length of output: 3007
🏁 Script executed:
python3 - <<'PY'
import os, shutil, tempfile, pathlib, inspect
print("shutil.move source:")
print(inspect.getsource(shutil.move))
print("\n--- behavior probe ---")
calls = []
orig_rename = os.rename
orig_copy2 = shutil.copy2
def fake_rename(src, dst):
calls.append(("rename", os.path.basename(src), os.path.basename(dst)))
raise OSError(18, "Invalid cross-device link")
def fake_copy2(src, dst, *, follow_symlinks=True):
calls.append(("copy2", os.path.basename(src), os.path.basename(dst)))
pathlib.Path(dst).write_text("new")
return dst
os.rename = fake_rename
shutil.copy2 = fake_copy2
with tempfile.TemporaryDirectory() as td:
td = pathlib.Path(td)
src = td / "src.txt"
dst = td / "dst.txt"
src.write_text("source")
dst.write_text("existing")
try:
result = shutil.move(str(src), str(dst))
calls.append(("result", os.path.basename(result)))
except Exception as e:
calls.append(("exception", type(e).__name__, str(e)))
print(calls)
print("dst content:", dst.read_text() if dst.exists() else "<missing>")
print("src exists:", src.exists())
os.rename = orig_rename
shutil.copy2 = orig_copy2
PYRepository: ventura8/Auto-Subtitle-Generator
Length of output: 3007
Keep the existing output until replacement succeeds. In modules/transcription.py:47-49, shutil.move() can fall back to copy+delete on cross-volume moves; deleting dst_path first can discard the last valid separator output if the copy is interrupted. Stage the file in target_dir and os.replace() it into place instead.
🤖 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 `@modules/transcription.py` around lines 47 - 49, Update the replacement flow
around shutil.move in the transcription output handling to preserve the existing
dst_path until the new file is fully staged. Copy or move src_path into a
temporary path within target_dir, then atomically install it with os.replace()
only after staging succeeds; remove the preemptive os.remove(dst_path) and
retain the existing output if staging fails.
| if os.path.exists(dst_path): | ||
| os.remove(dst_path) | ||
| os.rename(src_path, dst_path) | ||
| shutil.move(src_path, dst_path) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the test to patch shutil.move.
tests/test_transcription_behavior.py:96-106 still patches os.rename; after this change, the real shutil.move runs against the test’s fake paths and can raise FileNotFoundError. Patch modules.transcription.shutil.move and assert the expected destination.
🤖 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 `@modules/transcription.py` at line 49, Update the test covering the move
operation in test_transcription_behavior.py to patch
modules.transcription.shutil.move instead of os.rename, and assert that the
mocked call receives the expected destination path.
|
I rebased and had my agent address the comments, but I won't be able to test the docker build for a couple weeks. I'll set a reminder to myself to do this when I can, but I didn't want you to think I had abandoned this PR. |
e8c7d8d to
df902fe
Compare
The change in `transcription.py` makes it work when the file is moving across volumes (disks). `os.rename` only works if the source/destination are on the same volume. Pinned versions in `requirements.txt` were required to get this all working reliably, and is generally a good practice for reproducible builds and minimizing chance that something breaks suddenly. I spent about 2 days figuring out how to get this all working with the right version of things and I wouldn't want others to waste a similar amount of time. The main addition is the `Dockerfile`, which works for `amd64`. It is ridiculously large, given how simple its job is and that it doesn't include any actual models, but that is pytorch and cuda for you. It has to patch `requirements.txt` because faster-whisper will bring in the wrong version of onnx, so we have to manually install it and its unfullfilled dependencies. It also has to patch `modules/models.py` because the codebase switched to a newer version of pytorch it seems. I would be happy to move this change into the main repository, but I wanted to keep this PR as simple as possible.
|
Rebased against latest |
2 participants
The change in
transcription.pyis a bugfix that makes it work when the file is moving across volumes (disks).os.renameonly works if the source/destination are on the same logical volume.Pinned versions in
requirements.txtwere required to get this all working reliably, and is generally a good practice for reproducible builds and minimizing chance that something breaks suddenly. I spent about 2 days figuring out how to get this all working with the right version of things and I wouldn't want others to waste a similar amount of time.The main addition is the
Dockerfile, which works foramd64. It is ridiculously large, given how simple this project is and it doesn't even include any actual models, but that is pytorch and cuda for you.It has to patch
requirements.txtbecause faster-whisper will bring in the wrong version of onnx, so we have to manually install it and its dependencies.It also has to patch
modules/models.pybecause the codebase is built for a newer version of pytorch than I was able to get working. I would be happy to move this change into the main repository, but I wanted to minimize the chance that the PR broke something in other environments.