feat: add comprehensive CI pipeline orchestration system - #183
Merged
Conversation
Extracted from monster PR #171 as part of Phase 3A strategic decomposition. Comprehensive CI pipeline runner with: - Multi-environment detection (local/Colab/CI) - GPU compatibility testing with PyTorch - Dependency validation for all required packages - Individual CI script orchestration with timeouts - Unit and E2E test execution (pytest integration) - Performance benchmarking with thresholds - Detailed reporting with success metrics - CI artifact generation (conditional report writing) - Graceful error handling and logging Supports both local development and production CI workflows. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Reviewer's GuideThis PR introduces a unified CI orchestration script that structures the entire testing workflow—from environment detection and dependency checks to script execution, unit/E2E tests, GPU compatibility, performance benchmarks, and detailed reporting—using standardized subprocess invocations, enhanced logging, and robust error handling. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@uelkerd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds new subprocess helper and orchestration entrypoints, GPU compatibility checks and benchmarking, improved exception logging and report generation, performance measurements, and centralized exit handling in the CI pipeline runner. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Trigger as Developer/CI Trigger
participant Runner as CIPipelineRunner
participant Subp as Subprocess Helper
participant Scripts as CI Scripts
participant Tests as Unit & E2E Tests
participant GPU as GPU Checks
participant Bench as Benchmarks
participant Log as Logger
Trigger->>Runner: start()
Runner->>Log: validate environment & deps
rect rgb(245,248,255)
Runner->>Subp: _run_subprocess_command(cmd)
Subp-->>Runner: CompletedProcess / error
Runner->>Scripts: run static CI scripts (incl. ONNX)
Scripts-->>Runner: per-script results
end
Runner->>Tests: run unit & e2e tests
Tests-->>Runner: results
alt PyTorch available
Runner->>GPU: _test_gpu_model_forward_pass()
GPU-->>Runner: GPU validation results
else PyTorch missing / ImportError
Runner->>Log: logger.warning / skip GPU checks
end
Runner->>Bench: run_performance_benchmarks()
Bench-->>Runner: load_time, inference_time
rect rgb(245,255,245)
Runner->>Runner: aggregate results & validate thresholds
Runner->>Log: logger.exception on unexpected errors
Runner-->>Trigger: exit(code) via run_pipeline_and_exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Contributor
|
Here's the code health analysis summary for commits Analysis Summary
|
Contributor
Summary of ChangesHello @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 introduces a dedicated and comprehensive CI pipeline orchestration system, strategically extracted from a larger development effort. Its primary purpose is to streamline and standardize the end-to-end testing process, ensuring consistent quality and performance across different development and deployment environments. The system is designed to be robust, covering everything from environment detection and dependency validation to various testing phases (unit, E2E, GPU compatibility) and performance benchmarks, ultimately providing detailed insights into the health of the codebase. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive CI pipeline orchestration system that provides end-to-end testing workflow management for the SAMO Deep Learning project. The system detects environments automatically, validates dependencies, manages script execution with timeout protection, and generates detailed reports.
Key changes include:
- Multi-environment detection (local/Colab/CI) with GPU compatibility testing
- Robust error handling with timeout protection and graceful degradation
- Comprehensive logging and reporting system with CI artifact generation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In detect_environment you reference env_info["is_colab"] without ever setting that key, which will raise a KeyError—consider adding a proper Colab check or defining is_colab.
- You have repeated subprocess.run invocations with nearly identical parameters in multiple methods—extract a shared helper to reduce duplication and centralize timeout/logging behavior.
- The dynamic sys.path.insert calls in GPU and benchmark tests signal that your code isn’t installed as a package—consider packaging src/ (or using an editable install) so you can import models cleanly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In detect_environment you reference env_info["is_colab"] without ever setting that key, which will raise a KeyError—consider adding a proper Colab check or defining is_colab.
- You have repeated subprocess.run invocations with nearly identical parameters in multiple methods—extract a shared helper to reduce duplication and centralize timeout/logging behavior.
- The dynamic sys.path.insert calls in GPU and benchmark tests signal that your code isn’t installed as a package—consider packaging src/ (or using an editable install) so you can import models cleanly.
## Individual Comments
### Comment 1
<location> `scripts/ci/run_full_ci_pipeline.py:130` </location>
<code_context>
missing_packages.append(package)
- logger.error(f"❌ {package} missing")
-
+ logger.exception(f"❌ {package} missing")
+
if missing_packages:
</code_context>
<issue_to_address>
**suggestion:** Using logger.exception for missing package may be excessive.
Prefer logger.error over logger.exception here to avoid unnecessary stack traces for expected ImportErrors.
```suggestion
logger.error(f"❌ {package} missing")
```
</issue_to_address>
### Comment 2
<location> `scripts/ci/run_full_ci_pipeline.py:163-165` </location>
<code_context>
+
except subprocess.TimeoutExpired:
- logger.error(f"⏰ {script_path} TIMEOUT")
+ logger.exception(f"⏰ {script_path} TIMEOUT")
return False, "Script timed out after 5 minutes"
except Exception as e:
</code_context>
<issue_to_address>
**suggestion:** logger.exception on TimeoutExpired may be unnecessary.
Since TimeoutExpired is expected, logger.error is preferable to avoid unnecessary stack traces.
```suggestion
except subprocess.TimeoutExpired:
logger.error(f"⏰ {script_path} TIMEOUT")
return False, "Script timed out after 5 minutes"
```
</issue_to_address>
### Comment 3
<location> `scripts/ci/run_full_ci_pipeline.py:167-168` </location>
<code_context>
+
except subprocess.TimeoutExpired:
- logger.error("⏰ Unit tests TIMEOUT")
+ logger.exception("⏰ Unit tests TIMEOUT")
return False
except Exception as e:
</code_context>
<issue_to_address>
**suggestion:** logger.exception for unit test timeout may be excessive.
Since timeouts are common in CI, using logger.error instead will reduce log noise from stack traces.
```suggestion
logger.error(f"⏰ Unit tests TIMEOUT: {script_path} ERROR: {e}")
return False, str(e)
```
</issue_to_address>
### Comment 4
<location> `scripts/ci/run_full_ci_pipeline.py:148-154` </location>
<code_context>
result = subprocess.run(
[python_executable, script_path],
check=False,
capture_output=True,
text=True,
timeout=300, # 5 minute timeout
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 5
<location> `scripts/ci/run_full_ci_pipeline.py:175-181` </location>
<code_context>
result = subprocess.run(
[sys.executable, "-m", "pytest", "tests/unit/", "-v"],
check=False,
capture_output=True,
text=True,
timeout=1200, # 20 minute timeout (increased from 10)
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location> `scripts/ci/run_full_ci_pipeline.py:204-210` </location>
<code_context>
result = subprocess.run(
[sys.executable, "-m", "pytest", "tests/e2e/", "-v"],
check=False,
capture_output=True,
text=True,
timeout=900, # 15 minute timeout
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location> `scripts/ci/run_full_ci_pipeline.py:145` </location>
<code_context>
def run_ci_script(self, script_path: str) -> Tuple[bool, str]:
"""Run a single CI script and return success status and output."""
logger.info(f"🚀 Running {script_path}...")
try:
# Use the correct Python interpreter
python_executable = sys.executable
# Run the script
result = subprocess.run(
[python_executable, script_path],
check=False,
capture_output=True,
text=True,
timeout=300, # 5 minute timeout
)
if result.returncode == 0:
logger.info(f"✅ {script_path} PASSED")
return True, result.stdout
logger.error(f"❌ {script_path} FAILED")
logger.error(f"Error output: {result.stderr}")
return False, result.stderr
except subprocess.TimeoutExpired:
logger.exception(f"⏰ {script_path} TIMEOUT")
return False, "Script timed out after 5 minutes"
except Exception as e:
logger.exception(f"💥 {script_path} ERROR: {e}")
return False, str(e)
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>
### Comment 8
<location> `scripts/ci/run_full_ci_pipeline.py:175` </location>
<code_context>
def run_unit_tests(self) -> bool:
"""Run unit tests."""
logger.info("🧪 Running unit tests...")
try:
result = subprocess.run(
[sys.executable, "-m", "pytest", "tests/unit/", "-v"],
check=False,
capture_output=True,
text=True,
timeout=1200, # 20 minute timeout (increased from 10)
)
if result.returncode == 0:
logger.info("✅ Unit tests PASSED")
return True
logger.error("❌ Unit tests FAILED")
logger.error(f"Return code: {result.returncode}")
logger.error(f"Error output: {result.stderr}")
logger.error(f"Standard output: {result.stdout}")
return False
except subprocess.TimeoutExpired:
logger.exception("⏰ Unit tests TIMEOUT")
return False
except Exception as e:
logger.exception(f"💥 Unit tests ERROR: {e}")
return False
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>
### Comment 9
<location> `scripts/ci/run_full_ci_pipeline.py:228` </location>
<code_context>
def test_gpu_compatibility(self) -> bool:
"""Test GPU compatibility if available."""
logger.info("🖥️ Testing GPU compatibility...")
try:
import torch
if not torch.cuda.is_available():
logger.info("ℹ️ No GPU available, skipping GPU tests")
return True
logger.info(f"🎮 GPU detected: {torch.cuda.get_device_name(0)}")
# Test GPU model loading
device = torch.device("cuda")
# Add src to path for imports
import sys
sys.path.insert(0, str(Path(__file__).parent.parent.parent / "src"))
# Test BERT on GPU
try:
from models.emotion_detection.bert_classifier import (
BERTEmotionClassifier,
)
except ImportError:
from src.models.emotion_detection.bert_classifier import (
BERTEmotionClassifier,
)
model = BERTEmotionClassifier().to(device)
# Test forward pass
import torch
dummy_input = torch.randint(0, 1000, (2, 512)).to(device)
with torch.no_grad():
output = model(dummy_input, torch.ones_like(dummy_input))
logger.info(f"✅ GPU forward pass successful, output shape: {output.shape}")
return True
except Exception as e:
logger.exception(f"❌ GPU compatibility test failed: {e}")
return False
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>
### Comment 10
<location> `scripts/ci/run_full_ci_pipeline.py:275` </location>
<code_context>
def run_performance_benchmarks(self) -> bool:
"""Run performance benchmarks."""
logger.info("⚡ Running performance benchmarks...")
try:
# Simple performance test - model loading speed
import time
import torch
# Test BERT model loading speed
start_time = time.time()
# Add src to path
import sys
sys.path.insert(0, str(Path(__file__).parent.parent.parent / "src"))
try:
from models.emotion_detection.bert_classifier import (
BERTEmotionClassifier,
)
except ImportError:
from src.models.emotion_detection.bert_classifier import (
BERTEmotionClassifier,
)
model = BERTEmotionClassifier()
loading_time = time.time() - start_time
# Test inference speed
start_time = time.time()
dummy_input = torch.randint(0, 1000, (1, 512))
with torch.no_grad():
model(dummy_input, torch.ones_like(dummy_input))
inference_time = time.time() - start_time
logger.info(f"✅ Model loading time: {loading_time:.2f}s")
logger.info(f"✅ Inference time: {inference_time:.2f}s")
# Check if times are reasonable
if (
loading_time < 10.0 and inference_time < 5.0
): # Increased threshold for CPU environments
logger.info("✅ Performance benchmarks passed")
return True
logger.error(
f"❌ Performance too slow - loading: {loading_time:.2f}s, inference: {inference_time:.2f}s",
)
return False
except Exception as e:
logger.exception(f"❌ Performance benchmark failed: {e}")
return False
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>
### Comment 11
<location> `scripts/ci/run_full_ci_pipeline.py:421` </location>
<code_context>
def main():
"""Main function to run the CI pipeline."""
runner = CIPipelineRunner()
try:
_ = runner.run_full_pipeline()
report = runner.generate_report()
print(report)
# Only write report to file in CI so it can be uploaded as an artifact
write_ci_report_if_needed(report)
# Exit with appropriate code
_, total_tests, passed_tests = runner._get_test_stats()
if passed_tests == total_tests:
logger.info("🎉 CI Pipeline completed successfully!")
sys.exit(0)
else:
logger.error("❌ CI Pipeline failed!")
sys.exit(1)
except KeyboardInterrupt:
logger.info("⏹️ CI Pipeline interrupted by user")
sys.exit(1)
except Exception as e:
logger.exception(f"💥 CI Pipeline crashed: {e}")
sys.exit(1)
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into function ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive CI pipeline orchestration script. The changes are well-structured and improve the robustness of the CI process, particularly with better error logging using logger.exception and more explicit subprocess handling. My review focuses on a few areas to further improve maintainability and consistency: refactoring duplicated code, using more specific exception handling, and ensuring consistent logging across all test runners. Overall, this is a great addition to the project's CI infrastructure.
- Replace logger.exception with logger.error for expected errors (TimeoutExpired, ImportError) - Add security comments clarifying no command injection risk in subprocess calls - All arguments are static literals or controlled internally Addresses Sourcery AI review feedback to improve code quality and security clarity. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci/run_full_ci_pipeline.py (1)
49-56: New ONNX script added — ensure required deps are validated.Since you now run onnx_conversion_test.py, add onnx and runtime to required packages or skip the test when missing.
Apply this diff:
required_packages = [ "torch", "transformers", "fastapi", "pydantic", "datasets", "tokenizers", "numpy", - "pandas", + "pandas", + "onnx", + "onnxruntime", ]
🧹 Nitpick comments (6)
scripts/ci/run_full_ci_pipeline.py (6)
20-20: Import Optional for the updated type hint.Apply this diff:
-from typing import Dict, Tuple +from typing import Dict, Tuple, Optional
35-38: Harden file logging for CI environments (encoding, delayed open).Avoid encoding issues and file creation failures on ephemeral runners.
Apply this diff:
- logging.StreamHandler(sys.stdout), - logging.FileHandler("ci_pipeline.log"), + logging.StreamHandler(sys.stdout), + logging.FileHandler("ci_pipeline.log", encoding="utf-8", delay=True),
58-59: Unify typing style (Dict/Tuple vs dict/tuple).File mixes typing and builtin generics. Prefer one style consistently (e.g., builtin: dict[str, bool], tuple[...]).
160-162: Also log stdout on script failure for better diagnostics.Apply this diff:
- logger.error(f"❌ {script_path} FAILED") - logger.error(f"Error output: {result.stderr}") + logger.error(f"❌ {script_path} FAILED") + logger.error(f"Error output: {result.stderr}") + logger.error(f"Standard output: {result.stdout}") return False, result.stderr
242-256: Deduplicate sys.path tweaking and BERT import logic.Extract to a helper or perform once at module scope to remove repetition and reduce import flakiness.
Proposed helper (place near top of file):
def _add_src_to_path() -> None: sys.path.insert(0, str(Path(__file__).parent.parent.parent / "src")) def _import_bert_classifier(): try: from models.emotion_detection.bert_classifier import BERTEmotionClassifier except ImportError: from src.models.emotion_detection.bert_classifier import BERTEmotionClassifier return BERTEmotionClassifierThen in both call sites:
_add_src_to_path() BERTEmotionClassifier = _import_bert_classifier()Also applies to: 286-298
299-306: Set model to eval() for inference timing consistency.Apply this diff:
- model = BERTEmotionClassifier() + model = BERTEmotionClassifier() + model.eval()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/ci/run_full_ci_pipeline.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/ci/run_full_ci_pipeline.py (2)
src/models/emotion_detection/bert_classifier.py (1)
BERTEmotionClassifier(31-277)tests/unit/test_secure_model_loader.py (1)
BERTEmotionClassifier(39-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
scripts/ci/run_full_ci_pipeline.py (5)
147-149: Static argv to subprocess — good mitigation of injection risk.
185-193: Unit test runner control flow and logging look correct.
215-221: E2E failures should log stdout and return code (parity with unit tests).Apply this diff:
- logger.error("❌ E2E tests FAILED") - logger.error(f"Error output: {result.stderr}") + logger.error("❌ E2E tests FAILED") + logger.error(f"Return code: {result.returncode}") + logger.error(f"Error output: {result.stderr}") + logger.error(f"Standard output: {result.stdout}") return False
222-224: Handle E2E timeouts explicitly to avoid noisy stack traces.Apply this diff:
- except Exception as e: - logger.exception(f"💥 E2E tests ERROR: {e}") - return False + except subprocess.TimeoutExpired: + logger.error("⏰ E2E tests TIMEOUT") + return False + except Exception as e: + logger.exception(f"💥 E2E tests ERROR: {e}") + return False
23-29: Catch ImportError only and avoid 3.10-only type syntax in fallback.Use a narrow exception and Optional to keep 3.9 compatibility. Also see next comment to import Optional.
Apply this diff:
-try: +try: from src.common.env import is_truthy -except Exception: # Fallback to local helper if import path not available +except ImportError: # Fallback to local helper if import path not available - def is_truthy(value: str | None) -> bool: + def is_truthy(value: Optional[str]) -> bool: return bool(value) and value.strip().lower() in {"1", "true", "yes"}
… logging consistency - Change broad Exception to specific ImportError for is_truthy import - Move BERT classifier import logic to module level to eliminate duplication - Add stdout logging to run_ci_script and run_e2e_tests methods for consistency - Remove duplicated _import_bert_classifier method and sys.path.insert calls
- Detect when no boolean tests are executed (total_tests == 0) - Set success_rate = 0.0 explicitly for zero-test scenarios - Display explicit failure message: 'No boolean tests were executed. Treating pipeline as failed.' - Update exit logic to fail when no tests are executed - Prevent false 'All tests passed' reports when actually no tests ran
- Add else: clauses to run_ci_script, run_unit_tests, and run_e2e_tests methods - Make control flow more explicit and clear for developers and AI tools - Prevent any confusion about when error logging occurs (only on failure) - Maintain existing functionality while improving code readability
- Rename unused 'model' variable to '_' in _measure_model_loading_time method - Add comment explaining why model instantiation is needed (for timing measurement) - Resolve PYL-W0612 linter warning for unused variable
- Remove redundant local 'import time' statements in _measure_model_loading_time and _measure_inference_time methods - Use module-level 'time' import instead to avoid PYL-W0621 shadowing warnings - Maintain functionality while following Python best practices for imports
Resolved issues in scripts/ci/run_full_ci_pipeline.py with DeepSource Autofix
- Replace f-string logging with lazy % formatting for better performance - Convert all logger calls to use % formatting instead of f-strings - Fixes 8 occurrences of formatted string passed to logging module - Maintains all logging functionality while improving performance when logging is disabled
- Break long function signatures across multiple lines - Split long string literals in report generation - Reformat multi-line logger.error calls to stay within 88 char limit - Ensure all lines comply with configured maximum line length
- Break long logger.info call in GPU forward pass test to stay within 88 char limit - Ensure all lines comply with maximum line length configuration
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
scripts/ci/run_full_ci_pipeline.py (5)
20-20: Import missing typing symbols used in annotations.Add Optional and Any for the suggested type fixes.
-from typing import Dict, Tuple +from typing import Any, Dict, Optional, Tuple
106-106: Correct return type: detect_environment returns mixed types, not Dict[str, str].Use Any to reflect actual values (str, bool, int).
- def detect_environment(self) -> Dict[str, str]: + def detect_environment(self) -> Dict[str, Any]:
423-423: Correct return type: run_full_pipeline includes dicts in results.Return type should allow non-bool entries like the environment dict.
- def run_full_pipeline(self) -> Dict[str, bool]: + def run_full_pipeline(self) -> Dict[str, Any]:
189-193: Log return code on script failures (parity with unit test logging).Helps triage failures faster.
else: logger.error(f"❌ {script_path} FAILED") + logger.error(f"Return code: {result.returncode}") logger.error(f"Error output: {result.stderr}") logger.error(f"Standard output: {result.stdout}") return False, result.stderr
45-46: Set log file encoding to avoid Unicode issues.Prevents encoding errors on non-UTF-8 systems.
- logging.FileHandler("ci_pipeline.log"), + logging.FileHandler("ci_pipeline.log", encoding="utf-8"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/ci/run_full_ci_pipeline.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/ci/run_full_ci_pipeline.py (3)
src/models/emotion_detection/bert_classifier.py (1)
BERTEmotionClassifier(31-277)tests/unit/test_secure_model_loader.py (1)
BERTEmotionClassifier(39-48)scripts/ci/bert_model_test.py (1)
main(85-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
scripts/ci/run_full_ci_pipeline.py (2)
170-193: Verified CI scripts propagate failures via exit codes. All scripts invoked by run_full_ci_pipeline.py call sys.exit() with a non-zero status on failure.
285-296: Gate heavy GPU model load and add a fast CUDA smoke test to reduce CI flakiness.Loading HF models on GPU without a guard/timeout is fragile in CI. Add a quick matmul smoke test and gate the heavy load behind FULL_GPU_TEST.
- import torch - - if not torch.cuda.is_available(): - logger.info("ℹ️ No GPU available, skipping GPU tests") - return True - - logger.info(f"🎮 GPU detected: {torch.cuda.get_device_name(0)}") - - # Test GPU model loading and forward pass - return self._test_gpu_model_forward_pass() + import torch, time as _time + + if not torch.cuda.is_available(): + logger.info("ℹ️ No GPU available, skipping GPU tests") + return True + + logger.info(f"🎮 GPU detected: {torch.cuda.get_device_name(0)}") + + # Fast CUDA smoke test + try: + t0 = _time.time() + x = torch.randn(1024, 1024, device="cuda") + _ = x @ x + torch.cuda.synchronize() + logger.info("✅ Basic CUDA matmul successful (%.2fs)", _time.time() - t0) + except Exception as smoke_e: + logger.exception("❌ Basic CUDA operation failed: %s", smoke_e) + return False + + # Optionally run heavy model loading/forward pass + if not is_truthy(os.environ.get("FULL_GPU_TEST")): + logger.info("ℹ️ Skipping heavy GPU model load (set FULL_GPU_TEST=1 to enable)") + return True + + return self._test_gpu_model_forward_pass()
- Replace PEP 604 union syntax (str | None) with Optional[str] for Python 3.9 compatibility - Add Optional import from typing module - Maintain function behavior unchanged
Owner
Author
|
@sourcery-ai dismiss |
d-ulker
added a commit
that referenced
this pull request
Sep 26, 2025
* feat: add comprehensive CI pipeline orchestration system Extracted from monster PR #171 as part of Phase 3A strategic decomposition. Comprehensive CI pipeline runner with: - Multi-environment detection (local/Colab/CI) - GPU compatibility testing with PyTorch - Dependency validation for all required packages - Individual CI script orchestration with timeouts - Unit and E2E test execution (pytest integration) - Performance benchmarking with thresholds - Detailed reporting with success metrics - CI artifact generation (conditional report writing) - Graceful error handling and logging Supports both local development and production CI workflows. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: address code review feedback for CI pipeline orchestration - Replace logger.exception with logger.error for expected errors (TimeoutExpired, ImportError) - Add security comments clarifying no command injection risk in subprocess calls - All arguments are static literals or controlled internally Addresses Sourcery AI review feedback to improve code quality and security clarity. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Address gemini-code-assist review comments: refactor imports, improve logging consistency - Change broad Exception to specific ImportError for is_truthy import - Move BERT classifier import logic to module level to eliminate duplication - Add stdout logging to run_ci_script and run_e2e_tests methods for consistency - Remove duplicated _import_bert_classifier method and sys.path.insert calls * Fix zero-test case reporting logic in CI pipeline - Detect when no boolean tests are executed (total_tests == 0) - Set success_rate = 0.0 explicitly for zero-test scenarios - Display explicit failure message: 'No boolean tests were executed. Treating pipeline as failed.' - Update exit logic to fail when no tests are executed - Prevent false 'All tests passed' reports when actually no tests ran * Add explicit else clauses for test result handling - Add else: clauses to run_ci_script, run_unit_tests, and run_e2e_tests methods - Make control flow more explicit and clear for developers and AI tools - Prevent any confusion about when error logging occurs (only on failure) - Maintain existing functionality while improving code readability * Fix unused variable warning in CI pipeline - Rename unused 'model' variable to '_' in _measure_model_loading_time method - Add comment explaining why model instantiation is needed (for timing measurement) - Resolve PYL-W0612 linter warning for unused variable * Fix variable shadowing warnings for 'time' import - Remove redundant local 'import time' statements in _measure_model_loading_time and _measure_inference_time methods - Use module-level 'time' import instead to avoid PYL-W0621 shadowing warnings - Maintain functionality while following Python best practices for imports * feat: add comprehensive CI pipeline orchestration system Resolved issues in scripts/ci/run_full_ci_pipeline.py with DeepSource Autofix * Fix PYL-W1203 logging format warnings - Replace f-string logging with lazy % formatting for better performance - Convert all logger calls to use % formatting instead of f-strings - Fixes 8 occurrences of formatted string passed to logging module - Maintains all logging functionality while improving performance when logging is disabled * Fix line length violations (FLK-E501) - Break long function signatures across multiple lines - Split long string literals in report generation - Reformat multi-line logger.error calls to stay within 88 char limit - Ensure all lines comply with configured maximum line length * Fix remaining line length violation (FLK-E501) - Break long logger.info call in GPU forward pass test to stay within 88 char limit - Ensure all lines comply with maximum line length configuration * Fix Python 3.9 compatibility in is_truthy function - Replace PEP 604 union syntax (str | None) with Optional[str] for Python 3.9 compatibility - Add Optional import from typing module - Maintain function behavior unchanged --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Strategic extraction from monster PR #171 as part of Phase 3A focused decomposition.
Adds a comprehensive CI pipeline orchestration system that handles end-to-end testing workflows:
Key Features
src.and direct module importsis_truthy()helper for environment flag parsingPipeline Phases
Test Plan
🏰 Fortress-Protected Development: Single-purpose micro-PR (1 file, CI orchestration)
🤖 Generated with Claude Code
Summary by Sourcery
Add a comprehensive CI pipeline orchestration system that automates environment detection, dependency checks, test execution, performance benchmarking, and report generation across local, Colab, and CI environments.
New Features:
Enhancements:
CI:
Summary by CodeRabbit