Conversation
There was a problem hiding this comment.
Pull request overview
This PR (v25.12.19) refactors error handling in the voice extraction pipeline and fixes package installation issues for Jetson devices.
- Simplifies subprocess error handling by removing nested try-catch blocks and redundant return value checks
- Removes unused tqdm import from subprocess_pipe.py
- Fixes conda install command flag and adds numpy version constraint for older PyTorch versions on Jetson devices
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/classes/voice_extractor.py | Refactored error handling in _convert2wav, _demucs_voice, and normalize_audio methods by removing SubprocessPipe return value checks and simplifying control flow |
| lib/classes/subprocess_pipe.py | Removed unused tqdm import from imports |
| lib/classes/device_installer.py | Fixed conda install flag from '-d' to '-c' and added numpy version constraint for PyTorch 2.2.2 and earlier on Jetson devices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return True, msg | ||
| else: | ||
| error = f'_convert2wav() error:: {self.wav_file}' | ||
| SubprocessPipe(cmd, is_gui_process=self.session['is_gui_process'], total_duration=self._get_audio_duration(self.voice_file), msg='Convert') |
There was a problem hiding this comment.
The return value from SubprocessPipe is no longer being checked. According to the SubprocessPipe implementation, the _run_process method returns False when the process fails or is stopped, and True only when the process completes successfully with returncode 0. Without checking this return value, the code will proceed to check file existence even if the subprocess failed, which may lead to misleading error messages. The original implementation correctly verified the subprocess completed successfully before checking the output file.
| SubprocessPipe(cmd, is_gui_process=self.session['is_gui_process'], total_duration=self._get_audio_duration(self.voice_file), msg='Convert') | |
| success = SubprocessPipe( | |
| cmd, | |
| is_gui_process=self.session['is_gui_process'], | |
| total_duration=self._get_audio_duration(self.voice_file), | |
| msg='Convert' | |
| ) | |
| if not success: | |
| error = '_convert2wav ffmpeg process failed or was stopped before completion.' | |
| return False, error |
| error = f'_demucs_voice() subprocess FileNotFoundError error: The "demucs" command was not found. Ensure it is installed and in PATH.' | ||
| except Exception as e: | ||
| error = f'_demucs_voice() subprocess Exception error: {str(e)}' | ||
| SubprocessPipe(cmd, is_gui_process=self.session['is_gui_process'], total_duration=self._get_audio_duration(self.wav_file), msg='Denoising') |
There was a problem hiding this comment.
The return value from SubprocessPipe is no longer being checked. The _run_process method in SubprocessPipe returns False when the process fails and True when it succeeds. Without this check, the code assumes the demucs command always succeeds and immediately proceeds to construct the voice_track path and return success, even if the subprocess actually failed. This breaks the error handling logic and could lead to missing or incorrect files being used downstream.
| SubprocessPipe(cmd, is_gui_process=self.session['is_gui_process'], total_duration=self._get_audio_duration(self.wav_file), msg='Denoising') | |
| success = SubprocessPipe( | |
| cmd, | |
| is_gui_process=self.session['is_gui_process'], | |
| total_duration=self._get_audio_duration(self.wav_file), | |
| msg='Denoising' | |
| ) | |
| if not success: | |
| error = '_demucs_voice() subprocess error: demucs command failed.' | |
| return False, error |
| return True, msg | ||
| else: | ||
| error = f'normalize_audio() error: {dst_file}' | ||
| SubprocessPipe(cmd, is_gui_process=self.session['is_gui_process'], total_duration=self._get_audio_duration(src_file), msg='Normalize') |
There was a problem hiding this comment.
The return value from SubprocessPipe is no longer being checked. The SubprocessPipe._run_process method returns False on failure and True on success. Without checking this return value, the code will proceed to check file existence even if ffmpeg failed, potentially producing misleading error messages. The original logic correctly verified subprocess success before validating the output file.
| SubprocessPipe(cmd, is_gui_process=self.session['is_gui_process'], total_duration=self._get_audio_duration(src_file), msg='Normalize') | |
| success = SubprocessPipe( | |
| cmd, | |
| is_gui_process=self.session['is_gui_process'], | |
| total_duration=self._get_audio_duration(src_file), | |
| msg='Normalize' | |
| ) | |
| if not success: | |
| error = 'normalize_audio() error: ffmpeg failed during normalization.' | |
| return False, error |
No description provided.