feat/dl-add-whisper-transcription-model#164
Conversation
- Add SAMO-optimized Whisper audio transcription model - Create src/models/transcription/whisper_transcriber.py with full functionality - Add configs/whisper_config.yaml with transcription settings - Create test_whisper_standalone.py for comprehensive testing - Update requirements-audio.txt with Whisper dependencies - Support multiple audio formats (WAV, MP3, M4A, FLAC, OGG, WMA) - Auto-detect device (CPU, CUDA, MPS) with proper dtype handling - Support batch transcription and language auto-detection - Performance: 0.85 confidence, 3-16s processing time on MPS - Resolves PR-2 from surgical breakdown plan
Reviewer's GuideThis PR introduces a SAMO-optimized Whisper transcription model by adding a YAML-driven configuration and environment setup, implementing device- and dtype-aware audio processing and transcription within a new transcriber class, and providing standalone tests along with updated audio dependencies. Entity relationship diagram for whisper_config.yaml structureerDiagram
MODEL {
string name
string device
string torch_dtype
}
AUDIO {
int sample_rate
float max_duration
float chunk_length
float stride_length
}
TRANSCRIPTION {
string language
string task
boolean return_timestamps
boolean return_language
float chunk_length_s
float stride_length_s
}
SAMO_OPTIMIZATIONS {
string log_level
boolean enable_chunking
boolean enable_vad
float confidence_threshold
int max_retries
}
PERFORMANCE {
int batch_size
int num_workers
boolean pin_memory
}
SUPPORTED_FORMATS {
string format
}
WHISPER_CONFIG {
MODEL
AUDIO
TRANSCRIPTION
SAMO_OPTIMIZATIONS
PERFORMANCE
SUPPORTED_FORMATS
}
WHISPER_CONFIG ||--|{ MODEL : contains
WHISPER_CONFIG ||--|{ AUDIO : contains
WHISPER_CONFIG ||--|{ TRANSCRIPTION : contains
WHISPER_CONFIG ||--|{ SAMO_OPTIMIZATIONS : contains
WHISPER_CONFIG ||--|{ PERFORMANCE : contains
WHISPER_CONFIG ||--|{ SUPPORTED_FORMATS : contains
Class diagram for SAMOWhisperTranscriber and related functionsclassDiagram
class SAMOWhisperTranscriber {
- config: Dict[str, Any]
- model: WhisperForConditionalGeneration
- processor: WhisperProcessor
- device: str
+ __init__(config_path)
+ transcribe_audio(audio_path, language, return_timestamps)
+ transcribe_batch(audio_paths, language, return_timestamps)
- _load_model()
- _load_audio(audio_path)
- _preprocess_audio(audio_array, sample_rate)
- _extract_timestamps(generated_ids)
- _calculate_confidence(generated_ids)
}
class create_samo_whisper_transcriber {
+ create_samo_whisper_transcriber(config_path)
}
SAMOWhisperTranscriber <.. create_samo_whisper_transcriber: returns
class WhisperProcessor
class WhisperForConditionalGeneration
SAMOWhisperTranscriber --> WhisperProcessor: uses
SAMOWhisperTranscriber --> WhisperForConditionalGeneration: uses
class yaml
class torch
SAMOWhisperTranscriber ..> yaml: uses
SAMOWhisperTranscriber ..> torch: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdds a Whisper-based transcription capability: a YAML config for model/audio/transcription settings, audio ML dependencies, a new SAMOWhisperTranscriber module with single and batch transcription, and a standalone test script that loads the config and exercises both per-file and batch flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Test as test_whisper_standalone.py
participant Transcriber as SAMOWhisperTranscriber
participant YAML as Config Loader
participant Audio as librosa
participant Proc as WhisperProcessor
participant Model as WhisperModel
User->>Test: Run tests
Test->>YAML: Load configs/whisper_config.yaml
YAML-->>Test: Config dict
Test->>Transcriber: create_samo_whisper_transcriber(config)
Transcriber->>Transcriber: Determine device / dtype
Transcriber->>Proc: Load processor
Transcriber->>Model: Load/generate model on device
Test->>Transcriber: transcribe_audio(path, language="auto")
Transcriber->>Audio: Load waveform (mono, sr)
Audio-->>Transcriber: y, sr
Transcriber->>Transcriber: Resample/normalize
Transcriber->>Proc: Prepare inputs
Proc-->>Transcriber: Tensors on device
Transcriber->>Model: Generate (with/without timestamps)
Model-->>Transcriber: Tokens / outputs
Transcriber->>Proc: Decode text (+optional timestamps)
Proc-->>Transcriber: Text (+segments)
Transcriber-->>Test: {text, language, timestamps?, durations, confidence}
Test-->>User: Print results and summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @uelkerd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request integrates a new, optimized Whisper audio transcription model into the system. Its primary purpose is to enable robust and efficient conversion of various audio formats into text, supporting different hardware configurations and offering flexible language detection. This addition significantly enhances the system's audio processing capabilities, laying foundational groundwork for subsequent features.
Highlights
- New Whisper Transcription Model: Introduces a SAMO-optimized Whisper audio transcription model for high-performance audio processing.
- Multi-format and Device Support: Supports multiple audio formats (WAV, MP3, M4A, FLAC, OGG, WMA) and features auto-detection for CPU, CUDA, and MPS devices with proper dtype handling.
- Advanced Transcription Features: Includes batch transcription capabilities, automatic and manual language detection, and comprehensive error handling and logging.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements a SAMO-optimized Whisper audio transcription model with comprehensive audio processing capabilities, device auto-detection, and batch processing support for journal audio transcription needs.
Key changes:
- Adds complete Whisper transcription model with device optimization and error handling
- Implements configuration-driven approach with YAML-based settings
- Provides standalone testing infrastructure for model validation
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/models/transcription/whisper_transcriber.py | Core Whisper model implementation with SAMO optimizations, device management, and audio processing |
| configs/whisper_config.yaml | Configuration file defining model, audio, and transcription settings |
| test_whisper_standalone.py | Standalone test script for validation and testing functionality |
| dependencies/requirements-audio.txt | Updated audio dependencies including Whisper-specific packages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| audio_array = torch.from_numpy(audio_array).float() | ||
|
|
||
| # Normalize audio | ||
| audio_array = audio_array / torch.max(torch.abs(audio_array)) |
There was a problem hiding this comment.
Division by zero will occur if the audio array contains only zeros (silence). Add a check to prevent division by zero by using a small epsilon value or checking if max is zero.
| audio_array = audio_array / torch.max(torch.abs(audio_array)) | |
| epsilon = 1e-8 | |
| max_val = torch.max(torch.abs(audio_array)) | |
| audio_array = audio_array / (max_val + epsilon) |
| # This is a simplified implementation | ||
| # In practice, you'd need to decode the timestamp tokens | ||
| return [] |
There was a problem hiding this comment.
The method always returns an empty list but the transcribe_audio method uses this return value and the API promises timestamps when return_timestamps=True. This will mislead users expecting actual timestamp data.
| # This is a simplified implementation | |
| # In practice, you'd need to decode the timestamp tokens | |
| return [] | |
| # Use the processor to decode with timestamps | |
| decoded = self.processor.decode(generated_ids[0], skip_special_tokens=True, return_timestamps=True) | |
| # decoded is a dict with 'chunks' if return_timestamps=True | |
| timestamps = [] | |
| if isinstance(decoded, dict) and "chunks" in decoded: | |
| for chunk in decoded["chunks"]: | |
| # Each chunk is a dict with 'text', 'timestamp' (tuple of start, end) | |
| timestamps.append({ | |
| "word": chunk.get("text", ""), | |
| "start": chunk.get("timestamp", (None, None))[0], | |
| "end": chunk.get("timestamp", (None, None))[1] | |
| }) | |
| return timestamps |
| def _calculate_confidence(self, generated_ids: torch.Tensor) -> float: | ||
| """Calculate confidence score for transcription.""" | ||
| # Simplified confidence calculation | ||
| # In practice, you'd use the model's logits | ||
| return 0.85 |
There was a problem hiding this comment.
The method returns a hardcoded confidence value of 0.85 regardless of actual transcription quality. This provides misleading confidence information to users and could affect decision-making based on transcription reliability.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The stub methods
_extract_timestampsand_calculate_confidencereturn hardcoded values; implement real logic or remove these placeholders to avoid misleading downstream consumers. - You have duplicated chunk and stride settings under both
audioandtranscriptionin the config—consider consolidating these parameters into one section for clarity and maintainability. - The model move logic checks
device != "auto"even though_get_devicenever returns 'auto'; simplify this to always move the model to the resolved device.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The stub methods `_extract_timestamps` and `_calculate_confidence` return hardcoded values; implement real logic or remove these placeholders to avoid misleading downstream consumers.
- You have duplicated chunk and stride settings under both `audio` and `transcription` in the config—consider consolidating these parameters into one section for clarity and maintainability.
- The model move logic checks `device != "auto"` even though `_get_device` never returns 'auto'; simplify this to always move the model to the resolved device.
## Individual Comments
### Comment 1
<location> `src/models/transcription/whisper_transcriber.py:293` </location>
<code_context>
+ )
+ audio_array = torch.from_numpy(audio_array).float()
+
+ # Normalize audio
+ audio_array = audio_array / torch.max(torch.abs(audio_array))
+
+ return audio_array
</code_context>
<issue_to_address>
Potential division by zero in audio normalization.
If the input is silent, normalization will cause NaNs due to division by zero. Add an epsilon or check for zero before dividing.
</issue_to_address>
### Comment 2
<location> `src/models/transcription/whisper_transcriber.py:300` </location>
<code_context>
+
+ def _extract_timestamps(self, generated_ids: torch.Tensor) -> List[Dict[str, Any]]:
+ """Extract word-level timestamps from generated IDs."""
+ # This is a simplified implementation
+ # In practice, you'd need to decode the timestamp tokens
+ return []
+
+ def _calculate_confidence(self, generated_ids: torch.Tensor) -> float:
</code_context>
<issue_to_address>
Timestamp extraction is stubbed and always returns an empty list.
If return_timestamps is True, clarify that timestamp extraction is not implemented by raising NotImplementedError or updating the documentation.
</issue_to_address>
### Comment 3
<location> `src/models/transcription/whisper_transcriber.py:306` </location>
<code_context>
+
+ def _calculate_confidence(self, generated_ids: torch.Tensor) -> float:
+ """Calculate confidence score for transcription."""
+ # Simplified confidence calculation
+ # In practice, you'd use the model's logits
+ return 0.85
+
+
</code_context>
<issue_to_address>
Confidence calculation is hardcoded and not based on model output.
A fixed confidence value could misrepresent the model's certainty. Please remove the confidence field or compute it from model outputs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Normalize audio | ||
| audio_array = audio_array / torch.max(torch.abs(audio_array)) |
There was a problem hiding this comment.
issue (bug_risk): Potential division by zero in audio normalization.
If the input is silent, normalization will cause NaNs due to division by zero. Add an epsilon or check for zero before dividing.
| # This is a simplified implementation | ||
| # In practice, you'd need to decode the timestamp tokens | ||
| return [] |
There was a problem hiding this comment.
issue: Timestamp extraction is stubbed and always returns an empty list.
If return_timestamps is True, clarify that timestamp extraction is not implemented by raising NotImplementedError or updating the documentation.
| # Simplified confidence calculation | ||
| # In practice, you'd use the model's logits | ||
| return 0.85 |
There was a problem hiding this comment.
issue: Confidence calculation is hardcoded and not based on model output.
A fixed confidence value could misrepresent the model's certainty. Please remove the confidence field or compute it from model outputs.
| for audio_file in sample_files: | ||
| audio_path = REPO_ROOT / audio_file | ||
| if audio_path.exists(): | ||
| print(f"\n🎵 Testing with {audio_file}") | ||
| print("-" * 30) | ||
|
|
||
| try: | ||
| # Transcribe audio | ||
| result = transcriber.transcribe_audio( | ||
| str(audio_path), | ||
| language="auto", | ||
| return_timestamps=True | ||
| ) | ||
|
|
||
| print(f"📝 Transcription: {result['text']}") | ||
| print(f"🌍 Language: {result['language']}") | ||
| print(f"⏱️ Processing time: {result['processing_time']:.2f}s") | ||
| print(f"🎯 Confidence: {result['confidence']:.3f}") | ||
| print(f"⏰ Audio duration: {result['audio_duration']:.2f}s") | ||
|
|
||
| if result['timestamps']: | ||
| print(f"📍 Timestamps: {len(result['timestamps'])} segments") | ||
|
|
||
| except Exception as e: | ||
| print(f"❌ Error transcribing {audio_file}: {e}") | ||
|
|
||
| else: | ||
| print(f"⚠️ Sample file not found: {audio_file}") |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if audio_path.exists(): | ||
| print(f"\n🎵 Testing with {audio_file}") | ||
| print("-" * 30) | ||
|
|
||
| try: | ||
| # Transcribe audio | ||
| result = transcriber.transcribe_audio( | ||
| str(audio_path), | ||
| language="auto", | ||
| return_timestamps=True | ||
| ) | ||
|
|
||
| print(f"📝 Transcription: {result['text']}") | ||
| print(f"🌍 Language: {result['language']}") | ||
| print(f"⏱️ Processing time: {result['processing_time']:.2f}s") | ||
| print(f"🎯 Confidence: {result['confidence']:.3f}") | ||
| print(f"⏰ Audio duration: {result['audio_duration']:.2f}s") | ||
|
|
||
| if result['timestamps']: | ||
| print(f"📍 Timestamps: {len(result['timestamps'])} segments") | ||
|
|
||
| except Exception as e: | ||
| print(f"❌ Error transcribing {audio_file}: {e}") | ||
|
|
||
| else: | ||
| print(f"⚠️ Sample file not found: {audio_file}") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if "error" in result: | ||
| print(f"❌ File {i+1}: {result['error']}") | ||
| else: | ||
| print(f"✅ File {i+1}: {result['text'][:50]}...") |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
|
|
||
| def _configure_logging(level_str: str) -> None: | ||
| """Configure logging level from config.""" | ||
| level = getattr(logging, str(level_str).upper(), logging.INFO) |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary casts to int, str, float or bool (remove-unnecessary-cast)
| level = getattr(logging, str(level_str).upper(), logging.INFO) | |
| level = getattr(logging, level_str.upper(), logging.INFO) |
| start_time = time.time() | ||
|
|
||
| try: | ||
| # Load and preprocess audio |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Move setting of default value for variable into
elsebranch (introduce-default-else) - Extract code out into method (
extract-method) - Replace if statement with if expression (
assign-if-exp)
| available_files = [ | ||
| str(REPO_ROOT / f) for f in sample_files | ||
| if (REPO_ROOT / f).exists() | ||
| ] |
There was a problem hiding this comment.
issue (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)
| config_path = REPO_ROOT / "configs" / "whisper_config.yaml" | ||
| transcriber = create_samo_whisper_transcriber(str(config_path)) | ||
|
|
||
| print(f"✅ Configuration loaded successfully") | ||
| print(f"📱 Device: {transcriber.device}") | ||
| print(f"🤖 Model: {transcriber.config['model']['name']}") | ||
| print(f"🎵 Sample rate: {transcriber.config['audio']['sample_rate']}") |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract code out into function (
extract-method) - Replace f-string with no interpolated values with string (
remove-redundant-fstring)
There was a problem hiding this comment.
Code Review
This pull request introduces a Whisper transcription model. The implementation is a good starting point, but it has several critical issues. Key features mentioned in the description, such as word-level timestamps, confidence scores, and long-audio chunking, are not implemented and are currently just placeholders. The batch transcription method is also implemented as a sequential loop, which is inefficient. Additionally, there are bugs in the configuration loading and audio preprocessing that could lead to incorrect behavior or runtime errors. I've left detailed comments on these points with suggestions for fixes. The standalone test script also uses an anti-pattern for imports that should be addressed for better maintainability.
| def transcribe_audio( | ||
| self, | ||
| audio_path: Union[str, Path], | ||
| language: Optional[str] = None, | ||
| return_timestamps: bool = True | ||
| ) -> Dict[str, Any]: | ||
| """Transcribe audio file to text. | ||
|
|
||
| Args: | ||
| audio_path: Path to audio file | ||
| language: Language code (e.g., 'en', 'de', 'fr') or None for auto-detect | ||
| return_timestamps: Whether to return word-level timestamps | ||
|
|
||
| Returns: | ||
| Dictionary containing transcription results | ||
| """ | ||
| start_time = time.time() | ||
|
|
||
| try: | ||
| # Load and preprocess audio | ||
| audio_array, sample_rate = self._load_audio(audio_path) | ||
| audio_array = self._preprocess_audio(audio_array, sample_rate) | ||
|
|
||
| # Prepare inputs | ||
| inputs = self.processor( | ||
| audio_array, | ||
| sampling_rate=self.config["audio"]["sample_rate"], | ||
| return_tensors="pt" | ||
| ) | ||
|
|
||
| # Move to device and ensure dtype consistency | ||
| for key in inputs: | ||
| inputs[key] = inputs[key].to(self.device) | ||
| if hasattr(self.model, 'dtype'): | ||
| inputs[key] = inputs[key].to(self.model.dtype) | ||
|
|
||
| # Transcribe | ||
| with torch.inference_mode(): | ||
| # Handle language parameter - don't pass "auto" to Whisper | ||
| whisper_language = language or self.config["transcription"]["language"] | ||
| if whisper_language == "auto": | ||
| whisper_language = None # Let Whisper auto-detect | ||
|
|
||
| generated_ids = self.model.generate( | ||
| inputs["input_features"], | ||
| language=whisper_language, | ||
| task=self.config["transcription"]["task"], | ||
| return_timestamps=return_timestamps | ||
| ) | ||
|
|
||
| # Decode results | ||
| transcription = self.processor.batch_decode( | ||
| generated_ids, skip_special_tokens=True | ||
| )[0] | ||
|
|
||
| # Extract timestamps if requested | ||
| timestamps = None | ||
| if return_timestamps: | ||
| timestamps = self._extract_timestamps(generated_ids) | ||
|
|
||
| processing_time = time.time() - start_time | ||
|
|
||
| return { | ||
| "text": transcription.strip(), | ||
| "timestamps": timestamps, | ||
| "language": language or "auto", | ||
| "processing_time": processing_time, | ||
| "audio_duration": len(audio_array) / self.config["audio"]["sample_rate"], | ||
| "confidence": self._calculate_confidence(generated_ids) | ||
| } | ||
|
|
||
| except Exception as e: | ||
| logger.exception("Transcription failed for %s", audio_path) | ||
| raise RuntimeError(f"Transcription failed: {e}") from e | ||
|
|
There was a problem hiding this comment.
The transcribe_audio method processes the entire audio file at once. It does not implement chunking for long audio files, which is a critical feature for a robust transcription system as Whisper models are trained on 30-second audio clips. The configuration file (whisper_config.yaml) and PR description both mention chunking (enable_chunking, chunk_length_s), but this logic is missing from the implementation. Without it, transcribing files longer than 30 seconds will likely yield poor results or fail. You need to implement a sliding window mechanism to process long audio files in chunks. The transformers library has pipelines that handle this automatically, which could be a good reference.
| def _extract_timestamps(self, generated_ids: torch.Tensor) -> List[Dict[str, Any]]: | ||
| """Extract word-level timestamps from generated IDs.""" | ||
| # This is a simplified implementation | ||
| # In practice, you'd need to decode the timestamp tokens | ||
| return [] |
There was a problem hiding this comment.
This function is a placeholder and returns an empty list, but the feature "Include word-level timestamps" is advertised in the pull request description and enabled by default in the configuration. This is misleading as the feature is not implemented. To implement this, you'll need to correctly configure the model.generate() call (e.g., with return_word_timestamps=True) and then process the output to extract the timestamp tokens and their corresponding times.
| def _calculate_confidence(self, generated_ids: torch.Tensor) -> float: | ||
| """Calculate confidence score for transcription.""" | ||
| # Simplified confidence calculation | ||
| # In practice, you'd use the model's logits | ||
| return 0.85 |
There was a problem hiding this comment.
This function returns a hardcoded value of 0.85, which is misleading. The PR description claims a confidence score is calculated, but this is just a placeholder. To properly calculate confidence, you should pass output_scores=True and return_dict_in_generate=True to model.generate(). You can then compute a confidence score from the returned logits/scores, for example by calculating the average log probability of the generated tokens.
| def _deep_merge(base: Dict[str, Any], override: Dict[str, Any]) -> Dict[str, Any]: | ||
| """Recursively merge two dictionaries, with values from override taking precedence.""" | ||
| for key, value in override.items(): | ||
| if (key in base and | ||
| isinstance(base[key], dict) and | ||
| isinstance(value, dict)): | ||
| base[key].update(value) | ||
| else: | ||
| base[key] = value | ||
| return base |
There was a problem hiding this comment.
The _deep_merge function is not performing a deep merge as its docstring suggests. The line base[key].update(value) only performs a shallow merge of nested dictionaries. This can lead to loss of configuration keys from the default config if the user provides a partial nested dictionary in their custom config. For a true recursive merge, you should call _deep_merge on the nested dictionaries.
| def _deep_merge(base: Dict[str, Any], override: Dict[str, Any]) -> Dict[str, Any]: | |
| """Recursively merge two dictionaries, with values from override taking precedence.""" | |
| for key, value in override.items(): | |
| if (key in base and | |
| isinstance(base[key], dict) and | |
| isinstance(value, dict)): | |
| base[key].update(value) | |
| else: | |
| base[key] = value | |
| return base | |
| def _deep_merge(base: Dict[str, Any], override: Dict[str, Any]) -> Dict[str, Any]: | |
| """Recursively merge two dictionaries, with values from override taking precedence.""" | |
| for key, value in override.items(): | |
| if (key in base and | |
| isinstance(base[key], dict) and | |
| isinstance(value, dict)): | |
| _deep_merge(base[key], value) | |
| else: | |
| base[key] = value | |
| return base |
| def transcribe_batch( | ||
| self, | ||
| audio_paths: List[Union[str, Path]], | ||
| language: Optional[str] = None, | ||
| return_timestamps: bool = True | ||
| ) -> List[Dict[str, Any]]: | ||
| """Transcribe multiple audio files in batch. | ||
|
|
||
| Args: | ||
| audio_paths: List of paths to audio files | ||
| language: Language code or None for auto-detect | ||
| return_timestamps: Whether to return word-level timestamps | ||
|
|
||
| Returns: | ||
| List of transcription results | ||
| """ | ||
| start_time = time.time() | ||
| results = [] | ||
|
|
||
| for i, audio_path in enumerate(audio_paths): | ||
| try: | ||
| logger.info("Transcribing file %d/%d: %s", i + 1, len(audio_paths), audio_path) | ||
| result = self.transcribe_audio(audio_path, language, return_timestamps) | ||
| results.append(result) | ||
| except Exception as e: | ||
| logger.error("Failed to transcribe %s: %s", audio_path, e) | ||
| results.append({ | ||
| "text": "", | ||
| "error": str(e), | ||
| "audio_path": str(audio_path) | ||
| }) | ||
|
|
||
| total_time = time.time() - start_time | ||
| logger.info("Batch transcription completed in %.2f seconds", total_time) | ||
|
|
||
| return results |
There was a problem hiding this comment.
The transcribe_batch method is not a true batch implementation. It iterates through the list of audio paths and processes them one by one sequentially. This is inefficient and the method name is misleading. True batch processing would involve padding the audio inputs to the same length and feeding them to the model in a single call, which is significantly faster. Given the current implementation, a more accurate name would be transcribe_files_sequentially or similar. As it stands, this implementation does not deliver the performance benefits expected from batching.
| audio_array = torch.from_numpy(audio_array).float() | ||
|
|
||
| # Normalize audio | ||
| audio_array = audio_array / torch.max(torch.abs(audio_array)) |
There was a problem hiding this comment.
The audio normalization audio_array = audio_array / torch.max(torch.abs(audio_array)) can lead to a division-by-zero error (resulting in NaN values) if the input audio is completely silent (torch.max(...) is 0). This would cause the transcription to fail. You should add a check to prevent this.
max_abs_val = torch.max(torch.abs(audio_array))
if max_abs_val > 0:
audio_array = audio_array / max_abs_val| if self.device != "auto": | ||
| self.model = self.model.to(self.device) |
There was a problem hiding this comment.
The check if self.device != "auto": is redundant. The _get_device function is designed to resolve "auto" to a specific device ("cuda", "mps", or "cpu"), so it will never return "auto". This makes the if condition always true. You can simplify the code by removing the conditional check for better clarity.
self.model = self.model.to(self.device)| REPO_ROOT = Path(__file__).parent.resolve() | ||
| sys.path.insert(0, str(REPO_ROOT / "src")) |
There was a problem hiding this comment.
Modifying sys.path manually is generally considered an anti-pattern in Python. It can make the project structure brittle and lead to import issues, especially as the project grows. A more robust and standard approach is to make your src directory an installable package. You can do this by running pip install -e . from the project root. This will make your package available throughout your environment without needing to modify sys.path in scripts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (21)
dependencies/requirements-audio.txt (2)
13-16: Avoid dual Whisper stacks; removeopenai-whisperif unused.Code uses Hugging Face
transformersWhisper, notopenai-whisper. Keeping both increases install size and can confuse users. Dropopenai-whisperunless explicitly required elsewhere.Apply outside this hunk:
- openai-whisper>=20231117
13-16: Clarify CPU/MPS support expectations for torch version.MPS/CPU paths are sensitive to dtype support. Ensure torch version aligns with your target Apple silicon and CPU ops. Consider documenting minimum torch (e.g., 2.1+) if you rely on MPS half precision for speed.
configs/whisper_config.yaml (2)
36-43: WMA support is questionable with librosa/audioread.librosa often can’t load
.wmareliably. Either remove it or gate-load with clearer errors.YAML change (optional):
- - ".wma"
9-14: Duplicate chunk/stride settings (seconds vs samples) invite drift.There are two sets:
audio.chunk_length/stride_lengthandtranscription.chunk_length_s/stride_length_s. Consolidate to one.Also applies to: 20-21
src/models/transcription/whisper_transcriber.py (8)
142-145: Redundant device check.
self.deviceis never "auto" after_get_device, so theif self.device != "auto":guard can be dropped.
175-187: Tighten tensor casting.Only
input_featuresexists; cast directly and avoid looping keys. Minor clarity/perf win.- for key in inputs: - inputs[key] = inputs[key].to(self.device) - if hasattr(self.model, 'dtype'): - inputs[key] = inputs[key].to(self.model.dtype) + inputs["input_features"] = inputs["input_features"].to(self.device) + if hasattr(self.model, "dtype"): + inputs["input_features"] = inputs["input_features"].to(self.model.dtype)
214-221: Report detected language when auto-detecting.Currently returns
"auto". If you keep auto-detect, expose the resolved language (when forced ids are used) or omit the field to avoid confusion.
251-252: Uselogger.exceptionto preserve traceback.Keeps stack traces for failed files.
- logger.error("Failed to transcribe %s: %s", audio_path, e) + logger.exception("Failed to transcribe %s: %s", audio_path, e)
270-276: Optional: validate extension before load for clearer UX.Check against
supported_formatsand error early with a friendly message.
18-18: Unused import.
soundfile as sfis not used; remove to keep imports clean.-import soundfile as sf
54-66: Catching broadExceptioneverywhere hides actionable errors.Narrow where feasible (I/O vs model load vs decode) and attach context within custom exceptions.
Also applies to: 148-151, 223-226
298-309: Stubs: unused args and fixed confidence.Mark as
@staticmethodwithout params or implement real logic usingreturn_dict_in_generate=Trueand scores.Example direction:
- def _calculate_confidence(self, generated_ids: torch.Tensor) -> float: + def _calculate_confidence(self, generated_ids: torch.Tensor) -> float: """Calculate confidence score for transcription.""" - # Simplified confidence calculation - # In practice, you'd use the model's logits + # TODO: compute from model scores via generate(..., output_scores=True, return_dict_in_generate=True) return 0.85test_whisper_standalone.py (9)
10-10: Remove unused import.
textwrapisn't used; drop it to satisfy linters and keep imports lean.-import textwrap
67-67: Drop redundant f-strings without placeholders.These trigger Ruff F541 and add no value.
- print(f"\n🔄 Testing batch transcription") + print("\n🔄 Testing batch transcription") @@ - print(f"\n🎉 Whisper transcriber test completed successfully!") + print("\n🎉 Whisper transcriber test completed successfully!") @@ - print(f"✅ Configuration loaded successfully") + print("✅ Configuration loaded successfully")Also applies to: 89-89, 107-107
60-62: Narrow exception scope around per-file transcription.Catching
Exceptionhides real failures and violates BLE001. Catch the expectedRuntimeErrorraised by the transcriber; let unexpected exceptions bubble.- except Exception as e: - print(f"❌ Error transcribing {audio_file}: {e}") + except RuntimeError as e: + print(f"❌ Error transcribing {audio_file}: {e}")
91-93: Avoid blind catch in test_whisper_transcriber().Limit to anticipated errors (config or model init/runtime). This keeps failure modes visible while staying informative.
- except Exception as e: + except (FileNotFoundError, ValueError, RuntimeError) as e: print(f"❌ Test failed: {e}") return False
112-114: Avoid blind catch in test_configuration().Same rationale—BLE001 and clearer failure reporting.
- except Exception as e: + except (FileNotFoundError, ValueError, RuntimeError) as e: print(f"❌ Configuration test failed: {e}") return False
51-56: Clarify that printed language is the requested mode, not detected.
transcribe_audio()returns"language": language or "auto", so this prints “auto” rather than the detected language. Either adjust the label here, or extend the transcriber to return adetected_languagefield.- print(f"🌍 Language: {result['language']}") + print(f"🌍 Language (requested): {result['language']}") + if 'detected_language' in result and result['detected_language']: + print(f"🧭 Detected language: {result['detected_language']}")If you want, I can patch
SAMOWhisperTranscriber.transcribe_audio()to surfacedetected_languageextracted from the generated tokens.
27-29: Avoid loading the Whisper model twice; reuse a single transcriber instance.Model init is heavy; you create it in both tests. Cache/reuse to cut test time significantly.
- transcriber = create_samo_whisper_transcriber(str(config_path)) + transcriber = get_transcriber() @@ - transcriber = create_samo_whisper_transcriber(str(config_path)) + transcriber = get_transcriber()Add once near the imports (outside the shown ranges):
from functools import lru_cache @lru_cache(maxsize=1) def get_transcriber(): cfg = REPO_ROOT / "configs" / "whisper_config.yaml" return create_samo_whisper_transcriber(str(cfg))Also applies to: 103-111
12-16: Harden sys.path injection.Guard against duplicate inserts and missing src dir. Optional but cleaner in varied environments.
-# Add src to path -REPO_ROOT = Path(__file__).parent.resolve() -sys.path.insert(0, str(REPO_ROOT / "src")) +# Add src to path +REPO_ROOT = Path(__file__).parent.resolve() +SRC_DIR = REPO_ROOT / "src" +if SRC_DIR.is_dir(): + p = str(SRC_DIR) + if p not in sys.path: + sys.path.insert(0, p) +else: + print(f"⚠️ src directory not found at {SRC_DIR}")
70-76: Make “no sample files” explicit (don’t silently “pass”).As written, zero available files yields a “completed successfully” message. Consider marking as skipped to avoid false confidence.
- if available_files: + if available_files: batch_results = transcriber.transcribe_batch( available_files, language="auto", return_timestamps=True ) @@ - for i, result in enumerate(batch_results): + for i, result in enumerate(batch_results): if "error" in result: print(f"❌ File {i+1}: {result['error']}") else: print(f"✅ File {i+1}: {result['text'][:50]}...") + else: + print("⚠️ No sample files found. Skipping batch transcription.")Also applies to: 82-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/models/transcription/__pycache__/whisper_transcriber.cpython-38.pycis excluded by!**/*.pyc
📒 Files selected for processing (4)
configs/whisper_config.yaml(1 hunks)dependencies/requirements-audio.txt(1 hunks)src/models/transcription/whisper_transcriber.py(1 hunks)test_whisper_standalone.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test_whisper_standalone.py (1)
src/models/transcription/whisper_transcriber.py (3)
create_samo_whisper_transcriber(311-320)transcribe_audio(152-225)transcribe_batch(227-262)
🪛 Ruff (0.12.2)
test_whisper_standalone.py
60-60: Do not catch blind exception: Exception
(BLE001)
67-67: f-string without any placeholders
Remove extraneous f prefix
(F541)
89-89: f-string without any placeholders
Remove extraneous f prefix
(F541)
91-91: Do not catch blind exception: Exception
(BLE001)
107-107: f-string without any placeholders
Remove extraneous f prefix
(F541)
112-112: Do not catch blind exception: Exception
(BLE001)
src/models/transcription/whisper_transcriber.py
60-60: Do not catch blind exception: Exception
(BLE001)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Do not catch blind exception: Exception
(BLE001)
252-252: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
298-298: Unused method argument: generated_ids
(ARG002)
304-304: Unused method argument: generated_ids
(ARG002)
🔇 Additional comments (2)
src/models/transcription/whisper_transcriber.py (1)
189-201: Avoid passinglanguage=None; useforced_decoder_idsor omit kw.Build kwargs conditionally; for explicit language/task, set
forced_decoder_idsfrom the processor for stable behavior.- whisper_language = language or self.config["transcription"]["language"] - if whisper_language == "auto": - whisper_language = None # Let Whisper auto-detect - - generated_ids = self.model.generate( - inputs["input_features"], - language=whisper_language, - task=self.config["transcription"]["task"], - return_timestamps=return_timestamps - ) + whisper_language = language or self.config["transcription"]["language"] + task = self.config["transcription"]["task"] + gen_kwargs = {"return_timestamps": return_timestamps} + if whisper_language and whisper_language != "auto": + gen_kwargs["forced_decoder_ids"] = self.processor.get_decoder_prompt_ids( + language=whisper_language, task=task + ) + generated_ids = self.model.generate(inputs["input_features"], **gen_kwargs)Likely an incorrect or invalid review comment.
test_whisper_standalone.py (1)
38-66: Pathing assumption for sample assets—please confirm.The script looks for sample WAVs at repo root. If assets live under a folder (e.g., assets/ or samples/), update paths accordingly or make the location configurable.
| name: "openai/whisper-base" # Whisper model variant | ||
| device: "auto" # auto, cpu, cuda, mps | ||
| torch_dtype: "float16" # float16 for efficiency, float32 for precision |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Default torch_dtype: float16 will likely fail on CPU.
On CPU, many ops don't support fp16. Use an auto setting and let code pick fp32 on CPU, fp16 on CUDA/MPS.
YAML change:
- torch_dtype: "float16" # float16 for efficiency, float32 for precision
+ torch_dtype: "auto" # auto-select: fp32 on CPU, fp16 on CUDA/MPSAnd update loader to map "auto" to a safe dtype (see code suggestion in transcriber).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: "openai/whisper-base" # Whisper model variant | |
| device: "auto" # auto, cpu, cuda, mps | |
| torch_dtype: "float16" # float16 for efficiency, float32 for precision | |
| name: "openai/whisper-base" # Whisper model variant | |
| device: "auto" # auto, cpu, cuda, mps | |
| torch_dtype: "auto" # auto-select: fp32 on CPU, fp16 on CUDA/MPS |
🤖 Prompt for AI Agents
In configs/whisper_config.yaml around lines 5 to 7, the default torch_dtype is
set to "float16" which will likely fail on CPU; change the YAML value to "auto"
and update the model loader to interpret "auto" by detecting device: map to
torch.float32 for CPU and torch.bfloat16/torch.float16 (whichever your runtime
supports) for CUDA/MPS as appropriate, ensuring the loader performs device
detection and chooses a safe dtype before model instantiation.
| transcription: | ||
| language: "auto" # Language code or "auto" for detection | ||
| task: "transcribe" # "transcribe" or "translate" | ||
| return_timestamps: true # Include word-level timestamps | ||
| return_language: true # Include detected language | ||
| chunk_length_s: 30.0 # Chunk length in seconds | ||
| stride_length_s: 5.0 # Stride length in seconds |
There was a problem hiding this comment.
Config claims “word-level timestamps” but code returns an empty list.
Either implement extraction or set the default to false to avoid misleading behavior.
YAML change:
- return_timestamps: true # Include word-level timestamps
+ return_timestamps: false # Enable once timestamps are implemented📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transcription: | |
| language: "auto" # Language code or "auto" for detection | |
| task: "transcribe" # "transcribe" or "translate" | |
| return_timestamps: true # Include word-level timestamps | |
| return_language: true # Include detected language | |
| chunk_length_s: 30.0 # Chunk length in seconds | |
| stride_length_s: 5.0 # Stride length in seconds | |
| transcription: | |
| language: "auto" # Language code or "auto" for detection | |
| task: "transcribe" # "transcribe" or "translate" | |
| return_timestamps: false # Enable once timestamps are implemented | |
| return_language: true # Include detected language | |
| chunk_length_s: 30.0 # Chunk length in seconds | |
| stride_length_s: 5.0 # Stride length in seconds |
🤖 Prompt for AI Agents
In configs/whisper_config.yaml around lines 15 to 21, the config field
return_timestamps is documented as "Include word-level timestamps" but the code
currently returns an empty list; update the config to avoid misleading users by
changing return_timestamps to false as the default, update the inline comment to
"Include word-level timestamps (requires implementation)" and add a TODO note
referencing the code path that must be implemented to provide word-level
timestamps, or alternatively implement the extraction logic in the transcription
pipeline so that return_timestamps=true actually populates word-level
timestamps.
| torch_dtype = getattr(torch, self.config["model"]["torch_dtype"]) | ||
| self.model = WhisperForConditionalGeneration.from_pretrained( | ||
| model_name, | ||
| torch_dtype=torch_dtype | ||
| ) | ||
|
|
||
| # Move to device after loading | ||
| if self.device != "auto": | ||
| self.model = self.model.to(self.device) | ||
|
|
||
| logger.info("Whisper model loaded successfully on %s", self.device) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix dtype selection and set eval mode; fp16 on CPU will break.
Select dtype based on device, enable low-memory load, move to device, and set eval().
- # Load model with appropriate dtype
- torch_dtype = getattr(torch, self.config["model"]["torch_dtype"])
- self.model = WhisperForConditionalGeneration.from_pretrained(
- model_name,
- torch_dtype=torch_dtype
- )
-
- # Move to device after loading
- if self.device != "auto":
- self.model = self.model.to(self.device)
+ # Resolve dtype by device
+ dtype_cfg = str(self.config["model"].get("torch_dtype", "auto")).lower()
+ if dtype_cfg == "auto":
+ torch_dtype = torch.float32 if self.device == "cpu" else torch.float16
+ else:
+ torch_dtype = getattr(torch, dtype_cfg, torch.float32)
+
+ self.model = WhisperForConditionalGeneration.from_pretrained(
+ model_name,
+ torch_dtype=torch_dtype,
+ low_cpu_mem_usage=True,
+ ).to(self.device)
+ self.model.eval()Also safe to add device_map="auto" if you plan to leverage Accelerate.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/models/transcription/whisper_transcriber.py around lines 136 to 147, the
current dtype is chosen unconditionally and fp16 on CPU will break; change the
logic to pick torch.float16 only when the target device is a CUDA/metal MPS GPU
and fallback to torch.float32 for CPU, pass low_cpu_mem_usage=True (or
equivalent low_memory flag) to from_pretrained to reduce memory during load,
optionally include device_map="auto" if using Accelerate, move the model to the
chosen device after loading, and call self.model.eval() to set evaluation mode.
| self, | ||
| audio_path: Union[str, Path], | ||
| language: Optional[str] = None, | ||
| return_timestamps: bool = True | ||
| ) -> Dict[str, Any]: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Default return_timestamps=True is misleading given stub.
Flip the default to false to match current behavior.
- return_timestamps: bool = True
+ return_timestamps: bool = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self, | |
| audio_path: Union[str, Path], | |
| language: Optional[str] = None, | |
| return_timestamps: bool = True | |
| ) -> Dict[str, Any]: | |
| def transcribe( | |
| self, | |
| audio_path: Union[str, Path], | |
| language: Optional[str] = None, | |
| return_timestamps: bool = False | |
| ) -> Dict[str, Any]: |
🤖 Prompt for AI Agents
In src/models/transcription/whisper_transcriber.py around lines 153 to 157, the
method signature sets return_timestamps=True which is misleading given the
current stub/behavior; change the default to return_timestamps: bool = False in
the function signature so the default matches current behavior, and update any
related docstring or inline comment to reflect the new default.
| transcription = self.processor.batch_decode( | ||
| generated_ids, skip_special_tokens=True | ||
| )[0] | ||
|
|
||
| # Extract timestamps if requested | ||
| timestamps = None | ||
| if return_timestamps: | ||
| timestamps = self._extract_timestamps(generated_ids) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Timestamps not implemented; decoding drops them.
batch_decode(..., skip_special_tokens=True) removes timestamp tokens; _extract_timestamps returns []. Either implement parsing of timestamp tokens or default to no timestamps.
Minimal safe adjustment:
- transcription = self.processor.batch_decode(
- generated_ids, skip_special_tokens=True
- )[0]
+ transcription = self.processor.batch_decode(
+ generated_ids, skip_special_tokens=True
+ )[0]
# Extract timestamps if requested
- timestamps = None
- if return_timestamps:
- timestamps = self._extract_timestamps(generated_ids)
+ timestamps = None # TODO: implement timestamp extractionAnd consider defaulting return_timestamps=False in both code and config until implemented (see separate comments).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transcription = self.processor.batch_decode( | |
| generated_ids, skip_special_tokens=True | |
| )[0] | |
| # Extract timestamps if requested | |
| timestamps = None | |
| if return_timestamps: | |
| timestamps = self._extract_timestamps(generated_ids) | |
| transcription = self.processor.batch_decode( | |
| generated_ids, skip_special_tokens=True | |
| )[0] | |
| # Extract timestamps if requested | |
| timestamps = None # TODO: implement timestamp extraction |
🤖 Prompt for AI Agents
In src/models/transcription/whisper_transcriber.py around lines 203 to 211, the
current call to self.processor.batch_decode(..., skip_special_tokens=True)
strips out timestamp tokens so timestamps cannot be extracted (and
_extract_timestamps returns an empty list); either stop skipping special tokens
and parse timestamp tokens from the decoded output or directly parse timestamp
token IDs from generated_ids before decoding, returning structured timestamps;
as a minimal safe fix change the logic so when return_timestamps is True you
decode without skip_special_tokens and implement parsing of the timestamp token
format into timestamp pairs, and also default return_timestamps to False in the
function signature and config until full timestamp parsing is implemented.
| # Resample if necessary using librosa | ||
| if sample_rate != target_sample_rate: | ||
| audio_array = librosa.resample( | ||
| audio_array.numpy(), | ||
| orig_sr=sample_rate, | ||
| target_sr=target_sample_rate | ||
| ) | ||
| audio_array = torch.from_numpy(audio_array).float() | ||
|
|
||
| # Normalize audio | ||
| audio_array = audio_array / torch.max(torch.abs(audio_array)) | ||
|
|
||
| return audio_array |
There was a problem hiding this comment.
Guard normalization against divide-by-zero.
Silence-only inputs will cause NaNs.
- audio_array = audio_array / torch.max(torch.abs(audio_array))
+ denom = torch.max(torch.abs(audio_array))
+ if denom > 0:
+ audio_array = audio_array / denom📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Resample if necessary using librosa | |
| if sample_rate != target_sample_rate: | |
| audio_array = librosa.resample( | |
| audio_array.numpy(), | |
| orig_sr=sample_rate, | |
| target_sr=target_sample_rate | |
| ) | |
| audio_array = torch.from_numpy(audio_array).float() | |
| # Normalize audio | |
| audio_array = audio_array / torch.max(torch.abs(audio_array)) | |
| return audio_array | |
| # Resample if necessary using librosa | |
| if sample_rate != target_sample_rate: | |
| audio_array = librosa.resample( | |
| audio_array.numpy(), | |
| orig_sr=sample_rate, | |
| target_sr=target_sample_rate | |
| ) | |
| audio_array = torch.from_numpy(audio_array).float() | |
| # Normalize audio | |
| denom = torch.max(torch.abs(audio_array)) | |
| if denom > 0: | |
| audio_array = audio_array / denom | |
| return audio_array |
🤖 Prompt for AI Agents
In src/models/transcription/whisper_transcriber.py around lines 284 to 296, the
normalization step can divide by zero for silence-only inputs causing NaNs;
guard the normalization by computing the maximum absolute value (e.g., max_abs =
torch.max(torch.abs(audio_array))) and only divide when max_abs > 0, otherwise
return the original (or zero) audio_array unchanged; ensure you handle tensors
on the correct device/dtype and avoid in-place ops that could change gradients
unexpectedly.
🎤 Add SAMO-Optimized Whisper Audio Transcription Model
PR-2 from Surgical Breakdown Plan - PR #147
📋 SCOPE DECLARATION
ALLOWED: Whisper transcription model implementation only
FORBIDDEN: API endpoints, Docker changes, testing infrastructure, other models
FILES TOUCHED: 4 files (under 25 limit)
TIME ESTIMATE: 2 hours
🎯 What This PR Does
Adds a complete SAMO-optimized Whisper audio transcription model with:
📁 Files Added/Modified
src/models/transcription/whisper_transcriber.py- Core Whisper model implementationconfigs/whisper_config.yaml- Configuration settingstest_whisper_standalone.py- Standalone testing scriptdependencies/requirements-audio.txt- Updated with Whisper dependencies🚀 Performance Metrics
✅ Testing Results
🔧 Technical Features
📊 Compliance Check
🔗 Related
Ready for review and merge! 🚀
Summary by Sourcery
Add a SAMO-optimized Whisper transcription module with support for multi-format audio, configurable settings, auto device selection, batch processing, and standalone tests
New Features:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Tests
Chores