-
-
Notifications
You must be signed in to change notification settings - Fork 52
FIX: Cortex doctor - resolve dependency detection + add polished UI #333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughReplaced the Panel header with a CX-branded banner, wrapped all doctor check groups in a single console.status spinner, and improved dependency-name normalization (trim, lowercase, hyphen→underscore, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cortex/doctor.py (1)
61-72: Consider centralizing banner in branding module.The ASCII art banner is hardcoded here, while other branding elements like
cx_headerare incortex/branding.py. For consistency and maintainability, consider moving this banner to the branding module.💡 Example approach
In
cortex/branding.py, add:def show_health_check_banner(): """Display the SYSTEM HEALTH CHECK banner.""" console.print("[bold cyan] ██████╗██╗ ██╗ SYSTEM HEALTH CHECK[/bold cyan]") console.print("[bold cyan] ██╔════╝╚██╗██╔╝ ─────────────────────[/bold cyan]") # ... rest of banner lines console.print()Then in this file:
from cortex.branding import console, cx_header, show_health_check_banner # ... def run_checks(self) -> int: console.print() show_health_check_banner() console.print() # ...
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/doctor.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/doctor.py
🧬 Code graph analysis (1)
cortex/doctor.py (2)
cortex/cli.py (1)
status(704-735)cortex/branding.py (1)
cx_header(82-88)
🔇 Additional comments (3)
cortex/doctor.py (3)
74-93: Nice UX improvement with the spinner!The spinner-wrapped execution flow provides good visual feedback during checks. Rich's
console.status()correctly handles the mixed output from check results while the spinner is active.
102-104: Good refactoring to use centralized branding.Delegating to
cx_headerimproves consistency with other parts of the codebase (e.g.,cortex/cli.py).
174-174: Good addition for python-dotenv mapping.This correctly maps the
python-dotenvpackage name to itsdotenvimport name, preventing false "missing package" reports.
|
@Suyashd999 please review this and merge, I believe that CI / Lint error is from another pr files. |
|
Make sure to lint everything as well |
@Sahilbhatane the lint error is from test_parallel_llm.py which i haven't modified, its from another pr, i will lint that 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cortex/doctor.py (1)
14-14: Remove unused import.The
Statusclass is imported but never directly used. Theconsole.status()call at line 74 is a method that doesn't require explicitly importing theStatusclass.🔎 Proposed fix
-from rich.status import Status
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/doctor.pyexamples/parallel_llm_demo.py
✅ Files skipped from review due to trivial changes (1)
- examples/parallel_llm_demo.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/doctor.py
🧬 Code graph analysis (1)
cortex/doctor.py (2)
cortex/cli.py (1)
status(704-735)cortex/branding.py (1)
cx_header(82-88)
🔇 Additional comments (6)
cortex/doctor.py (6)
60-71: Great branding enhancement!The CX-styled ASCII banner with "SYSTEM HEALTH CHECK" provides a polished, branded header that significantly improves the user experience. This aligns well with the PR objectives for UI improvements.
73-92: Excellent UX improvement with status spinner.Wrapping all checks in a
console.statusspinner provides clear feedback to users during the health check process. The logical grouping of checks (Python & Dependencies, GPU & Acceleration, AI & Services, System Resources) improves readability and organization.
101-103: Good refactoring for consistent branding.Delegating to
cx_headercentralizes the header styling logic and ensures consistent CX branding across all section headers. The updated docstring accurately reflects this change.
173-173: Excellent fix for dependency detection!The addition of the
python-dotenv→dotenvmapping and the complete case normalization in the fallback correctly address the issue raised in past review comments and resolve issue #332.The fallback at line 183 now properly applies
.lower()before replacing hyphens with underscores, ensuring packages with non-standard casing inrequirements.txt(e.g.,Requests,PyYAML) are correctly recognized.Note: This addresses the previous review comment from @coderabbitai that @Suyashd999 requested be fixed.
Also applies to: 181-184
241-241: Clearer messaging for CPU-only mode.The updated warning message better communicates that CPU-only mode is supported while setting appropriate expectations about performance. This improves user understanding of system capabilities.
327-327: LGTM - clean message formatting.The simplified API key warning message maintains clarity while improving readability.
|
@Sahilbhatane @Suyashd999 ready to merge! |
e4d9334 to
287a353
Compare
|




Related Issue
Closes #332
Summary
PyYAML→yaml,python-dotenv→dotenv...raw_name.replace("-", "_")fallback for hyphensNew Features:
cx_header()for all sectionsdemo
Screen.Recording.2025-12-21.234242.mp4
Checklist
pytest tests/test_cortex_doctor)[#XX] DescriptionSummary by CodeRabbit
New Features
Bug Fixes
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.