feat: add API dependencies and configuration#152
Conversation
- Enhanced BERT-based emotion classifier for journal entries - Multi-label emotion classification (28 emotions from GoEmotions) - Temperature scaling for calibrated predictions - Comprehensive emotion labels and descriptions - Standalone test script for validation - Configuration file with SAMO-specific optimizations - Error handling and logging improvements Files: - src/models/emotion_detection/samo_bert_emotion_classifier.py - src/models/emotion_detection/emotion_labels.py - configs/samo_emotion_detection_config.yaml - test_samo_emotion_detection_standalone.py This completes PR-3 of the surgical breakdown plan.
- Fixed NameError for classifier_dropout_prob and freeze_bert_layers - Updated constructor to use self.classifier_dropout_prob and self.freeze_bert_layers - Model now initializes correctly and passes standalone tests - Maintains 110M parameters with 66M frozen BERT layers Part of PR-3: Emotion Detection model completion
- Created FastAPI-based unified server integrating T5, Whisper, and BERT models - Individual endpoints: /summarize, /transcribe, /detect-emotions - Combined pipeline endpoint: /process-audio (transcription -> summary -> emotions) - Comprehensive error handling and validation - Health monitoring endpoint - CORS support for web applications - Request/response models with Pydantic validation - Background task cleanup for uploaded files Part of PR-4: Unified API Server implementation
- Added API configuration file (samo_api_config.yaml) with model settings - Created comprehensive test suite (test_unified_api_server.py) with mocked tests - Added startup script (start_api_server.py) with CLI arguments - Updated API requirements file with all necessary dependencies - Includes health checks, validation, error handling, and combined pipeline tests Part of PR-4: Unified API Server implementation
- Fixed emotion detection model loading (tuple unpacking issue) - Successfully tested all endpoints: /summarize, /transcribe, /detect-emotions, /process-audio - Combined pipeline working: transcription -> summarization -> emotion detection - All models loading correctly with proper error handling - API server running on http://localhost:8000 with full documentation Part of PR-4: Unified API Server - all endpoints functional and tested
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughAdds a unified API stack: new FastAPI server with multi-model endpoints, a Flask-based alternative server, configuration YAMLs for API and emotion detection, start script, emotion labels/utilities, BERT emotion classifier, dependency manifests, and comprehensive tests (including mocked end-to-end flows). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as FastAPI Server
participant Whisper as Transcriber
participant T5 as Summarizer
participant Emo as EmotionDetector
rect rgba(230,245,255,0.6)
note right of API: Health
Client->>API: GET /health
API-->>Client: status, models_loaded, memory
end
rect rgba(240,255,240,0.6)
note right of API: Summarize
Client->>API: POST /summarize {text,...}
API->>T5: generate(text, params)
T5-->>API: summary, meta
API-->>Client: summary response
end
rect rgba(255,245,230,0.6)
note right of API: Transcribe
Client->>API: POST /transcribe (audio)
API->>Whisper: transcribe(file, opts)
Whisper-->>API: text, lang, metrics
API-->>Client: transcription response
end
rect rgba(255,240,250,0.6)
note right of API: Detect Emotions
Client->>API: POST /detect-emotions {text, threshold, top_k}
API->>Emo: predict(text, threshold, top_k)
Emo-->>API: emotions, probs, preds
API-->>Client: emotion response
end
rect rgba(245,245,255,0.6)
note right of API: Combined Pipeline
Client->>API: POST /process-audio (file, params)
API->>Whisper: transcribe(file)
Whisper-->>API: transcript
API->>T5: summarize(transcript)
T5-->>API: summary
API->>Emo: predict(summary or transcript)
Emo-->>API: emotions
API-->>Client: transcription + summary + emotions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 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 |
|
Here's the code health analysis summary for commits Analysis Summary
|
![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 lays the groundwork for the SAMO-DL platform's API capabilities by introducing a robust, unified API server. It integrates and exposes various deep learning models, notably a new BERT-based emotion detection system, through well-defined endpoints. The changes include essential configuration, updated dependencies, and initial testing to ensure the stability and functionality of these new API services.
Highlights
- New Unified API Server: Introduction of a new FastAPI-based unified API server (
src/models/unified_api_server.py) designed to expose various deep learning models, including summarization, transcription, and emotion detection. - Comprehensive API Configuration: Addition of
configs/samo_api_config.yamlto centralize API settings, security configurations (rate limiting, CORS), and parameters for integrated machine learning models (T5, Whisper, BERT). - Advanced Emotion Detection Model: Implementation of a new BERT-based emotion classifier (
src/models/emotion_detection/samo_bert_emotion_classifier.py) optimized for journal entries, complete with detailed configuration (configs/samo_emotion_detection_config.yaml) and a comprehensive set of 28 GoEmotions labels (src/models/emotion_detection/emotion_labels.py). - Updated API Dependencies: Significant modification of
dependencies/requirements-api.txtto align with the new FastAPI server and its machine learning/NLP model requirements, includingtorch,transformers,openai-whisper, andscikit-learn. - Integrated API Endpoints and Pipelines: New API endpoints for individual model functionalities (summarization, transcription, emotion detection) and a combined audio processing pipeline (
/process-audio) that chains transcription, summarization, and emotion analysis. - Enhanced Testing Framework: Introduction of comprehensive test suites, including a standalone test for the emotion detection model (
test_samo_emotion_detection_standalone.py) and a dedicated pytest suite for the new unified FastAPI server (tests/test_unified_api_server.py).
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. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR implements API dependencies and configuration for the SAMO-DL project by adding FastAPI-based unified API server components, emotion detection modules, and necessary configuration files. The changes establish the foundation for serving machine learning models through RESTful APIs.
Key changes include:
- Addition of a unified FastAPI server integrating T5 summarization, Whisper transcription, and BERT emotion detection
- Implementation of BERT-based emotion classification with 28 emotion categories
- Comprehensive test suites for API endpoints and emotion detection functionality
- Configuration files for API server settings and emotion detection parameters
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_unified_api_server.py | Comprehensive test suite for API endpoints with mocked model dependencies |
| test_samo_emotion_detection_standalone.py | Standalone testing script for emotion detection model validation |
| src/unified_api_server.py | Flask-based API server implementation with emotion, transcription, and summarization endpoints |
| src/models/unified_api_server.py | FastAPI-based unified server with comprehensive model integration and error handling |
| src/models/emotion_detection/samo_bert_emotion_classifier.py | BERT-based emotion classifier with multi-label prediction capabilities |
| src/models/emotion_detection/emotion_labels.py | Emotion label definitions and utility functions for 28 emotion categories |
| scripts/start_api_server.py | Server startup script with command-line argument handling |
| requirements-api.txt | Production API dependencies with pinned versions |
| dependencies/requirements-api.txt | Updated API requirements with FastAPI and ML dependencies |
| configs/samo_emotion_detection_config.yaml | Comprehensive emotion detection model configuration |
| configs/samo_api_config.yaml | API server configuration with security and model settings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| logging.basicConfig(level=logging.INFO) | ||
|
|
||
| # Load configuration | ||
| with open('configs/samo_api_config.yaml', 'r') as f: |
There was a problem hiding this comment.
The hardcoded path 'configs/samo_api_config.yaml' may fail if the working directory is different. Consider using Path(file).parent to build relative paths or making the config path configurable.
| "text": text, | ||
| "emotions": result, | ||
| "confidence": max([r['score'] for r in result]), | ||
| "timestamp": "2025-09-10T10:00:00Z", # Replace with actual timestamp |
There was a problem hiding this comment.
Hardcoded timestamps should be replaced with actual timestamp generation using datetime.now().isoformat() or similar to provide accurate response timestamps.
| "text": texts[i], | ||
| "emotions": [results[i]], | ||
| "confidence": max([r['score'] for r in results[i]]), | ||
| "timestamp": "2025-09-10T10:00:00Z", |
There was a problem hiding this comment.
Hardcoded timestamps should be replaced with actual timestamp generation using datetime.now().isoformat() or similar to provide accurate response timestamps.
| "transcription": result['text'], | ||
| "language": result.get('language', 'en'), | ||
| "duration": len(audio_bytes) / 16000, # Approximate duration | ||
| "timestamp": "2025-09-10T10:00:00Z", |
There was a problem hiding this comment.
Hardcoded timestamps should be replaced with actual timestamp generation using datetime.now().isoformat() or similar to provide accurate response timestamps.
| "original_text": text, | ||
| "summary": result[0]['summary_text'], | ||
| "summary_length": len(result[0]['summary_text'].split()), | ||
| "timestamp": "2025-09-10T10:00:00Z", |
There was a problem hiding this comment.
Hardcoded timestamps should be replaced with actual timestamp generation using datetime.now().isoformat() or similar to provide accurate response timestamps.
|
|
||
| try: | ||
| # Save uploaded file temporarily | ||
| temp_path = f"/tmp/{file.filename}" |
There was a problem hiding this comment.
Using user-provided filenames directly in file paths creates a path traversal vulnerability. Consider using secure temporary file creation with tempfile.NamedTemporaryFile() or sanitizing the filename.
| if not self.models["transcriber"]: | ||
| raise HTTPException(status_code=503, detail="Transcription model not available") | ||
|
|
||
| temp_path = f"/tmp/{file.filename}" |
There was a problem hiding this comment.
Using user-provided filenames directly in file paths creates a path traversal vulnerability. Consider using secure temporary file creation with tempfile.NamedTemporaryFile() or sanitizing the filename.
| return { | ||
| "input_ids": encoding["input_ids"].squeeze(0), | ||
| "attention_mask": encoding["attention_mask"].squeeze(0), | ||
| "token_type_ids": encoding.get("token_type_ids", torch.zeros_like(encoding["input_ids"])).squeeze(0), |
There was a problem hiding this comment.
Creating torch.zeros_like(encoding["input_ids"]) when token_type_ids is None creates unnecessary tensor operations. Consider using None or a more efficient default.
| "token_type_ids": encoding.get("token_type_ids", torch.zeros_like(encoding["input_ids"])).squeeze(0), | |
| "token_type_ids": encoding["token_type_ids"].squeeze(0) if "token_type_ids" in encoding else None, |
| Flask==3.0.3 | ||
| Flask-RESTX==1.3.0 |
There was a problem hiding this comment.
The requirements file includes Flask dependencies, but the main unified API server uses FastAPI. This creates confusion about which framework is actually used and may lead to unnecessary dependencies.
| Flask==3.0.3 | |
| Flask-RESTX==1.3.0 |
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for the API, including configurations, dependencies, and server implementations. However, it adds two distinct API server implementations: one using Flask (src/unified_api_server.py) and another using FastAPI (src/models/unified_api_server.py), along with two separate requirements-api.txt files. This creates redundancy and architectural ambiguity. It is highly recommended to consolidate into a single, consistent framework. Additionally, the FastAPI implementation contains a critical path traversal vulnerability and lacks proper configuration loading, while the Flask implementation has issues with hardcoded values. These issues should be addressed before merging.
| temp_path = f"/tmp/{file.filename}" | ||
| with open(temp_path, "wb") as buffer: | ||
| content = await file.read() | ||
| buffer.write(content) |
There was a problem hiding this comment.
Using the user-provided file.filename directly to construct a file path is a security risk and can lead to a path traversal vulnerability. A malicious user could provide a filename like ../../home/user/.bashrc to overwrite arbitrary files on the system. The same issue exists on line 311.
To fix this, you should use a secure method to handle temporary files, for example, by using Python's tempfile module. You will need to add import tempfile.
| temp_path = f"/tmp/{file.filename}" | |
| with open(temp_path, "wb") as buffer: | |
| content = await file.read() | |
| buffer.write(content) | |
| with tempfile.NamedTemporaryFile(delete=False, suffix=Path(file.filename).suffix) as tmp_file: | |
| content = await file.read() | |
| tmp_file.write(content) | |
| temp_path = tmp_file.name |
| fastapi==0.104.1 | ||
| uvicorn[standard]==0.24.0 | ||
| pydantic==2.5.0 | ||
|
|
||
| # Machine Learning and Transformers | ||
| torch>=2.0.0 | ||
| transformers>=4.35.0 | ||
| datasets>=2.15.0 | ||
| accelerate>=0.24.0 | ||
|
|
||
| # Audio processing for Whisper | ||
| openai-whisper>=20231117 | ||
| pydub>=0.25.1 | ||
| librosa>=0.10.0 | ||
|
|
||
| # Scientific computing | ||
| numpy>=1.24.0 | ||
| scipy>=1.11.0 | ||
|
|
||
| # Data processing and ML | ||
| scikit-learn>=1.3.0 | ||
| pandas>=2.1.0 | ||
|
|
||
| # Configuration and utilities | ||
| pyyaml>=6.0 | ||
| python-multipart>=0.0.6 | ||
|
|
||
| # Development and testing | ||
| pytest>=7.4.0 | ||
| pytest-asyncio>=0.21.0 | ||
| httpx>=0.25.0 | ||
| pytest-mock>=3.12.0 | ||
|
|
||
| # Logging and monitoring | ||
| structlog>=23.2.0 | ||
|
|
||
| # Optional: GPU support (uncomment if needed) | ||
| # torch-audio>=2.0.0 # For better audio processing on GPU | ||
|
|
||
| # Optional: Model optimization | ||
| # onnxruntime>=1.16.0 # For ONNX model inference | ||
| # optimum>=1.14.0 # For optimized transformers No newline at end of file |
There was a problem hiding this comment.
The dependencies in this file use >= which allows for installing newer versions of packages. This can lead to non-reproducible builds and potential breaking changes if a dependency releases a new version. It is a best practice to pin dependency versions using == for production environments to ensure consistency and stability. The other requirements-api.txt file at the root level does this correctly.
| parser.add_argument( | ||
| "--config", | ||
| default="configs/samo_api_config.yaml", | ||
| help="Path to configuration file" | ||
| ) |
There was a problem hiding this comment.
The --config command-line argument is parsed but never used. The SAMOUnifiedAPIServer is instantiated without any arguments, meaning it cannot use the provided configuration file. This can lead to unexpected behavior as the server might be running with default or hardcoded configurations.
| try: | ||
| logger.info("Loading T5 Summarization Model...") | ||
| self.models["summarizer"] = create_t5_summarizer("t5-small") | ||
| logger.info("✅ T5 Summarization Model loaded") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"❌ Failed to load T5 Summarization Model: {e}") | ||
| self.models["summarizer"] = None | ||
|
|
||
| try: | ||
| logger.info("Loading Whisper Transcription Model...") | ||
| self.models["transcriber"] = create_whisper_transcriber("base") | ||
| logger.info("✅ Whisper Transcription Model loaded") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"❌ Failed to load Whisper Transcription Model: {e}") | ||
| self.models["transcriber"] = None | ||
|
|
||
| try: | ||
| logger.info("Loading BERT Emotion Detection Model...") | ||
| model, loss_fn = create_samo_bert_emotion_classifier() | ||
| self.models["emotion_detector"] = model | ||
| logger.info("✅ BERT Emotion Detection Model loaded") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"❌ Failed to load BERT Emotion Detection Model: {e}") | ||
| self.models["emotion_detector"] = None |
There was a problem hiding this comment.
The models are loaded with hardcoded names (e.g., "t5-small", "base"). This makes it difficult to change models without modifying the code. The configuration should be loaded from a file like configs/samo_api_config.yaml, which already defines these model names. The SAMOUnifiedAPIServer should accept a configuration object or file path in its __init__ method and use it to load the models.
| requests_per_minute: 100 | ||
| burst_size: 10 | ||
| cors: | ||
| allowed_origins: ["*"] # Restrict to specific domains in production |
There was a problem hiding this comment.
Using "*" for allowed_origins is a security risk as it allows any website to make requests to your API. While the comment mentions restricting this in production, it's better to use a more specific origin even for development (e.g., http://localhost:3000) to prevent potential Cross-Site Request Forgery (CSRF) attacks.
allowed_origins: ["http://localhost:3000", "http://127.0.0.1:3000"] # Restrict to specific domains in production| emotions = [ | ||
| f"emotion_{i}" for i, p in enumerate(pred) if p > 0 | ||
| ] |
There was a problem hiding this comment.
The predicted emotions are labeled with generic names like emotion_0, emotion_1, etc. The src/models/emotion_detection/emotion_labels.py file provides a get_emotion_name(index) function that should be used to get the actual, human-readable emotion names. You will need to import get_emotion_name from models.emotion_detection.emotion_labels.
| emotions = [ | |
| f"emotion_{i}" for i, p in enumerate(pred) if p > 0 | |
| ] | |
| emotions = [ | |
| get_emotion_name(i) for i, p in enumerate(pred) if p > 0 | |
| ] |
| "text": text, | ||
| "emotions": result, | ||
| "confidence": max([r['score'] for r in result]), | ||
| "timestamp": "2025-09-10T10:00:00Z", # Replace with actual timestamp |
There was a problem hiding this comment.
The timestamp is hardcoded to a future date. This should be dynamically generated at the time of the request. You will need to import datetime.
| "timestamp": "2025-09-10T10:00:00Z", # Replace with actual timestamp | |
| "timestamp": datetime.now().isoformat() + "Z", |
| return { | ||
| "transcription": result['text'], | ||
| "language": result.get('language', 'en'), | ||
| "duration": len(audio_bytes) / 16000, # Approximate duration |
There was a problem hiding this comment.
The audio duration is approximated using len(audio_bytes) / 16000, which is incorrect and assumes a specific encoding that is not guaranteed. The file already imports librosa, which can be used to get the accurate duration. You should load the audio from audio_bytes into a numpy array and then use librosa.get_duration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (34)
requirements-api.txt (1)
44-44: Missing newline at end of fileAdd a newline character at the end of the file to follow Unix conventions.
# Pin versions to ensure reproducibility # Updated for compatibility with Python 3.12 +dependencies/requirements-api.txt (2)
33-36: Development dependencies should be in separate fileIncluding test dependencies in the production requirements file (even if commented as "Development and testing") can be confusing. Consider moving these to a separate
requirements-dev.txtfile.Would you like me to help create a separate
requirements-dev.txtfile for development and testing dependencies?
46-46: Missing newline at end of fileAdd a newline character at the end of the file.
# optimum>=1.14.0 # For optimized transformers +configs/samo_api_config.yaml (2)
48-49: Consider environment-based worker configurationThe number of workers (4) and timeout (120s) are hardcoded. These values should vary based on the deployment environment and available resources.
Consider making these configurable via environment variables:
server: - workers: 4 # Gunicorn workers - timeout: 120 # Request timeout in seconds + workers: ${API_WORKERS:4} # Gunicorn workers + timeout: ${API_TIMEOUT:120} # Request timeout in seconds
65-65: Missing newline at end of fileAdd a newline character at the end of the file to follow YAML best practices.
allowed_origins: ["https://yourdomain.com"] +src/unified_api_server.py (2)
1-1: Shebang present but file not executableThe file has a shebang line but is not marked as executable. Either remove the shebang or make the file executable.
Either remove the shebang:
-#!/usr/bin/env python3Or make the file executable (run in terminal):
chmod +x src/unified_api_server.py
189-190: Global model loading may cause startup delaysLoading all models synchronously at startup can cause significant delays and memory issues. Consider lazy loading or async initialization.
Consider implementing:
- Lazy loading - load models on first request
- Async model loading with health checks
- Model-specific endpoints that only load required models
- Environment variable to control which models to load
Would you like me to provide an implementation for any of these approaches?
src/models/emotion_detection/emotion_labels.py (7)
1-1: Drop the shebang or make the file executable.This is a library module, not an executable script. Either remove the shebang or mark the file as executable to satisfy linters.
-#!/usr/bin/env python3
180-183: Use explicit exception chaining suppression.Clarify origin by raising from None; also keeps tracebacks clean.
- except ValueError: - raise ValueError(f"Emotion '{emotion}' not found in GoEmotions list") + except ValueError as err: + raise ValueError(f"Emotion '{emotion}' not found in GoEmotions list") from None
199-202: Consistent exception style.Mirror the approach above for IndexError.
- else: - raise IndexError(f"Index {index} out of range for GoEmotions list") + else: + raise IndexError(f"Index {index} out of range for GoEmotions list") from None
215-215: Normalize group keys to be case-insensitive.This avoids surprising empty results for callers passing title/upper case.
- return EMOTION_VALENCE_GROUPS.get(valence, []) + return EMOTION_VALENCE_GROUPS.get(valence.lower(), [])
228-228: Also normalize arousal key.- return EMOTION_AROUSAL_GROUPS.get(arousal, []) + return EMOTION_AROUSAL_GROUPS.get(arousal.lower(), [])
241-241: Also normalize dominance key.- return EMOTION_DOMINANCE_GROUPS.get(dominance, []) + return EMOTION_DOMINANCE_GROUPS.get(dominance.lower(), [])
337-337: Remove redundant f-string.Fix F541.
- print(f"\nEmotion descriptions:") + print("\nEmotion descriptions:")scripts/start_api_server.py (3)
79-85: Doc links ignore chosen host/port; build from args.Current logs always point to localhost:8000. Use the provided host/port.
- logger.info("✅ Server initialized successfully") - logger.info("📖 API Documentation: http://localhost:8000/docs") - logger.info("🔄 ReDoc Documentation: http://localhost:8000/redoc") - logger.info("💚 Health Check: http://localhost:8000/health") + logger.info("✅ Server initialized successfully") + base = f"http://{args.host}:{args.port}" + logger.info(f"📖 API Documentation: {base}/docs") + logger.info(f"🔄 ReDoc Documentation: {base}/redoc") + logger.info(f"💚 Health Check: {base}/health")
88-90: Prefer logging.exception to capture traceback.Improves diagnosability on startup failures.
- except Exception as e: - logger.error(f"❌ Failed to start server: {e}") + except Exception: + logger.exception("❌ Failed to start server") sys.exit(1)
25-27: 0.0.0.0 default is fine for containers; consider 127.0.0.1 for local default.If this is primarily for local dev, binding to localhost reduces exposure. Keep as-is if you intend container/K8s usage.
configs/samo_emotion_detection_config.yaml (3)
13-13: Clean trailing spaces for YAML lint compliance.There are many trailing-space warnings. Run yamlfmt or pre-commit to auto-fix. No semantic changes.
- num_emotions: 28 + num_emotions: 28 # ...repeat for other trailing spaces...Also applies to: 16-16, 19-19, 28-28, 31-31, 34-34, 43-43, 47-47, 51-51, 55-55, 66-66, 69-69, 83-83, 86-86, 96-96, 105-105, 108-108, 117-117, 121-121, 124-124, 132-132, 135-135, 138-138, 141-141, 150-150, 154-154, 163-163, 166-166, 169-169, 177-177, 180-180, 183-183
10-22: Confirm threshold strategy alignment.prediction_threshold is 0.6 here, but API default (see EmotionDetectionRequest) is 0.5. Recommend making them consistent or documenting why they differ.
Do you want me to align the API’s default to 0.6?
160-172: Config isn’t wired into the server yet.The unified server doesn’t read this YAML. Consider passing a config path into server init and plumb relevant values (device, thresholds, rate limits) to components.
I can add a minimal loader and apply prediction_threshold and device to the emotion model at startup.
src/models/emotion_detection/samo_bert_emotion_classifier.py (5)
88-92: Use separate attention dropout config.You’re assigning attention_probs_dropout_prob from hidden_dropout_prob. Respect a distinct setting.
- self.config.hidden_dropout_prob = self.hidden_dropout_prob - self.config.attention_probs_dropout_prob = self.hidden_dropout_prob + self.config.hidden_dropout_prob = self.hidden_dropout_prob + # Allow distinct attention dropout if provided + attn_dp = config.get("attention_probs_dropout_prob", self.hidden_dropout_prob) + self.config.attention_probs_dropout_prob = attn_dp
190-193: Fix type hint for optional threshold.PEP 484: annotate optional properly.
- threshold: float = None, + threshold: Optional[float] = None,
254-261: Return semantic emotion names instead of “emotion_i”.Map indices to GoEmotions labels for human-friendly responses. Keep a fallback if import fails.
- for pred in batch_predictions: - emotions = [ - f"emotion_{i}" for i, p in enumerate(pred) if p > 0 - ] + # Prefer GoEmotions labels if available + try: + from .emotion_labels import GOEMOTIONS_EMOTIONS as _LABELS + except Exception: + _LABELS = None + for pred in batch_predictions: + emotions = [ + (_LABELS[i] if _LABELS and i < len(_LABELS) else f"emotion_{i}") + for i, p in enumerate(pred) if p > 0 + ]
318-329: Prefer BCEWithLogitsLoss for numerical stability.Computing BCE on sigmoid probabilities can be unstable. Switching to BCEWithLogitsLoss lets you pass raw logits.
- # Apply sigmoid to get probabilities - probabilities = torch.sigmoid(logits) - - # Compute BCE loss - bce_loss = F.binary_cross_entropy( - probabilities, targets.float(), reduction="none" - ) + # Compute BCE with logits for stability + bce_loss = F.binary_cross_entropy_with_logits( + logits, targets.float(), reduction="none" + )If you adopt this, ensure any training code passes raw logits into the loss.
86-87: Align API default threshold with model default (0.6).Model uses 0.6; API request model currently defaults to 0.5. Consider aligning (see unified_api_server.py).
test_samo_emotion_detection_standalone.py (2)
50-85: Show meaningful emotion names in outputs.You print top probabilities mapped to GoEmotions but “Detected emotions” shows “emotion_i”. After classifier change (mapping), this will improve; otherwise map via labels here as well.
200-202: Catching bare Exception in test harness.It’s okay for a CLI harness, but consider logging the traceback with traceback.print_exc() (you already do in the previous test) for symmetry.
tests/test_unified_api_server.py (1)
56-89: Avoid heavy model downloads in tests; patch summarizer like other endpoints.test_summarize_endpoint_success loads a real T5 model, inflating CI time. Mirror the mocking approach used for transcribe/combined tests.
I can provide a small patch to mock create_t5_summarizer for this test to keep runtime <1s.
src/models/unified_api_server.py (6)
209-212: Log full tracebacks on errors.Use logging.exception to include stack traces; keeps the HTTP responses unchanged.
- except Exception as e: - logger.error(f"Summarization error: {e}") + except Exception: + logger.exception("Summarization error") raise HTTPException(status_code=500, detail=f"Summarization failed: {str(e)}")
257-260: Same for transcription error handling.- except Exception as e: - logger.error(f"Transcription error: {e}") + except Exception: + logger.exception("Transcription error") raise HTTPException(status_code=500, detail=f"Transcription failed: {str(e)}")
289-292: Same for emotion detection error handling.- except Exception as e: - logger.error(f"Emotion detection error: {e}") + except Exception: + logger.exception("Emotion detection error") raise HTTPException(status_code=500, detail=f"Emotion detection failed: {str(e)}")
397-400: Same for combined pipeline error handling.- except Exception as e: - logger.error(f"Combined processing error: {e}") + except Exception: + logger.exception("Combined processing error") raise HTTPException(status_code=500, detail=f"Processing failed: {str(e)}")
82-84: Align API default threshold to model’s 0.6 (config).Reduces surprises between API and classifier defaults.
- threshold: Optional[float] = Field(0.5, ge=0.1, le=0.9, description="Prediction threshold") + threshold: Optional[float] = Field(0.6, ge=0.1, le=0.9, description="Prediction threshold")
24-36: Minor: drop unused imports and avoid global basicConfig in library code.Consider removing unused JSONResponse/validator and configuring logging in entrypoints only. Low priority.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
configs/samo_api_config.yaml(1 hunks)configs/samo_emotion_detection_config.yaml(1 hunks)dependencies/requirements-api.txt(1 hunks)requirements-api.txt(1 hunks)scripts/start_api_server.py(1 hunks)src/models/emotion_detection/emotion_labels.py(1 hunks)src/models/emotion_detection/samo_bert_emotion_classifier.py(1 hunks)src/models/unified_api_server.py(1 hunks)src/unified_api_server.py(1 hunks)test_samo_emotion_detection_standalone.py(1 hunks)tests/test_unified_api_server.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
scripts/start_api_server.py (1)
src/models/unified_api_server.py (1)
SAMOUnifiedAPIServer(115-427)
test_samo_emotion_detection_standalone.py (2)
src/models/emotion_detection/samo_bert_emotion_classifier.py (5)
create_samo_bert_emotion_classifier(388-428)count_parameters(275-277)count_frozen_parameters(279-281)predict_emotions(187-268)set_temperature(270-273)src/models/emotion_detection/emotion_labels.py (2)
get_all_emotions(270-277)get_emotion_description(244-254)
tests/test_unified_api_server.py (2)
src/models/unified_api_server.py (1)
SAMOUnifiedAPIServer(115-427)scripts/start_api_server.py (1)
main(20-90)
src/models/unified_api_server.py (3)
src/models/summarization/t5_summarizer.py (1)
T5SummarizationModel(120-348)src/models/voice_processing/whisper_transcriber.py (1)
WhisperTranscriber(181-438)src/models/emotion_detection/samo_bert_emotion_classifier.py (2)
create_samo_bert_emotion_classifier(388-428)SAMOBERTEmotionClassifier(38-281)
🪛 Ruff (0.12.2)
scripts/start_api_server.py
1-1: Shebang is present but file is not executable
(EXE001)
25-25: Possible binding to all interfaces
(S104)
88-88: Do not catch blind exception: Exception
(BLE001)
89-89: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
test_samo_emotion_detection_standalone.py
1-1: Shebang is present but file is not executable
(EXE001)
200-200: Do not catch blind exception: Exception
(BLE001)
src/unified_api_server.py
1-1: Shebang is present but file is not executable
(EXE001)
tests/test_unified_api_server.py
1-1: Shebang is present but file is not executable
(EXE001)
src/models/emotion_detection/emotion_labels.py
1-1: Shebang is present but file is not executable
(EXE001)
183-183: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
183-183: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
337-337: f-string without any placeholders
Remove extraneous f prefix
(F541)
src/models/unified_api_server.py
1-1: Shebang is present but file is not executable
(EXE001)
153-153: Do not catch blind exception: Exception
(BLE001)
154-154: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
162-162: Do not catch blind exception: Exception
(BLE001)
163-163: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
172-172: Do not catch blind exception: Exception
(BLE001)
173-173: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
209-209: Do not catch blind exception: Exception
(BLE001)
210-210: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
211-211: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
211-211: Use explicit conversion flag
Replace with conversion flag
(RUF010)
216-216: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
230-230: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
257-257: Do not catch blind exception: Exception
(BLE001)
258-258: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
259-259: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
259-259: Use explicit conversion flag
Replace with conversion flag
(RUF010)
289-289: Do not catch blind exception: Exception
(BLE001)
290-290: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
291-291: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
291-291: Use explicit conversion flag
Replace with conversion flag
(RUF010)
296-296: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
309-309: Abstract raise to an inner function
(TRY301)
311-311: Probable insecure usage of temporary file or directory: "/tmp/"
(S108)
397-397: Do not catch blind exception: Exception
(BLE001)
398-398: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
399-399: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
399-399: Use explicit conversion flag
Replace with conversion flag
(RUF010)
424-424: Possible binding to all interfaces
(S104)
src/models/emotion_detection/samo_bert_emotion_classifier.py
1-1: Shebang is present but file is not executable
(EXE001)
190-190: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
🪛 YAMLlint (1.37.1)
configs/samo_api_config.yaml
[error] 65-65: no new line character at the end of file
(new-line-at-end-of-file)
configs/samo_emotion_detection_config.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[error] 28-28: trailing spaces
(trailing-spaces)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 66-66: trailing spaces
(trailing-spaces)
[error] 69-69: trailing spaces
(trailing-spaces)
[error] 83-83: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 96-96: trailing spaces
(trailing-spaces)
[error] 105-105: trailing spaces
(trailing-spaces)
[error] 108-108: trailing spaces
(trailing-spaces)
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 121-121: trailing spaces
(trailing-spaces)
[error] 124-124: trailing spaces
(trailing-spaces)
[error] 132-132: trailing spaces
(trailing-spaces)
[error] 135-135: trailing spaces
(trailing-spaces)
[error] 138-138: trailing spaces
(trailing-spaces)
[error] 141-141: trailing spaces
(trailing-spaces)
[error] 150-150: trailing spaces
(trailing-spaces)
[error] 154-154: trailing spaces
(trailing-spaces)
[error] 163-163: trailing spaces
(trailing-spaces)
[error] 166-166: trailing spaces
(trailing-spaces)
[error] 169-169: trailing spaces
(trailing-spaces)
[error] 177-177: trailing spaces
(trailing-spaces)
[error] 180-180: trailing spaces
(trailing-spaces)
[error] 183-183: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
dependencies/requirements-api.txt (1)
16-16: openai-whisper version constraint is valid
The package uses date-based versions (e.g. 20240930, 20250625), soopenai-whisper>=20231117is correct and won’t cause installation issues.Likely an incorrect or invalid review comment.
| # This file configures the unified API server for SAMO-DL | ||
|
|
||
| api: | ||
| host: "0.0.0.0" |
There was a problem hiding this comment.
Security concern: Binding to all interfaces
The API is configured to bind to 0.0.0.0, which exposes it to all network interfaces. This could be a security risk if not properly firewalled.
Consider using 127.0.0.1 for local development and only use 0.0.0.0 in production with proper security measures:
- host: "0.0.0.0"
+ host: "127.0.0.1" # Use 0.0.0.0 only in production with proper firewall📝 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.
| host: "0.0.0.0" | |
| host: "127.0.0.1" # Use 0.0.0.0 only in production with proper firewall |
🤖 Prompt for AI Agents
In configs/samo_api_config.yaml around line 5, the host is set to "0.0.0.0"
which exposes the API on all interfaces; change this for development to
"127.0.0.1" and make host configurable via an environment variable or separate
prod config so production can use "0.0.0.0" only when behind proper
firewalls/load balancers; update deployment/docs to ensure the env/config value
is set appropriately and add a comment in the file warning about using 0.0.0.0
only in secured production environments.
| requests_per_minute: 100 | ||
| burst_size: 10 | ||
| cors: | ||
| allowed_origins: ["*"] # Restrict to specific domains in production |
There was a problem hiding this comment.
CORS security risk in default configuration
The CORS configuration allows all origins (["*"]) by default, which could expose the API to cross-origin attacks. The comment mentions restricting in production, but it's safer to be restrictive by default.
- allowed_origins: ["*"] # Restrict to specific domains in production
+ allowed_origins: ["http://localhost:3000"] # Add specific domains as needed📝 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.
| allowed_origins: ["*"] # Restrict to specific domains in production | |
| allowed_origins: ["http://localhost:3000"] # Add specific domains as needed |
🤖 Prompt for AI Agents
In configs/samo_api_config.yaml around line 17, the CORS setting currently
allows all origins (allowed_origins: ["*"]) which is insecure by default; change
this to a restrictive default such as an explicit whitelist of trusted domains
(e.g., your dev/staging/production hostnames) or an empty list, and document
that environments should override this value via environment-specific config or
secrets management; ensure any deployment process injects the appropriate domain
list for production rather than relying on "*".
| torch>=2.0.0 | ||
| transformers>=4.35.0 | ||
| datasets>=2.15.0 | ||
| accelerate>=0.24.0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pinning ML dependencies for production stability
Using flexible version constraints (>=) for critical ML libraries can lead to unexpected behavior when new versions are released. For production deployments, consider pinning exact versions or using more restrictive ranges.
Apply this diff for better version control:
-torch>=2.0.0
-transformers>=4.35.0
-datasets>=2.15.0
-accelerate>=0.24.0
+torch>=2.0.0,<3.0.0
+transformers>=4.35.0,<5.0.0
+datasets>=2.15.0,<3.0.0
+accelerate>=0.24.0,<1.0.0📝 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.
| torch>=2.0.0 | |
| transformers>=4.35.0 | |
| datasets>=2.15.0 | |
| accelerate>=0.24.0 | |
| --- dependencies/requirements-api.txt | |
| @@ Lines 10-13 | |
| -torch>=2.0.0 | |
| -transformers>=4.35.0 | |
| -datasets>=2.15.0 | |
| torch>=2.0.0,<3.0.0 | |
| transformers>=4.35.0,<5.0.0 | |
| datasets>=2.15.0,<3.0.0 | |
| accelerate>=0.24.0,<1.0.0 |
🤖 Prompt for AI Agents
In dependencies/requirements-api.txt around lines 10 to 13, the ML dependencies
use open-ended >= version specifiers which risks surprising upgrades in
production; replace these with pinned, tested versions (or narrow ranges with
both minimum and maximum bounds) for torch, transformers, datasets, and
accelerate in this file so CI/builds/installations use deterministic, validated
releases.
| Flask==3.0.3 | ||
| Flask-RESTX==1.3.0 |
There was a problem hiding this comment.
Framework version mismatch with dependencies folder
There's an inconsistency between the Flask version specified here (3.0.3) and the FastAPI framework used in dependencies/requirements-api.txt. Since the PR introduces a unified API server primarily using FastAPI, consider aligning these dependencies or clarifying the intended usage of both frameworks.
🤖 Prompt for AI Agents
In requirements-api.txt around lines 5-6, the file pins Flask==3.0.3 while the
project/PR is standardizing on FastAPI; remove this mismatch by deciding which
framework is intended and aligning dependencies accordingly: if FastAPI is the
chosen server, remove Flask from requirements-api.txt (or move it to a separate
optional/dev requirements file with a comment explaining why both are needed),
add the correct FastAPI package/version to this file if missing, and update any
dependency lists in the dependencies/ folder and CI/install scripts to reference
the unified requirements so installs/tests reflect the intended framework.
| torch==2.4.1 | ||
| transformers==4.45.2 | ||
| accelerate==1.0.1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PyTorch version should be pinned for reproducibility
The torch version is pinned here (2.4.1) but uses a flexible constraint (>=2.0.0) in dependencies/requirements-api.txt. For production deployments, consider using the same pinned version across both files to ensure reproducibility.
Apply this diff to align versions:
-torch==2.4.1
+torch>=2.0.0 # Or pin both to 2.4.1📝 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.
| torch==2.4.1 | |
| transformers==4.45.2 | |
| accelerate==1.0.1 | |
| ++ b/requirements-api.txt | |
| @@ Lines 20-22 | |
| torch>=2.0.0 # Or pin both to 2.4.1 for strict reproducibility | |
| transformers==4.45.2 | |
| accelerate==1.0.1 |
🤖 Prompt for AI Agents
In requirements-api.txt around lines 20 to 22, the file pins torch to 2.4.1 but
a different file (dependencies/requirements-api.txt) uses a flexible constraint
(>=2.0.0); update the dependency declaration in
dependencies/requirements-api.txt to match and pin torch to torch==2.4.1 so both
files use the identical exact version, ensuring consistent reproducible installs
across environments.
| # Use emotion pipeline | ||
| result = emotion_pipeline(text) | ||
|
|
||
| return { | ||
| "text": text, | ||
| "emotions": result, | ||
| "confidence": max([r['score'] for r in result]), |
There was a problem hiding this comment.
Error handling for missing emotion pipeline
The emotion analysis endpoint doesn't check if the emotion_pipeline is loaded before use, which could cause AttributeError.
def post(self):
try:
+ if emotion_pipeline is None:
+ return {"error": "Emotion detection model not loaded"}, 503
+
data = request.get_json()
request_model = PredictionRequest(**data)📝 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.
| # Use emotion pipeline | |
| result = emotion_pipeline(text) | |
| return { | |
| "text": text, | |
| "emotions": result, | |
| "confidence": max([r['score'] for r in result]), | |
| def post(self): | |
| try: | |
| if emotion_pipeline is None: | |
| return {"error": "Emotion detection model not loaded"}, 503 | |
| data = request.get_json() | |
| request_model = PredictionRequest(**data) | |
| # Use emotion pipeline | |
| result = emotion_pipeline(text) | |
| return { | |
| "text": text, | |
| "emotions": result, | |
| "confidence": max([r['score'] for r in result]), | |
| } | |
| except Exception as e: | |
| # existing error handling... | |
| ... |
🤖 Prompt for AI Agents
In src/unified_api_server.py around lines 108 to 114, the code calls
emotion_pipeline(text) without verifying the pipeline is initialized; add a
guard that checks if emotion_pipeline is present and callable before using it,
and if not return an appropriate error (e.g., raise an HTTP 503/ValueError or
JSON response indicating the pipeline is not loaded) with a clear message;
alternatively initialize or lazy-load the pipeline prior to calling it, and
ensure any exceptions from the pipeline call are caught and converted to a
controlled error response.
| "text": text, | ||
| "emotions": result, | ||
| "confidence": max([r['score'] for r in result]), | ||
| "timestamp": "2025-09-10T10:00:00Z", # Replace with actual timestamp |
There was a problem hiding this comment.
Hardcoded timestamps should use actual time
Multiple endpoints return hardcoded timestamps. This makes debugging and monitoring difficult.
Add this import at the top:
+from datetime import datetimeThen update the timestamps:
- "timestamp": "2025-09-10T10:00:00Z", # Replace with actual timestamp
+ "timestamp": datetime.utcnow().isoformat() + "Z",Also applies to: 137-137, 162-162, 182-182
🤖 Prompt for AI Agents
In src/unified_api_server.py around lines 115, 137, 162, and 182, replace the
hardcoded timestamp strings with generated UTC timestamps and add the import at
the top; specifically add an import for datetime/timezone (e.g. from datetime
import datetime, timezone) and change each hardcoded value to generate the
current UTC ISO8601 timestamp (for example using
datetime.now(timezone.utc).isoformat() or equivalent) so endpoints return the
actual current time with a 'Z' or timezone info.
| results = emotion_pipeline(texts) | ||
|
|
||
| return { | ||
| "results": [ | ||
| { | ||
| "text": texts[i], | ||
| "emotions": [results[i]], | ||
| "confidence": max([r['score'] for r in results[i]]), | ||
| "timestamp": "2025-09-10T10:00:00Z", | ||
| "request_id": request.headers.get('X-Request-ID', 'unknown') | ||
| } for i in range(len(texts)) | ||
| ] | ||
| } |
There was a problem hiding this comment.
Incorrect batch processing result structure
The batch emotion analysis has a bug in handling results. When processing multiple texts, the pipeline returns a list, not individual results indexed.
results = emotion_pipeline(texts)
return {
"results": [
{
"text": texts[i],
- "emotions": [results[i]],
- "confidence": max([r['score'] for r in results[i]]),
+ "emotions": results[i],
+ "confidence": max([r['score'] for r in results[i]]) if results[i] else 0,
"timestamp": "2025-09-10T10:00:00Z",
"request_id": request.headers.get('X-Request-ID', 'unknown')
} for i in range(len(texts))
]
}📝 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.
| results = emotion_pipeline(texts) | |
| return { | |
| "results": [ | |
| { | |
| "text": texts[i], | |
| "emotions": [results[i]], | |
| "confidence": max([r['score'] for r in results[i]]), | |
| "timestamp": "2025-09-10T10:00:00Z", | |
| "request_id": request.headers.get('X-Request-ID', 'unknown') | |
| } for i in range(len(texts)) | |
| ] | |
| } | |
| results = emotion_pipeline(texts) | |
| return { | |
| "results": [ | |
| { | |
| "text": texts[i], | |
| "emotions": results[i], | |
| "confidence": max([r['score'] for r in results[i]]) if results[i] else 0, | |
| "timestamp": "2025-09-10T10:00:00Z", | |
| "request_id": request.headers.get('X-Request-ID', 'unknown') | |
| } for i in range(len(texts)) | |
| ] | |
| } |
🤖 Prompt for AI Agents
In src/unified_api_server.py around lines 129 to 141, the batch handling is
wrong: emotion_pipeline returns a list of per-text result lists, but the code
incorrectly wraps each per-text result in an extra list and indexes by texts
length; change the comprehension to iterate over enumerate(results) (or
zip(texts, results)) so you get each text and its corresponding emotion list,
set "emotions" to the per-text result directly (no extra brackets), compute
"confidence" from that per-text list (max(r['score'] for r in
per_text_results)), and ensure you validate lengths or fallback if results and
texts counts differ; keep the request_id and timestamp logic as-is.
| # Use whisper pipeline | ||
| result = whisper_pipeline(audio_bytes) | ||
|
|
There was a problem hiding this comment.
Missing error handling for Whisper pipeline
The transcription endpoint doesn't check if the whisper_pipeline is loaded and lacks error handling for audio processing.
def post(self):
+ if whisper_pipeline is None:
+ return {"error": "Whisper model not loaded"}, 503
+
if 'audio_file' not in request.files:
return {"error": "No audio file provided"}, 400
audio_file = request.files['audio_file']
audio_bytes = audio_file.read()
- # Use whisper pipeline
- result = whisper_pipeline(audio_bytes)
+ try:
+ # Use whisper pipeline
+ result = whisper_pipeline(audio_bytes)
+ except Exception as e:
+ log.error(f"Transcription failed: {str(e)}")
+ return {"error": "Transcription failed"}, 500📝 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.
| # Use whisper pipeline | |
| result = whisper_pipeline(audio_bytes) | |
| def post(self): | |
| # Ensure the Whisper model is loaded | |
| if whisper_pipeline is None: | |
| return {"error": "Whisper model not loaded"}, 503 | |
| if 'audio_file' not in request.files: | |
| return {"error": "No audio file provided"}, 400 | |
| audio_file = request.files['audio_file'] | |
| audio_bytes = audio_file.read() | |
| try: | |
| # Use whisper pipeline | |
| result = whisper_pipeline(audio_bytes) | |
| except Exception as e: | |
| log.error(f"Transcription failed: {str(e)}") | |
| return {"error": "Transcription failed"}, 500 | |
| # ... (rest of handler) |
🤖 Prompt for AI Agents
In src/unified_api_server.py around lines 155 to 157, the code calls
whisper_pipeline(audio_bytes) without verifying the pipeline is initialized or
handling runtime errors; add a guard and robust error handling by checking
whisper_pipeline is not None (or loaded) before calling it, validate audio_bytes
is non-empty and of expected type/format, wrap the pipeline call in a try/except
to catch and log exceptions (including the exception message and stacktrace),
and return an appropriate HTTP error response (400 for bad input, 500 for
processing errors) with a clear message so the endpoint fails gracefully instead
of crashing.
| def test_model_unavailable_errors(self, client): | ||
| """Test error handling when models are not available.""" | ||
| # Temporarily set models to None | ||
| original_models = client.app.state.models.copy() | ||
|
|
||
| try: | ||
| # Mock unavailable models | ||
| client.app.state.models = { | ||
| "summarizer": None, | ||
| "transcriber": None, | ||
| "emotion_detector": None | ||
| } | ||
|
|
||
| # Test summarization | ||
| response = client.post("/summarize", json={"text": "Test text"}) | ||
| assert response.status_code == 503 | ||
| assert "not available" in response.json()["detail"] | ||
|
|
||
| # Test emotion detection | ||
| response = client.post("/detect-emotions", json={"text": "Test text"}) | ||
| assert response.status_code == 503 | ||
| assert "not available" in response.json()["detail"] | ||
|
|
||
| finally: | ||
| # Restore original models | ||
| client.app.state.models = original_models | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
client.app.state.models is assumed but never set by the server.
The server stores models on self.models only. Set app.state.models = self.models in server init so these tests can toggle availability without private access. See suggested change in unified_api_server.py.
🏁 Script executed:
#!/usr/bin/env bash
set -eo pipefail
# Locate the server implementation file
file=$(find . -type f -name unified_api_server.py)
echo "Server file: $file"
# Check for setting models on app.state
grep -R "app\.state\.models" -n "$file" || echo "No app.state.models references found"
# Check for storing models on self.models
grep -R "self\.models" -n "$file" || echo "No self.models references found"
# Check for FastAPI or Starlette app initialization
grep -R "FastAPI" -n "$file" || grep -R "Starlette" -n "$file" || echo "No FastAPI/Starlette import found"Length of output: 562
🏁 Script executed:
#!/usr/bin/env bash
set -eo pipefail
FILE="src/models/unified_api_server.py"
if [[ -f "$FILE" ]]; then
sed -n '1,200p' "$FILE"
else
echo "$FILE not found"
fiLength of output: 7156
Set app.state.models in server init
In src/models/unified_api_server.py inside SAMOUnifiedAPIServer.__init__, after self._load_models(), add:
self.app.state.models = self.modelsso tests toggling client.app.state.models pass.
🤖 Prompt for AI Agents
In src/models/unified_api_server.py around the SAMOUnifiedAPIServer.__init__
(add after self._load_models()), the app.state.models attribute is not set which
causes tests that mutate client.app.state.models to fail; set
self.app.state.models = self.models immediately after loading models so the
TestClient can read and modify that mapping during tests, ensuring the tests
that toggle client.app.state.models pass and restoring the original models
works.
Implements PR-5: API dependencies including requirements-api.txt and samo_api_config.yaml. All dependencies installed and verified. Part of surgical breakdown of PR #145. References PR #147.
Summary by CodeRabbit
New Features
Chores
Tests