Fixed 'torchcodec' not supported issue on XPU and ROCm when calling torchaudio.save - #1242
Fixed 'torchcodec' not supported issue on XPU and ROCm when calling torchaudio.save#1242fisheryv wants to merge 3 commits into
torchaudio.save#1242Conversation
…th torchcodec as the priority and soundfile/ffmpeg as fallbacks, to address the issue where XPU and ROCm do not support torchcodec, or torchcodec incompatibility prevents saving generated audio.
📝 WalkthroughWalkthroughThe PR adds runtime torchcodec detection and dual-path audio export. Saves use torchaudio when available and soundfile/ffmpeg otherwise, with format-specific fallbacks, expanded backend tests, and hardened smoke-test WAV export. ChangesAudio Export Backend Selection and Fallback Pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acestep/audio_utils.py (1)
437-458:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback handler will fail for opus/aac formats.
If
_save_via_ffmpegfails for opus/aac (e.g., ffmpeg not found), the exception handler at line 437 catches it and attemptssf.write(..., format=format.upper())where format is "OPUS" or "AAC". However, soundfile does not support these formats—it will raiseValueError: Unknown formator similar, masking the original ffmpeg error.Consider re-raising immediately for formats that soundfile cannot handle:
🐛 Proposed fix
except Exception as e: if format == "mp3": logger.error(f"[AudioSaver] MP3 export failed without fallback: {e}") raise + if format in ["opus", "aac"]: + logger.error(f"[AudioSaver] {format.upper()} export failed (ffmpeg required): {e}") + raise try: import soundfile as sf audio_np = audio_tensor.transpose(0, 1).numpy() # -> [samples, channels]🤖 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 `@acestep/audio_utils.py` around lines 437 - 458, The fallback path in the except block after _save_via_ffmpeg attempts to call soundfile for formats it doesn't support (e.g., "opus" or "aac"), which masks the original ffmpeg error; update the handler in the except Exception as e: block to check format.lower() and if it's an unsupported soundfile format (at minimum "opus" and "aac") immediately log and re-raise the original exception `e` instead of trying sf.write; otherwise proceed with the existing soundfile fallback logic that transposes audio_tensor and calls sf.write.
🤖 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.
Outside diff comments:
In `@acestep/audio_utils.py`:
- Around line 437-458: The fallback path in the except block after
_save_via_ffmpeg attempts to call soundfile for formats it doesn't support
(e.g., "opus" or "aac"), which masks the original ffmpeg error; update the
handler in the except Exception as e: block to check format.lower() and if it's
an unsupported soundfile format (at minimum "opus" and "aac") immediately log
and re-raise the original exception `e` instead of trying sf.write; otherwise
proceed with the existing soundfile fallback logic that transposes audio_tensor
and calls sf.write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45f1de59-be6b-42b0-8808-dc9008558f1e
📒 Files selected for processing (3)
acestep/audio_utils.pyacestep/audio_utils_test.pyscripts/flow_edit_overlay_smoke.py
|
This solved the issue here, where I otherwise had zero progress on getting it working: Arch Linux / 20GB AMD 7900 XT Followed https://github.com/ace-step/ACE-Step-1.5/blob/main/docs/en/ACE-Step1.5-Rocm-Manual-Linux.md |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acestep/audio_utils.py (1)
43-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the temp file before probing
torchaudio.save(acestep/audio_utils.py:43-51)
In torchaudio 2.9,backend="soundfile"is ignored, so this probe still runs through the torchcodec-backed save path while theNamedTemporaryFilehandle is open. On Windows that can raisePermissionError/OSErrorand makeacestep.audio_utilsunimportable. Usedelete=False(or a temp dir), close the file before saving, and broaden the exception handling here.🤖 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 `@acestep/audio_utils.py` around lines 43 - 51, Update the torchaudio availability probe to create the temporary WAV with delete=False, close the NamedTemporaryFile before calling torchaudio.save, and remove the temporary file afterward (preferably in a finally block). Broaden the probe’s exception handling to include OSError/PermissionError and other relevant save failures so importing acestep.audio_utils cannot fail due to platform-specific temporary-file or backend errors.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@acestep/audio_utils.py`:
- Around line 43-51: Update the torchaudio availability probe to create the
temporary WAV with delete=False, close the NamedTemporaryFile before calling
torchaudio.save, and remove the temporary file afterward (preferably in a
finally block). Broaden the probe’s exception handling to include
OSError/PermissionError and other relevant save failures so importing
acestep.audio_utils cannot fail due to platform-specific temporary-file or
backend errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c846250c-441a-4067-8aa2-74a8171027b8
📒 Files selected for processing (2)
acestep/audio_utils.pyacestep/audio_utils_test.py
2 participants
In torchaudio 2.9+ and later versions,
torchaudio.save()internally callssave_with_torchcodec, even if thebackend='soundfile'orbackend='ffmpeg'parameter is specified, which will be ignored. See https://docs.pytorch.org/audio/stable/generated/torchaudio.save.html#torchaudio.save. Whentorchcodecis not installed (e.g., XPU and ROCm do not supporttorchcodec), this results in anImportError: TorchCodec is required for save_with_torchcodecorModuleNotFoundError: No module named 'torchcodec'.I implemented a dual-path compatibility scheme with
torchcodecas the priority andsoundfile/ffmpegas fallbacks, to address the issue where XPU and ROCm do not supporttorchcodec, ortorchcodecincompatibility prevents saving generated audio.Related issues: #665
Summary by CodeRabbit
Bug Fixes
Tests