Hybrid GPU Manager: battery impact estimation and GPU state detection v1 #517
Conversation
|
Warning Rate limit exceeded@tuanwannafly has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 46 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)
📝 WalkthroughWalkthroughAdds a Hybrid GPU Manager: hardware GPU detection and battery estimation, per‑app GPU preferences and env injection, GPU mode‑switch planning/execution with backend detection, CLI commands ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CortexCLI
participant HD as hardware_detection
participant GM as gpu_manager
participant Sys as System
participant Config as Config(~/.cortex/config.yaml)
User->>CLI: run `cortex gpu status` / `cortex gpu-battery` / `cortex gpu run`
CLI->>HD: detect_gpu_mode() or estimate_gpu_battery_impact()
HD->>Sys: run lspci / nvidia-smi / read battery files / PowerShell
Sys-->>HD: sensor & battery outputs
HD-->>CLI: mode / estimates
CLI->>GM: detect_gpu_switch_backend() / plan_gpu_mode_switch(target)
GM->>Sys: probe for prime-select / system76-power
GM->>Config: read/write per-app prefs
GM-->>CLI: plan (commands, backend, restart) / env for run
CLI->>Sys: run_command_with_env(cmd, extra_env) or apply_gpu_mode_switch(plan, execute?)
Sys-->>CLI: command exit status
CLI->>User: print status / results / exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @cortex/cli.py:
- Around line 1951-1953: A blank line after the elif args.command ==
"gpu-battery": return cli.gpu_battery() block contains trailing whitespace which
triggers the linter; remove the trailing spaces so the line is truly empty (or
collapse the blank line entirely) to eliminate the whitespace character and
satisfy the pipeline, locating the spot by the args.command == "gpu-battery"
check and the cli.gpu_battery() call.
In @cortex/hardware_detection.py:
- Around line 756-777: Remove the trailing whitespace on the blank line
following the _run function definition: open the file, locate the _run function
(the try/except block that returns "" on exceptions) and delete any spaces/tabs
at the end of the blank line after the except block so the line is truly empty
(ensure the file ends with a single newline and no trailing whitespace).
- Around line 813-847: The file is missing a trailing newline which causes
lint/pipeline failures; add a final newline character at the end of
cortex/hardware_detection.py so the file ends with a newline. Also (optional)
expand the estimate_gpu_battery_impact docstring to describe the return dict
structure (keys "mode", "current", "estimates"), the heuristic basis for the
power/impact values, and caveats that these are non-root, non-realtime estimates
to improve maintainability and clarity.
In @tests/test_hybrid_gpu_manager.py:
- Around line 39-41: The file is missing a trailing newline which breaks
linters; open the test file containing
test_detect_gpu_mode_does_not_crash_and_returns_value (which calls
detect_gpu_mode()) and add a single newline character at the end of the file so
the file ends with a newline (ensure the editor saves the file with a trailing
newline).
🧹 Nitpick comments (2)
cortex/cli.py (1)
935-958: Consider adding error handling for robustness.The method assumes
estimate_gpu_battery_impact()always returns a well-formed dictionary. If the underlying implementation changes or fails, this could raiseKeyErrororAttributeError. Consider wrapping the call in a try-except block to handle potential errors gracefully.🔎 Proposed error handling
def gpu_battery(self) -> int: """Estimate battery impact based on GPU usage""" + try: data = estimate_gpu_battery_impact() cx_print(f"GPU Mode: {data['mode']}", "info") print() cx_print("Estimated power draw:", "info") print(f"- Integrated GPU only: {data['estimates']['integrated']['power']}") print(f"- Hybrid (idle dGPU): {data['estimates']['hybrid_idle']['power']}") print(f"- NVIDIA active: {data['estimates']['nvidia_active']['power']}") print() cx_print("Estimated battery impact:", "info") print(f"- Hybrid idle: {data['estimates']['hybrid_idle']['impact']}") print(f"- NVIDIA active: {data['estimates']['nvidia_active']['impact']}") print() cx_print( "Note: Estimates are heuristic and vary by hardware and workload.", "warning", ) return 0 + except (KeyError, TypeError) as e: + self._print_error(f"Failed to estimate GPU battery impact: {e}") + return 1cortex/hardware_detection.py (1)
800-810: Consider enhancing the docstring.The function logic is sound for best-effort GPU mode detection. However, per the coding guidelines requiring docstrings for all public APIs, consider expanding the docstring to document:
- Return values and their meanings
- Detection methodology
- Limitations/caveats
🔎 Proposed docstring enhancement
def detect_gpu_mode() -> str: """ - Best-effort GPU mode detection + Detect the current GPU mode on the system. + + Returns one of: + - "Integrated": Only integrated GPU detected or no PCI devices found + - "NVIDIA": NVIDIA discrete GPU detected and available + - "Hybrid": PCI devices found but no NVIDIA GPU active (may be AMD or powered-down NVIDIA) + + Note: This is a best-effort heuristic and may not reflect the actual hardware + configuration in all cases (e.g., if NVIDIA GPU is present but powered down). + + Returns: + str: One of "Integrated", "NVIDIA", or "Hybrid" """
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pycortex/hardware_detection.pytests/test_hybrid_gpu_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/hardware_detection.pycortex/cli.pytests/test_hybrid_gpu_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_hybrid_gpu_manager.py
🧬 Code graph analysis (3)
cortex/hardware_detection.py (1)
cortex/first_run_wizard.py (1)
run(169-228)
cortex/cli.py (2)
cortex/hardware_detection.py (1)
estimate_gpu_battery_impact(813-847)cortex/branding.py (1)
cx_print(49-69)
tests/test_hybrid_gpu_manager.py (1)
cortex/hardware_detection.py (3)
detect_gpu_mode(800-810)detect_nvidia_gpu(780-797)estimate_gpu_battery_impact(813-847)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1953-1953: Ruff check failed. W293 Blank line contains whitespace. cortex/cli.py:1953
🪛 GitHub Check: lint
cortex/hardware_detection.py
[failure] 847-847: Ruff (W292)
cortex/hardware_detection.py:847:6: W292 No newline at end of file
[failure] 777-777: Ruff (W293)
cortex/hardware_detection.py:777:1: W293 Blank line contains whitespace
cortex/cli.py
[failure] 1953-1953: Ruff (W293)
cortex/cli.py:1953:1: W293 Blank line contains whitespace
tests/test_hybrid_gpu_manager.py
[failure] 41-41: Ruff (W292)
tests/test_hybrid_gpu_manager.py:41:28: W292 No newline at end of file
🪛 GitHub Check: Lint
cortex/hardware_detection.py
[failure] 847-847: Ruff (W292)
cortex/hardware_detection.py:847:6: W292 No newline at end of file
[failure] 777-777: Ruff (W293)
cortex/hardware_detection.py:777:1: W293 Blank line contains whitespace
cortex/cli.py
[failure] 1953-1953: Ruff (W293)
cortex/cli.py:1953:1: W293 Blank line contains whitespace
tests/test_hybrid_gpu_manager.py
[failure] 41-41: Ruff (W292)
tests/test_hybrid_gpu_manager.py:41:28: W292 No newline at end of file
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (4)
tests/test_hybrid_gpu_manager.py (3)
8-11: LGTM!The test properly validates that
detect_gpu_mode()returns a valid state string.
14-31: LGTM!The test comprehensively validates the structure of the battery impact estimation data, ensuring the contract is met.
34-36: LGTM!The test validates that
detect_nvidia_gpu()safely returns a boolean value.cortex/hardware_detection.py (1)
780-797: LGTM!The function provides a clean, safe way to detect NVIDIA GPUs using
nvidia-smi. The docstring appropriately documents limitations and the implementation is straightforward.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @cortex/hardware_detection.py:
- Around line 800-810: The detect_gpu_mode function's docstring is missing
documentation of its possible return values; update the docstring for
detect_gpu_mode() to list and describe each return string ("Integrated" = no PCI
GPUs detected or lspci unavailable, "NVIDIA" = an NVIDIA GPU was detected via
detect_nvidia_gpu(), "Hybrid" = PCI devices present but no NVIDIA detected), and
include the return type annotation note (str) and brief explanation of detection
heuristics so callers understand the meanings.
♻️ Duplicate comments (3)
cortex/cli.py (1)
1951-1953: Fix trailing whitespace on line 1953.Line 1953 contains trailing whitespace, causing linter and pipeline failures (Ruff W293). This issue was previously flagged and remains unresolved.
🔎 Proposed fix
elif args.command == "gpu-battery": return cli.gpu_battery() - + else:cortex/hardware_detection.py (2)
757-778: Fix trailing whitespace on blank line.The
_run()helper function is well-implemented with proper error handling, comprehensive docstring, and type hints. However, the blank line after the function (line 778) contains trailing whitespace, causing linter failures (Ruff W293). This issue was previously flagged and remains unresolved.🔎 Proposed fix
return result.stdout.strip() except (subprocess.CalledProcessError, OSError): return "" - + def detect_nvidia_gpu() -> bool:
813-847: Add newline at end of file and expand docstring.The battery impact estimation logic is sound and follows the heuristic approach described in the PR objectives. However:
Critical: Line 847 is missing a trailing newline, causing linter failures (Ruff W292). This issue was previously flagged and remains unresolved.
Essential: Per coding guidelines and previous review feedback, the docstring should document the return value structure, estimation methodology, and caveats.
🔎 Proposed fixes
Fix missing newline:
return { "mode": mode, "current": current, "estimates": estimates, } +Expand docstring:
def estimate_gpu_battery_impact() -> dict[str, Any]: """ - Heuristic battery impact estimation based on GPU mode. - No realtime measurement, safe for non-root usage. + Estimate battery impact of GPU usage based on detected GPU mode. + + Provides heuristic estimates of power draw and battery life impact for + different GPU configurations. Does not perform real-time power measurement + and is safe to call without root privileges. + + Returns: + dict[str, Any]: Dictionary containing: + - mode (str): Detected GPU mode ("Integrated", "NVIDIA", or "Hybrid") + - current (str): Current power profile ("integrated", "nvidia_active", or "hybrid_idle") + - estimates (dict): Power and impact estimates for each profile, with keys: + - "integrated": {"power": str, "impact": str} + - "hybrid_idle": {"power": str, "impact": str} + - "nvidia_active": {"power": str, "impact": str} + + Note: + Estimates are heuristic approximations. Actual power draw varies + significantly based on hardware, workload, and system configuration. """As per coding guidelines, docstrings are required for all public APIs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pycortex/hardware_detection.pytests/test_hybrid_gpu_manager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_hybrid_gpu_manager.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/hardware_detection.pycortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/hardware_detection.py (1)
estimate_gpu_battery_impact(813-847)cortex/branding.py (1)
cx_print(49-69)
⏰ 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). (4)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (3)
cortex/cli.py (2)
21-21: LGTM!The import is properly formatted and correctly used by the
gpu_battery()method.
1669-1673: LGTM!The subparser registration follows the established pattern and provides clear help text.
cortex/hardware_detection.py (1)
780-797: LGTM!Excellent implementation with comprehensive docstring, proper type hints, and sound detection logic. The use of
nvidia-smifor detection is appropriate and the error handling via_run()is robust.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @cortex/cli.py:
- Around line 1951-1954: Remove the extra blank line after the "return
cli.gpu_battery()" branch to satisfy Black's formatting rules: locate the elif
block handling args.command == "gpu-battery" (the line with "return
cli.gpu_battery()") and ensure there is only a single blank line following that
statement (no consecutive blank lines).
In @cortex/hardware_detection.py:
- Around line 780-801: The docstring for detect_nvidia_gpu() is incorrect: it
describes string mode values but the function returns a bool via return
bool(_run(["nvidia-smi"])). Update the docstring to document that
detect_nvidia_gpu() returns a boolean (True if an NVIDIA GPU is detectable via
nvidia-smi, False otherwise), describe the detection method
(_run(["nvidia-smi"]) invocation) and remove or replace the
"Integrated"/"NVIDIA"/"Hybrid" return value text; alternatively, if you intended
string modes, change the function implementation to call detect_gpu_mode() or
implement mode detection—ensure detect_nvidia_gpu and its return type and
docstring are consistent.
♻️ Duplicate comments (2)
cortex/cli.py (1)
935-958: Enhance docstring and add dictionary validation.The past review comment on this method remains valid:
Essential: The docstring should document the return value (0 on success), any exceptions that may propagate, and provide more detail about what battery impact estimation entails, per coding guidelines.
Recommended: Direct access to nested dictionary keys (e.g.,
data['estimates']['integrated']['power']) assumes a specific structure. While the main() exception handler provides a safety net, consider adding validation or a try/except block to handle unexpected dictionary structures more gracefully.As per coding guidelines, docstrings are required for all public APIs.
cortex/hardware_detection.py (1)
804-814: Expand docstring to document return values.The past review comment on this function remains valid: The docstring should document the possible return values ("Integrated", "NVIDIA", "Hybrid") and their meanings per coding guidelines.
As per coding guidelines, docstrings are required for all public APIs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/hardware_detection.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/cli.pycortex/hardware_detection.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/hardware_detection.py (1)
estimate_gpu_battery_impact(817-868)cortex/branding.py (1)
cx_print(49-69)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted, 124 files would be left unchanged. Run 'black --write' to format the code.
⏰ 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). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (4)
cortex/cli.py (2)
21-21: LGTM!The import is correctly placed and follows the existing import organization pattern.
1669-1673: LGTM!The subparser definition follows the existing pattern and provides clear help text.
cortex/hardware_detection.py (2)
757-777: LGTM!The helper function is well-implemented with proper error handling, comprehensive docstring, and appropriate type hints. The empty string fallback on failure is a safe default for the detection use cases.
817-868: LGTM!The function is well-implemented with a comprehensive docstring that documents the return value structure, estimation methodology, and appropriate caveats. The heuristic power estimates are reasonable and the logic is clear.
|
Hi @Anshgrover23 , I’ve addressed all CodeRabbit bot comments and pushed the final fixes. |
|
@Anshgrover23 hey man still looking for your review |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@tuanwannafly Documentation is missing. Also your PR covers battery impact estimation and basic GPU detection, but per-app GPU assignment and actual GPU mode switching are still missing, so it doesn’t fully address the Hybrid GPU Manager scope yet.
There was a problem hiding this comment.
Need to add more tests.
There was a problem hiding this comment.
thanks
I'll update soon
CLA Verification PassedAll contributors have signed the CLA.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/hardware_detection.py:
- Around line 817-864: The docstring for estimate_gpu_battery_impact is
incorrect (it describes a boolean NVIDIA detection and return type) — update it
to accurately describe what estimate_gpu_battery_impact does, its return type,
and the dictionary shape it returns: mention it calls detect_gpu_mode() and
detect_nvidia_gpu(), returns dict[str, Any] with keys "mode" (e.g.,
"Integrated"/other), "current" (one of "integrated", "hybrid_idle",
"nvidia_active"), and "estimates" (mapping of those keys to power/impact
strings); also fix the Returns section to show a dict with those keys and adjust
the Note/description accordingly.
🧹 Nitpick comments (1)
cortex/hardware_detection.py (1)
804-814: Expand docstring to document return values.The implementation is correct. Per coding guidelines, public API docstrings should document return values.
🔎 Proposed docstring enhancement
def detect_gpu_mode() -> str: """ - Best-effort GPU mode detection + Detect the current GPU mode on hybrid graphics systems. + + Returns: + str: One of: + - "Integrated": lspci unavailable or no GPU devices detected + - "NVIDIA": NVIDIA discrete GPU detected via nvidia-smi + - "Hybrid": GPU devices present but NVIDIA not active """ if not _run(["lspci"]): return "Integrated"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/hardware_detection.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/cli.pycortex/hardware_detection.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/hardware_detection.py (1)
estimate_gpu_battery_impact(817-864)cortex/branding.py (1)
cx_print(49-69)
⏰ 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). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (4)
cortex/cli.py (3)
21-21: LGTM!Import is correctly placed alongside other
cortex.hardware_detectionmodule imports.
1770-1774: LGTM!Subparser registration follows the established pattern for simple commands without additional arguments.
2061-2063: LGTM!Command dispatch correctly routes to the new
gpu_battery()method, following the established pattern.cortex/hardware_detection.py (1)
757-777: LGTM!Well-implemented helper with proper error handling. The function gracefully returns an empty string on failure, allowing callers to use truthiness checks.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 1174-1178: Wrap the call to set_app_gpu_preference in a try/except
that catches ValueError, print a user-friendly error via cx_print including the
exception message, and return a non-zero exit code (e.g., return 1) so the CLI
fails gracefully; apply this handling where args.gpu_command == "app" and
args.app_action == "set" around the set_app_gpu_preference(args.app, args.mode)
invocation.
- Around line 1143-1148: The interactive prompt uses input() without catching
EOFError or KeyboardInterrupt, so when stdin is closed or the user hits Ctrl+C
the command will crash; wrap the input() call in a try/except that catches
EOFError and KeyboardInterrupt around the resp = input().strip().lower() logic
(in the same block that checks getattr(args, "yes", False)), treat these
exceptions as a non-affirmative response (e.g., set resp = "" or return 0) and
preserve the existing flow that aborts when resp is not "y"/"yes"; update the
code paths referenced (args, console.print, resp) accordingly to ensure a clean
exit instead of an uncaught exception.
In @cortex/hardware_detection.py:
- Around line 824-845: The docstring for detect_nvidia_gpu is incorrect: it
describes returning "Integrated"/"NVIDIA"/"Hybrid" strings but the function
actually returns a bool; update the docstring to accurately state that
detect_nvidia_gpu() performs a best-effort NVIDIA presence check (using
_run(["nvidia-smi"])) and returns True if an NVIDIA GPU was detected (i.e.,
nvidia-smi succeeded) and False otherwise, and remove or replace the paragraph
that lists string return values so the Return section and Notes match the bool
behavior.
In @docs/HYBRID_GPU_MANAGER.md:
- Around line 281-380: Add a starting fenced code block (```bash) before the
script that begins with set -euo pipefail so the entire shell snippet is
rendered as code, and remove the unnecessary backslashes escaping underscores in
the variable names (replace \_\_NV_PRIME_RENDER_OFFLOAD and
\_\_GLX_VENDOR_LIBRARY_NAME with __NV_PRIME_RENDER_OFFLOAD and
__GLX_VENDOR_LIBRARY_NAME) in the cortex gpu run echo lines and any other
occurrences so the bash variables render correctly.
🧹 Nitpick comments (5)
cortex/gpu_manager.py (3)
116-120: Consider cleanup of temp file on failure.If
os.replacefails (e.g., permission error), the temp file remains orphaned. This is a minor edge case but could leave stale files in the config directory.♻️ Suggested improvement
path = _config_path() - with tempfile.NamedTemporaryFile("w", delete=False, dir=str(cfg_dir), encoding="utf-8") as tf: - tf.write(serialized) - tmp = Path(tf.name) - os.replace(tmp, path) + tmp: Path | None = None + try: + with tempfile.NamedTemporaryFile("w", delete=False, dir=str(cfg_dir), encoding="utf-8") as tf: + tf.write(serialized) + tmp = Path(tf.name) + os.replace(tmp, path) + except OSError: + if tmp and tmp.exists(): + tmp.unlink(missing_ok=True) + raise
282-298: Consider adding a timeout to prevent indefinite hangs.
subprocess.runwithout a timeout could block indefinitely if the command hangs (e.g., waiting for user input from sudo without a TTY).♻️ Suggested improvement
- completed = subprocess.run(cmd, env=env) + completed = subprocess.run(cmd, env=env, timeout=60)You may want to catch
subprocess.TimeoutExpiredand return a non-zero code or raise.
320-325: Add docstring for public function.Per coding guidelines, docstrings are required for all public APIs. This function lacks documentation explaining its purpose and the expected
modeparameter values.📝 Suggested improvement
def get_gpu_profile(mode: str) -> dict[str, bool]: + """Return a profile dict indicating which GPU mode is active. + + Args: + mode: One of "Integrated", "NVIDIA", or "Hybrid". + + Returns: + Dict with keys 'use_integrated', 'use_nvidia', 'hybrid' set to True/False. + """ return { "use_integrated": mode == "Integrated", "use_nvidia": mode == "NVIDIA", "hybrid": mode == "Hybrid", }cortex/hardware_detection.py (2)
759-778: Consider adding a timeout to prevent hangs.The
_runhelper doesn't specify a timeout, which could cause the process to hang indefinitely if the command blocks.♻️ Suggested improvement
try: result = subprocess.run( cmd, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, text=True, check=False, + timeout=5, ) except OSError: return "" + except subprocess.TimeoutExpired: + return ""
881-882: Minor inefficiency:detect_nvidia_gpu()called twice.
detect_nvidia_gpu()is called both insidedetect_gpu_mode()(line 855) and directly on line 882. This results in twonvidia-smiinvocations. Consider refactoring to avoid the redundant call.♻️ Suggested optimization
- mode = detect_gpu_mode() - nvidia_active = detect_nvidia_gpu() + nvidia_active = detect_nvidia_gpu() + if not _run(["lspci"]): + mode = "Integrated" + elif nvidia_active: + mode = "NVIDIA" + else: + mode = "Hybrid"Alternatively, refactor
detect_gpu_modeto accept or return both values.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/cli.pycortex/gpu_manager.pycortex/hardware_detection.pydocs/HYBRID_GPU_MANAGER.mdtests/test_gpu_manager.pytests/test_hybrid_gpu_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
tests/test_gpu_manager.pytests/test_hybrid_gpu_manager.pycortex/hardware_detection.pycortex/cli.pycortex/gpu_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_gpu_manager.pytests/test_hybrid_gpu_manager.py
🧬 Code graph analysis (2)
tests/test_gpu_manager.py (1)
cortex/gpu_manager.py (9)
detect_gpu_switch_backend(220-231)GPUSwitchBackend(31-34)plan_gpu_mode_switch(234-279)run_command_with_env(282-298)set_app_gpu_preference(134-150)get_app_gpu_preference(170-180)list_app_gpu_preferences(183-190)get_per_app_gpu_env(193-217)remove_app_gpu_preference(153-167)
tests/test_hybrid_gpu_manager.py (1)
cortex/hardware_detection.py (3)
detect_gpu_mode(848-858)detect_nvidia_gpu(824-845)estimate_gpu_battery_impact(861-939)
🪛 markdownlint-cli2 (0.18.1)
docs/HYBRID_GPU_MANAGER.md
354-354: No space after hash on atx style heading
(MD018, no-missing-space-atx)
361-361: No space after hash on atx style heading
(MD018, no-missing-space-atx)
⏰ 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: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (16)
cortex/gpu_manager.py (5)
1-28: LGTM!Module docstring, imports, and
PRIME_OFFLOAD_ENVconstant are well-structured and appropriately typed.
31-51: LGTM!Enums and dataclass are well-designed. Using
strinheritance for enums enables clean serialization, and the frozen dataclass ensures immutability of switch plans.
134-191: LGTM!Per-app preference APIs have proper input validation, docstrings, and type hints. The config read-modify-write cycle is correctly implemented.
193-218: LGTM!The function correctly handles both explicit
use_nvidiaand app-based lookup, and returns a copy to prevent mutation of the module constant.
220-280: LGTM!Backend detection and plan construction are well-implemented with proper validation, docstrings, and correct command mappings for both
prime-selectandsystem76-power.cortex/cli.py (4)
22-34: LGTM!Imports are well-organized and all are used by the new GPU management methods.
1028-1116: LGTM!The
gpu_batterymethod handles both heuristic and measured data gracefully with appropriate fallbacks. The defensive float conversions and distinct return codes (0 success, 2 no data) are good practices.
2312-2354: LGTM!CLI subparser structure is well-organized with appropriate choices, flags, and required subcommands. The use of
argparse.REMAINDERfor the run command correctly captures arbitrary command arguments.
2756-2759: LGTM!Command routing for
gpu-batteryandgpuis correctly wired to the respective CLI methods.docs/HYBRID_GPU_MANAGER.md (1)
1-80: LGTM!The documentation provides clear overview, feature table, and quick start examples that align with the implemented CLI commands and API.
tests/test_hybrid_gpu_manager.py (1)
1-82: LGTM!Tests provide good coverage for GPU mode detection and battery impact estimation. The monkeypatched scenarios effectively test the three code paths (Integrated, NVIDIA, Hybrid) without requiring actual hardware.
tests/test_gpu_manager.py (1)
1-115: LGTM!Comprehensive test coverage for GPU manager functionality including backend detection, plan generation, command execution, and per-app configuration. Good use of monkeypatching and
tmp_pathfixture for isolation.cortex/hardware_detection.py (4)
781-821: LGTM!Robust implementation that handles multiple GPUs, non-zero exit codes, N/A values, and locale-specific decimal separators. Good defensive parsing.
848-858: LGTM!Simple and effective GPU mode detection logic with clear fallback behavior.
942-999: LGTM!Battery metrics implementation correctly handles the various sysfs file formats (
power_nowvscurrent_now+voltage_now) with proper unit conversions from micro-units to Watts.
1002-1054: LGTM!Windows/WSL battery metrics implementation handles both environments correctly with appropriate timeouts and graceful fallbacks.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Line 1118: The gpu method lacks a type annotation for its args parameter;
update the signature of gpu to include an explicit type (e.g., change def
gpu(self, args) -> int to def gpu(self, args: argparse.Namespace) -> int) and
add or ensure import argparse is present at the top of the file; if this project
prefers a different type (e.g., click.Context or a specific Args dataclass),
annotate args with that concrete type instead and import the corresponding
symbol.
- Around line 1161-1177: The GPU flag handling for args.gpu_command == "run"
allows conflicting flags (--nvidia, --integrated, --app); enforce mutual
exclusion either at parse time by using argparse.add_mutually_exclusive_group()
when defining the --nvidia, --integrated and --app arguments, or add a runtime
guard before calling get_per_app_gpu_env that counts how many of getattr(args,
"nvidia", False), getattr(args, "integrated", False), and getattr(args, "app",
None) are set and returns an error (cx_print + return 2) if more than one is
present, otherwise proceed to call get_per_app_gpu_env with the single selected
flag.
In @cortex/hardware_detection.py:
- Around line 839-849: The detect_gpu_mode docstring is too terse; update the
docstring of detect_gpu_mode() to document each return value ("NVIDIA",
"Hybrid", "Integrated") and explain what each means (e.g., "NVIDIA": NVIDIA GPU
detected; "Hybrid": non‑NVIDIA PCI GPU found via lspci, e.g., AMD dGPU + iGPU;
"Integrated": no discrete GPU detected or lspci unavailable) and add a note
about the NVIDIA‑centric/Optimus assumption and limitation that non‑NVIDIA
discrete GPUs may be misclassified as "Hybrid".
🧹 Nitpick comments (1)
cortex/cli.py (1)
1047-1051: Clarify the API contract for NVIDIA power metrics.Three different keys are checked for NVIDIA power draw (
nvidia_power_w,nvidia_power_watts,nvidia_power). This suggests API instability or unclear data contracts fromestimate_gpu_battery_impact().Consider standardizing on a single key name in the hardware detection module to avoid this defensive fallback pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/hardware_detection.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/hardware_detection.pycortex/cli.py
⏰ 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). (4)
- GitHub Check: Test (Python 3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (11)
cortex/cli.py (4)
22-33: LGTM! Clean GPU manager imports.All imported functions are used in the new
gpu_batteryandgpumethods. The multi-line import style follows PEP 8.
1103-1109: Verify exit code 2 convention for missing measurements.When
--measured-onlyis specified but no real measurements are available, the method returns exit code2. Confirm this is the intended convention for "data unavailable" scenarios.Typical exit code conventions are:
- 0: success
- 1: general error
- 2+: specific error conditions
1028-1038: Potential AttributeError if args is None.Line 1028 allows
argsto beNone, but line 1038 callsgetattr(args, "measured_only", False)without checking. If this method is ever called withargs=None, it will raiseAttributeError.🔒 Recommended fix
def gpu_battery(self, args: argparse.Namespace | None = None) -> int: """ Estimate battery impact based on current GPU usage and mode. Prints: - Heuristic estimates (unless --measured-only) - Measured battery discharge (if available) - Measured NVIDIA GPU power draw (when supported by nvidia-smi) """ data = estimate_gpu_battery_impact() - measured_only = bool(getattr(args, "measured_only", False)) + measured_only = bool(getattr(args, "measured_only", False) if args else False)Likely an incorrect or invalid review comment.
2337-2367: No issues found. The GPU mode choices in the argument parser correctly align with the modes supported by the backend functions.cortex/hardware_detection.py (7)
10-10: LGTM! Future annotations import.Adding
from __future__ import annotationsenables postponed evaluation of type hints, allowing cleaner syntax likestr | Noneinstead ofOptional[str]. This is good practice for Python 3.9+ compatibility.
759-778: LGTM! Defensive command runner.The
_runhelper properly handles missing commands and non-zero exit codes. The docstring clearly explains thecheck=Falserationale.Note: Line 771 suppresses stderr (
stderr=subprocess.DEVNULL), which is appropriate for this heuristic detection use case but means diagnostic information won't be visible if commands fail.
781-818: LGTM! Robust NVIDIA power draw detection.Excellent defensive coding:
- Handles missing
nvidia-smi(line 799)- Handles "N/A" values (line 809)
- Supports comma decimal separators for European locales (line 812)
- Sums power across multiple GPUs (line 818)
- Returns
Nonewhen unavailable
821-836: LGTM! NVIDIA detection helper.Simple and effective detection using
nvidia-smi. The docstring appropriately notes the heuristic nature and limitations.
852-930: LGTM! Comprehensive battery impact estimation.The function provides both heuristic estimates and optional real measurements with good error handling. The docstring clearly explains the capabilities and limitations.
The hard-coded string estimates (lines 875-888) like
"~6–8 W"are human-readable but not machine-parseable. This is acceptable for the current implementation, but consider adding numeric fields in future versions if programmatic access to estimates is needed.
933-990: LGTM! Robust Linux battery metrics.Excellent implementation:
- Safe file reading with None fallback (lines 948-963)
- Correct power calculation from current × voltage when
power_nowunavailable (lines 969-980)- Proper unit conversions (µW → W, µA × µV → W)
- Returns
Nonewhen battery unavailable rather than raising errors
993-1046: LGTM! Windows/WSL battery metrics support.Great cross-platform support:
- Detects Windows native vs WSL (line 1004)
- Uses modern
Get-CimInstanceinstead of deprecated WMI- 2-second timeout prevents hangs (line 1014)
- Graceful degradation on any error (returns
None)This enables the battery impact feature to work on WSL, which aligns with the PR's stated testing environment.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 1118-1207: The public method CortexCLI.gpu() lacks a docstring and
a type annotation for its args parameter; add a concise docstring describing
purpose, accepted subcommands (status/set/run/app), parameters (args:
argparse.Namespace or a more specific CLI args dataclass), and return value (int
exit code), and add a type hint to the signature (e.g., args:
argparse.Namespace) so the declaration becomes def gpu(self, args:
argparse.Namespace) -> int; ensure imports include argparse.Namespace (or the
specific type) and keep the docstring in Google/NumPy style consistent with the
project's guidelines.
In @cortex/hardware_detection.py:
- Around line 759-779: The _run() helper can hang because subprocess.run has no
timeout; update _run to pass a reasonable timeout (e.g., 5–10s) into
subprocess.run and catch subprocess.TimeoutExpired to return "" (same behavior
as OSError), and then update any direct subprocess.run calls in
get_nvidia_power_draw_watts() (and similar calls around lines 781-819) to either
call the updated _run() or include the same timeout and TimeoutExpired handling
so CLI hangs are prevented.
- Around line 842-847: The docstring for detect_gpu_mode contains trailing
whitespace on otherwise blank lines causing Ruff W293; edit the detect_gpu_mode
docstring to remove any spaces on the empty lines (ensure blank lines between
the Returns: description lines are truly empty with no spaces or tabs) so there
is no trailing whitespace on those lines.
🧹 Nitpick comments (2)
cortex/hardware_detection.py (1)
861-905:estimate_gpu_battery_impact()uses “tooling present” as “NVIDIA active”.
nvidia_active = detect_nvidia_gpu()is just “nvidia-smiproduced stdout”, not “dGPU is currently driving rendering / drawing power”. This can incorrectly labelcurrentasnvidia_activeon hybrid laptops where NVIDIA is idle but drivers are installed.Suggested direction
- nvidia_active = detect_nvidia_gpu() + # "nvidia-smi available" != "NVIDIA active". + # If we can measure power, use it as a proxy for activity; otherwise keep it heuristic. + nvidia_power_w = get_nvidia_power_draw_watts() + nvidia_active = bool(nvidia_power_w is not None and nvidia_power_w > 3.0) @@ - if mode == "Integrated": + if mode == "Integrated": current = "integrated" - elif nvidia_active: + elif nvidia_active: current = "nvidia_active" else: current = "hybrid_idle"cortex/cli.py (1)
2342-2347:--dry-runoncortex gpu setis currently redundant / unused.The command defaults to printing the plan unless
--executeis set, so--dry-rundoesn’t change behavior. Either remove it, or use it to explicitly block execution (e.g., error if both--dry-runand--executeare set).Also applies to: 1129-1160
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/hardware_detection.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/cli.pycortex/hardware_detection.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/gpu_manager.py (9)
apply_gpu_mode_switch(301-317)detect_gpu_switch_backend(220-231)get_app_gpu_preference(170-180)get_per_app_gpu_env(193-217)list_app_gpu_preferences(183-190)plan_gpu_mode_switch(234-279)remove_app_gpu_preference(153-167)run_command_with_env(282-298)set_app_gpu_preference(134-150)cortex/hardware_detection.py (2)
detect_gpu_mode(839-858)estimate_gpu_battery_impact(861-939)
🪛 GitHub Actions: CI
cortex/hardware_detection.py
[error] 842-842: ruff: W293 Blank line contains whitespace.
🪛 GitHub Check: lint
cortex/hardware_detection.py
[failure] 847-847: Ruff (W293)
cortex/hardware_detection.py:847:1: W293 Blank line contains whitespace
[failure] 842-842: Ruff (W293)
cortex/hardware_detection.py:842:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
cortex/hardware_detection.py
[failure] 847-847: Ruff (W293)
cortex/hardware_detection.py:847:1: W293 Blank line contains whitespace
[failure] 842-842: Ruff (W293)
cortex/hardware_detection.py:842:1: W293 Blank line contains whitespace
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (2)
cortex/cli.py (2)
22-33: GPU imports look consistent with the new CLI surface.No issues with the import wiring for the new
gpu/gpu-batterycommands.
2337-2340: No action required. The repository declares a minimum Python version of 3.10 (requires-python = ">=3.10"in pyproject.toml andpython_requires=">=3.10"in setup.py), which exceeds the Python 3.7+ requirement forrequired=Trueinadd_subparsers(). The code is fully compatible with the declared minimum supported version.
| def _run(cmd: list[str]) -> str: | ||
| """ | ||
| Run a system command and return stdout. | ||
| Notes: | ||
| - Uses check=False so we can still parse stdout even when returncode != 0. | ||
| (Some driver/tool combinations emit useful output but exit non-zero.) | ||
| """ | ||
| try: | ||
| result = subprocess.run( | ||
| cmd, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.DEVNULL, | ||
| text=True, | ||
| check=False, | ||
| ) | ||
| except OSError: | ||
| return "" | ||
|
|
||
| return (result.stdout or "").strip() | ||
|
|
There was a problem hiding this comment.
Add timeouts to _run() / nvidia-smi calls to avoid rare CLI hangs.
Right now _run() and get_nvidia_power_draw_watts() can block indefinitely if a command stalls (driver/tooling edge-cases). This is especially painful for CLI commands.
Proposed fix
def _run(cmd: list[str]) -> str:
@@
try:
result = subprocess.run(
cmd,
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL,
text=True,
check=False,
+ timeout=2,
)
- except OSError:
+ except (OSError, subprocess.TimeoutExpired):
return ""
@@
def get_nvidia_power_draw_watts() -> float | None:
@@
try:
proc = subprocess.run(
["nvidia-smi", "--query-gpu=power.draw", "--format=csv,noheader,nounits"],
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL,
text=True,
check=False,
+ timeout=2,
)
- except OSError:
+ except (OSError, subprocess.TimeoutExpired):
return NoneAlso applies to: 781-819
🤖 Prompt for AI Agents
In @cortex/hardware_detection.py around lines 759 - 779, The _run() helper can
hang because subprocess.run has no timeout; update _run to pass a reasonable
timeout (e.g., 5–10s) into subprocess.run and catch subprocess.TimeoutExpired to
return "" (same behavior as OSError), and then update any direct subprocess.run
calls in get_nvidia_power_draw_watts() (and similar calls around lines 781-819)
to either call the updated _run() or include the same timeout and TimeoutExpired
handling so CLI hangs are prevented.
| def detect_gpu_mode() -> str: | ||
| """ | ||
| Detect GPU mode for NVIDIA Optimus hybrid systems. | ||
| Returns: | ||
| "NVIDIA": NVIDIA GPU detected (nvidia-smi available) | ||
| "Hybrid": Non-NVIDIA GPU detected via lspci (e.g., AMD dGPU with iGPU) | ||
| "Integrated": No discrete GPU detected or lspci unavailable | ||
| Note: This detection is NVIDIA-centric and may not accurately classify | ||
| non-NVIDIA discrete GPUs. Returns "Hybrid" for any PCI GPU when NVIDIA | ||
| is not present. | ||
| """ | ||
| if not _run(["lspci"]): | ||
| return "Integrated" | ||
|
|
||
| if detect_nvidia_gpu(): | ||
| return "NVIDIA" | ||
|
|
||
| return "Hybrid" |
There was a problem hiding this comment.
detect_gpu_mode() misclassifies Intel-only systems as “Hybrid”.
On any normal Linux machine, lspci prints output, so this returns "Hybrid" whenever nvidia-smi is absent—even if there’s only an integrated GPU. That makes cortex gpu status / gpu-battery misleading.
Proposed fix (still “best-effort”, but avoids the Intel-only false positive)
def detect_gpu_mode() -> str:
@@
- if not _run(["lspci"]):
+ lspci_out = _run(["lspci", "-nn"])
+ if not lspci_out:
return "Integrated"
if detect_nvidia_gpu():
return "NVIDIA"
- return "Hybrid"
+ # Heuristic: consider it "Hybrid" only if lspci indicates multiple GPUs
+ # or a likely discrete vendor (AMD/ATI/NVIDIA) alongside an iGPU.
+ gpu_lines = [
+ ln for ln in lspci_out.splitlines()
+ if (" vga " in ln.lower()) or ("3d controller" in ln.lower()) or ("display controller" in ln.lower())
+ ]
+ if len(gpu_lines) >= 2:
+ return "Hybrid"
+ if gpu_lines and any(v in gpu_lines[0].upper() for v in ("AMD", "ATI", "NVIDIA")):
+ # Single GPU line but non-Intel vendor; keep "Hybrid" as a conservative bucket.
+ return "Hybrid"
+ return "Integrated"🧰 Tools
🪛 GitHub Actions: CI
[error] 842-842: ruff: W293 Blank line contains whitespace.
🪛 GitHub Check: lint
[failure] 847-847: Ruff (W293)
cortex/hardware_detection.py:847:1: W293 Blank line contains whitespace
[failure] 842-842: Ruff (W293)
cortex/hardware_detection.py:842:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
[failure] 847-847: Ruff (W293)
cortex/hardware_detection.py:847:1: W293 Blank line contains whitespace
[failure] 842-842: Ruff (W293)
cortex/hardware_detection.py:842:1: W293 Blank line contains whitespace
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Line 1118: The public method gpu is missing a type hint for its parameter and
a docstring; update the signature of gpu to annotate args (e.g., args:
argparse.Namespace) and add a concise docstring immediately under def gpu that
explains the method's purpose, documents the args parameter and return value
(int), and notes any raised exceptions or side effects; ensure imports include
argparse if not already present and follow existing docstring style in the
module.
🧹 Nitpick comments (3)
cortex/cli.py (3)
1038-1038: Optional: Remove redundantbool()call.The
getattr()with aFalsedefault already returns a boolean, making the outerbool()call unnecessary.♻️ Simplify the expression
- measured_only = bool(getattr(args, "measured_only", False)) + measured_only = getattr(args, "measured_only", False)
1085-1099: Consider consolidating repetitive float conversion logic.The method contains multiple similar try-except blocks for converting values to float. This pattern could be extracted into a helper function to reduce duplication and improve maintainability.
♻️ Proposed refactor with helper function
Add a helper function at the module or class level:
def _safe_format_float(value, precision: int = 2, fallback_unit: str = "") -> str: """Safely format a value as float with precision, or return as-is.""" try: return f"~{float(value):.{precision}f}{fallback_unit}" except (TypeError, ValueError): return f"~{value}{fallback_unit}"Then simplify lines 1085-1099:
if power_watts is not None: - try: - print(f"- Battery draw: ~{float(power_watts):.2f} W") - except (TypeError, ValueError): - print(f"- Battery draw: ~{power_watts} W") + print(f"- Battery draw: {_safe_format_float(power_watts, 2, ' W')}") if hours_remaining is not None: - try: - print(f"- Est. time remaining: ~{float(hours_remaining):.2f} h") - except (TypeError, ValueError): - print(f"- Est. time remaining: ~{hours_remaining} h") + print(f"- Est. time remaining: {_safe_format_float(hours_remaining, 2, ' h')}") if has_nvidia_data: - try: - print(f"- NVIDIA power draw: ~{float(nvidia_watts):.2f} W") - except (TypeError, ValueError): - print(f"- NVIDIA power draw: ~{nvidia_watts} W") + print(f"- NVIDIA power draw: {_safe_format_float(nvidia_watts, 2, ' W')}")
1130-1133: Consider error handling for GPU backend functions.The
plan_gpu_mode_switch()and other imported GPU functions could raise exceptions. Whileplan_gpu_mode_switchreturnsNoneon failure (handled on line 1131), adding broader error handling would improve robustness.🛡️ Consider adding try-except wrapper
Wrap the GPU operations in a try-except block to catch potential exceptions:
def gpu(self, args: argparse.Namespace) -> int: + try: if args.gpu_command == "status": backend = detect_gpu_switch_backend() # ... rest of logic + except Exception as e: + cx_print(f"GPU operation failed: {e}", "error") + if self.verbose: + import traceback + traceback.print_exc() + return 1Note: This is optional since individual subcommands already have some error handling (e.g., ValueError on line 1185).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/hardware_detection.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/hardware_detection.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (8)
cortex/gpu_manager.py (8)
apply_gpu_mode_switch(301-317)detect_gpu_switch_backend(220-231)get_app_gpu_preference(170-180)get_per_app_gpu_env(193-217)list_app_gpu_preferences(183-190)plan_gpu_mode_switch(234-279)remove_app_gpu_preference(153-167)run_command_with_env(282-298)cortex/hardware_detection.py (2)
detect_gpu_mode(839-858)estimate_gpu_battery_impact(861-939)cortex/kernel_features/llm_device.py (1)
getattr(81-105)cortex/graceful_degradation.py (1)
get(109-141)cortex/kernel_features/accelerator_limits.py (2)
get(69-72)status(97-105)cortex/branding.py (1)
cx_print(49-69)cortex/kernel_features/kv_cache_manager.py (1)
status(130-137)cortex/coordinator.py (1)
execute(230-279)
⏰ 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). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 1127-1128: Remove the malformed duplicated line checking
args.gpu_command (the extra "if args.gpu_command == \"status\":" with incorrect
indentation) so only a single correctly-indented conditional remains; locate the
GPU command handling block in cortex/cli.py (the if args.gpu_command == "status"
check) and delete the duplicate/incorrect line, then confirm surrounding
indentation and syntax for that conditional are correct so the module can
import.
- Around line 1118-1126: The gpu method's docstring contains trailing whitespace
on the second and fifth lines causing a PEP 8 W293 violation; edit the gpu
function docstring (def gpu(...)) to remove any trailing spaces at the ends of
lines (especially the lines starting with "Args:" and "Returns:" in that
docstring) so all lines end with no extra whitespace.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/gpu_manager.py (9)
apply_gpu_mode_switch(301-317)detect_gpu_switch_backend(220-231)get_app_gpu_preference(170-180)get_per_app_gpu_env(193-217)list_app_gpu_preferences(183-190)plan_gpu_mode_switch(234-279)remove_app_gpu_preference(153-167)run_command_with_env(282-298)set_app_gpu_preference(134-150)cortex/hardware_detection.py (2)
detect_gpu_mode(839-858)estimate_gpu_battery_impact(861-939)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1120-1120: W293 Blank line contains whitespace. (ruff check)
🪛 GitHub Actions: Cortex Automation
cortex/cli.py
[error] 1127-1127: IndentationError: unexpected indent encountered while importing cortex.cli during pytest collection (line 1127).
🪛 GitHub Check: lint
cortex/cli.py
[failure] 1127-1128: Ruff (invalid-syntax)
cortex/cli.py:1127:42: invalid-syntax: Expected an indented block after if statement
[failure] 1127-1127: Ruff (invalid-syntax)
cortex/cli.py:1127:1: invalid-syntax: Unexpected indentation
[failure] 1123-1123: Ruff (W293)
cortex/cli.py:1123:1: W293 Blank line contains whitespace
[failure] 1120-1120: Ruff (W293)
cortex/cli.py:1120:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 1127-1128: Ruff (invalid-syntax)
cortex/cli.py:1127:42: invalid-syntax: Expected an indented block after if statement
[failure] 1127-1127: Ruff (invalid-syntax)
cortex/cli.py:1127:1: invalid-syntax: Unexpected indentation
[failure] 1123-1123: Ruff (W293)
cortex/cli.py:1123:1: W293 Blank line contains whitespace
[failure] 1120-1120: Ruff (W293)
cortex/cli.py:1120:1: W293 Blank line contains whitespace
cortex/cli.py
Outdated
| if args.gpu_command == "status": | ||
| if args.gpu_command == "status": |
There was a problem hiding this comment.
Critical syntax error: Remove duplicate malformed line.
Line 1127 has incorrect indentation and duplicates line 1128, causing a syntax error that prevents the module from being imported. This blocks all tests and execution.
🐛 Fix: Remove the malformed duplicate line
- if args.gpu_command == "status":
if args.gpu_command == "status":🧰 Tools
🪛 GitHub Actions: Cortex Automation
[error] 1127-1127: IndentationError: unexpected indent encountered while importing cortex.cli during pytest collection (line 1127).
🪛 GitHub Check: lint
[failure] 1127-1128: Ruff (invalid-syntax)
cortex/cli.py:1127:42: invalid-syntax: Expected an indented block after if statement
[failure] 1127-1127: Ruff (invalid-syntax)
cortex/cli.py:1127:1: invalid-syntax: Unexpected indentation
🪛 GitHub Check: Lint
[failure] 1127-1128: Ruff (invalid-syntax)
cortex/cli.py:1127:42: invalid-syntax: Expected an indented block after if statement
[failure] 1127-1127: Ruff (invalid-syntax)
cortex/cli.py:1127:1: invalid-syntax: Unexpected indentation
🤖 Prompt for AI Agents
In @cortex/cli.py around lines 1127 - 1128, Remove the malformed duplicated line
checking args.gpu_command (the extra "if args.gpu_command == \"status\":" with
incorrect indentation) so only a single correctly-indented conditional remains;
locate the GPU command handling block in cortex/cli.py (the if args.gpu_command
== "status" check) and delete the duplicate/incorrect line, then confirm
surrounding indentation and syntax for that conditional are correct so the
module can import.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 1138-1142: plan_gpu_mode_switch can raise ValueError for an
invalid target_mode; wrap the call to plan_gpu_mode_switch(args.mode) (the block
handling args.gpu_command == "set") in a try/except that catches ValueError and
prints a friendly error via cx_print (or returns nonzero) instead of letting the
exception propagate, keeping the existing logic that returns 2 when plan is
None.
- Around line 1210-1212: Call and capture the boolean result of
remove_app_gpu_preference(args.app) instead of ignoring it; e.g. result =
remove_app_gpu_preference(args.app) then print or log a clear user-facing
message: on True print "GPU preference removed for app '<app>'" and return 0, on
False print "No GPU preference found for app '<app>'" (or "failed to remove GPU
preference for '<app>'" if failure), and return a non-zero exit code; update the
branch handling args.app_action == "remove" accordingly.
- Around line 1047-1051: The code in the NVIDIA power compatibility block (the
nvidia_watts assignment in cortex/cli.py) checks fallback keys
"nvidia_power_watts" and "nvidia_power" that are never produced by
estimate_gpu_battery_impact() (which only sets "nvidia_power_w"); remove those
two obsolete fallback gets and simplify the logic to only read
measured.get("nvidia_power_w") (or, if you want to retain explicit
compatibility, replace them with a single documented alias check that maps
legacy keys upstream), ensuring the variable name nvidia_watts is set directly
from the actual key emitted by estimate_gpu_battery_impact().
🧹 Nitpick comments (2)
cortex/cli.py (2)
1028-1028: Remove unused None default from args parameter.The
argsparameter has aNonedefault, but the calling code at line 2782 always passesargsfrom argparse. The None default is never used and adds unnecessary complexity.♻️ Simplify signature
- def gpu_battery(self, args: argparse.Namespace | None = None) -> int: + def gpu_battery(self, args: argparse.Namespace) -> int:
1085-1099: Reduce duplication in float conversion logic.The try/except pattern for converting values to float is repeated three times with identical logic. Extract this into a helper function to improve maintainability.
♻️ Extract helper function
Add a helper method to the class:
def _safe_float_format(self, value: Any, precision: int = 2) -> str: """Format a value as float with fallback to string representation.""" try: return f"~{float(value):.{precision}f} W" except (TypeError, ValueError): return f"~{value} W"Then apply this diff to use the helper:
if power_watts is not None: - try: - print(f"- Battery draw: ~{float(power_watts):.2f} W") - except (TypeError, ValueError): - print(f"- Battery draw: ~{power_watts} W") + print(f"- Battery draw: {self._safe_float_format(power_watts)}") if hours_remaining is not None: - try: - print(f"- Est. time remaining: ~{float(hours_remaining):.2f} h") - except (TypeError, ValueError): - print(f"- Est. time remaining: ~{hours_remaining} h") + # Note: hours use different unit, so handle separately + try: + print(f"- Est. time remaining: ~{float(hours_remaining):.2f} h") + except (TypeError, ValueError): + print(f"- Est. time remaining: ~{hours_remaining} h") if has_nvidia_data: - try: - print(f"- NVIDIA power draw: ~{float(nvidia_watts):.2f} W") - except (TypeError, ValueError): - print(f"- NVIDIA power draw: ~{nvidia_watts} W") + print(f"- NVIDIA power draw: {self._safe_float_format(nvidia_watts)}")Or even simpler: create a local helper that adapts for both watts and hours.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (3)
cortex/gpu_manager.py (9)
apply_gpu_mode_switch(301-317)detect_gpu_switch_backend(220-231)get_app_gpu_preference(170-180)get_per_app_gpu_env(193-217)list_app_gpu_preferences(183-190)plan_gpu_mode_switch(234-279)remove_app_gpu_preference(153-167)run_command_with_env(282-298)set_app_gpu_preference(134-150)cortex/hardware_detection.py (2)
detect_gpu_mode(839-858)estimate_gpu_battery_impact(861-939)cortex/branding.py (1)
cx_print(49-69)
⏰ 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). (6)
- GitHub Check: Test (Python 3.11)
- GitHub Check: Test (Python 3.10)
- GitHub Check: Test (Python 3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (3)
cortex/cli.py (3)
22-33: LGTM: GPU module imports are well-organized.The imports from
cortex.gpu_managerandcortex.hardware_detectionare clean, necessary, and follow the existing import patterns in the file.
2334-2379: LGTM: GPU CLI argument parsing is well-structured.The argument parsing for both
gpu-batteryandgpucommands is properly organized:
- Clear subcommand hierarchy
- Appropriate use of mutually exclusive groups for
gpu run- Mode validation via
choicesprevents invalid inputargparse.REMAINDERcorrectly captures arbitrary command arguments forgpu run
2781-2784: LGTM: Command routing is correct.The routing for
gpu-batteryandgpucommands follows the established pattern inmain()and correctly passes the parsed arguments to the respective handler methods.
…com/tuanwannafly/cortex into feature/454-hybrid-gpu-manager-done
|
@Anshgrover23 review my pr, thanks |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@tuanwannafly Do not resolve the coderabbitai comments, address them fully, then ask for review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 1114-1170: The gpu set flow currently ignores a --dry-run flag and
allows --dry-run and --execute together; update the argparse setup to make
--dry-run and --execute mutually exclusive (or remove --dry-run) and change the
gpu(self, args) logic in the gpu method to honor args.dry_run explicitly: when
args.dry_run is True, do not prompt or call apply_gpu_mode_switch(execute=True)
and always treat as dry-run (show plan only); when args.execute is True, require
confirmation (unless args.yes) and then call apply_gpu_mode_switch(plan,
execute=True); also add an explicit error/return when both flags are present if
you cannot change argparse, and update the user tip text to reflect the
corrected flags.
- Around line 1028-1113: Wrap the call to estimate_gpu_battery_impact() inside
gpu_battery with a try/except that catches expected probing errors (e.g.,
OSError, PermissionError, subprocess.CalledProcessError, ValueError,
RuntimeError and a broad Exception fallback), and on error print a concise
user-facing message via cx_print (e.g., "Failed to probe battery/GPU info:
<error>") and return a non-zero exit code (e.g., 2); keep the rest of
gpu_battery logic unchanged so measured_only handling and printing still work
when probing succeeds.
🧹 Nitpick comments (3)
cortex/cli.py (3)
22-33: Consider lazy-importing GPU modules to avoid slowing CLI startup for non-GPU commands.
Right nowcortex.gpu_managerand especiallycortex.hardware_detectionget imported on everycortexinvocation, even when running unrelated commands. Moving these imports intogpu_battery()/gpu()keeps feature behavior the same but reduces baseline startup risk.Proposed refactor (lazy imports)
-from cortex.gpu_manager import ( - apply_gpu_mode_switch, - detect_gpu_switch_backend, - get_app_gpu_preference, - get_per_app_gpu_env, - list_app_gpu_preferences, - plan_gpu_mode_switch, - remove_app_gpu_preference, - run_command_with_env, - set_app_gpu_preference, -) -from cortex.hardware_detection import detect_gpu_mode, estimate_gpu_battery_impact +# +# NOTE: Consider lazy-importing GPU modules inside gpu_battery()/gpu() +# to avoid CLI startup overhead for non-GPU commands. +#Then inside
gpu_battery()/gpu()add local imports.
1171-1222: CLI ergonomics:gpu runshould print the injected env in verbose mode (optional) + align exit codes.
The flow is solid and keeps switching “per app” without global mode changes. Two small nits:
- Consider printing (or
self._debug-logging) the env keys being injected when--verboseis enabled (helps users verify PRIME offload vars).gpu app removereturns1when nothing removed, while other “not available” states often return2in this command; not wrong, but worth aligning if you want2to consistently mean “not applicable / missing backend / invalid usage.”
2340-2385: Argparse wiring: makegpu set --dry-run/--executemutually exclusive; consider adding help strings + update rich help table.
gpu_set.add_argument("--dry-run"...should be mutually exclusive with--executeto avoid ambiguous invocation.- Subcommands like
gpu app listhave nohelp=...(minor, but improves UX in--help).show_rich_help()prints a curated command table; consider addinggpuandgpu-batterythere so they show up when users runcortexwith no args.Proposed argparse tweak (mutually exclusive dry-run/execute)
gpu_set = gpu_sub.add_parser("set", help="Switch GPU mode") gpu_set.add_argument("mode", choices=["integrated", "hybrid", "nvidia"]) - gpu_set.add_argument("--dry-run", action="store_true") - gpu_set.add_argument("--execute", action="store_true") + gpu_set_mode = gpu_set.add_mutually_exclusive_group() + gpu_set_mode.add_argument("--dry-run", action="store_true", help="Show commands only") + gpu_set_mode.add_argument("--execute", action="store_true", help="Run commands (may require sudo)") gpu_set.add_argument("-y", "--yes", action="store_true")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/gpu_manager.py (9)
apply_gpu_mode_switch(301-317)detect_gpu_switch_backend(220-231)get_app_gpu_preference(170-180)get_per_app_gpu_env(193-217)list_app_gpu_preferences(183-190)plan_gpu_mode_switch(234-279)remove_app_gpu_preference(153-167)run_command_with_env(282-298)set_app_gpu_preference(134-150)cortex/hardware_detection.py (2)
detect_gpu_mode(839-858)estimate_gpu_battery_impact(861-939)
⏰ 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). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (1)
cortex/cli.py (1)
2787-2791: Main dispatch additions look correct.
Routesgpu-batteryandgputo the new handlers consistently with existing command routing.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 1235-1238: The current gpu app get branch prints "app: None",
which is unclear; update the block that calls get_app_gpu_preference(args.app)
to check if pref is None, and if so call cx_print with a friendly message like
"{args.app}: GPU preference not set" and return a distinct non-zero exit code
(e.g., return 2) so callers can detect the "not set" case; otherwise keep the
existing cx_print("{args.app}: {pref}") and return 0.
- Around line 2373-2430: show_rich_help() currently builds the top-level command
table but misses the new "gpu" and "gpu-battery" parsers so running `cortex`
with no args won't list them; update show_rich_help() to include entries for the
gpu subparser (created as gpu_parser) and gpu-battery subparser
(gpu_battery_parser) when it enumerates top-level subcommands (or when iterating
subparsers._actions), ensuring both names and short help texts are added to the
displayed table so the no-args landing help remains discoverable.
- Around line 1204-1223: The integrated-GPU path can be ineffective because
get_per_app_gpu_env(use_nvidia=False) (and the app-resolves-to-integrated case)
returns an empty dict and run_command_with_env simply merges it into os.environ,
leaving existing PRIME vars set in the parent process; fix by explicitly
unsetting those PRIME vars when launching integrated GPU runs: in the
args.gpu_command == "run" branch (the code that calls get_per_app_gpu_env and
run_command_with_env), detect the integrated case (get_per_app_gpu_env called
with use_nvidia=False or app preference resolved to integrated) and if the
returned env is empty, prepend an explicit wrapper to the command such as
["env", "-u", "__NV_PRIME_RENDER_OFFLOAD", "-u", "__GLX_VENDOR_LIBRARY_NAME",
"-u", "__VK_LAYER_NV_optimus", "--"] + cmd before calling
run_command_with_env(cmd, extra_env=env), so the child process runs with those
PRIME vars unset; apply the same logic for the --app path when the app
preference resolves to integrated.
🧹 Nitpick comments (3)
cortex/cli.py (3)
4-35: Consider lazy-importing GPU modules to avoid CLI startup overhead / platform import hazardsRight now
cortex/cli.pyimports GPU probing/switching modules at import time. Ifcortex.hardware_detection(or transitive imports) ever becomes “heavy” or platform-sensitive, this impacts every CLI invocation, even non-GPU commands. A simple pattern is to import GPU modules insidegpu_battery()/gpu()only.Proposed refactor (lazy imports)
-import subprocess +import subprocess ... -from cortex.gpu_manager import ( - apply_gpu_mode_switch, - detect_gpu_switch_backend, - get_app_gpu_preference, - get_per_app_gpu_env, - list_app_gpu_preferences, - plan_gpu_mode_switch, - remove_app_gpu_preference, - run_command_with_env, - set_app_gpu_preference, -) -from cortex.hardware_detection import detect_gpu_mode, estimate_gpu_battery_impact + # NOTE: Consider lazy-importing GPU modules inside `gpu()` / `gpu_battery()` + # to avoid import-time overhead for non-GPU commands.And inside
gpu_battery()/gpu():from cortex.gpu_manager import ... from cortex.hardware_detection import ...
1029-1127:gpu_battery()looks solid; consider returning structured output (or a--jsonmode) laterError handling and the
--measured-onlyUX are reasonable, and the public method has a docstring + type hints (per guidelines).
1151-1203: Useshlex.join()when printing planned commands to avoid ambiguity
" ".join(c)can mislead when args contain spaces/special chars;shlex.join(c)prints a shell-accurate representation.Proposed change
+import shlex ... cx_print("Commands:") for c in plan.commands: - cx_print(" " + " ".join(c)) + cx_print(" " + shlex.join(c))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/gpu_manager.py (9)
apply_gpu_mode_switch(301-317)detect_gpu_switch_backend(220-231)get_app_gpu_preference(170-180)get_per_app_gpu_env(193-217)list_app_gpu_preferences(183-190)plan_gpu_mode_switch(234-279)remove_app_gpu_preference(153-167)run_command_with_env(282-298)set_app_gpu_preference(134-150)cortex/hardware_detection.py (1)
detect_gpu_mode(839-858)
⏰ 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). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (1)
cortex/cli.py (1)
2832-2836: Dispatch forgpu-battery/gpuis clean
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 1040-1051: The except tuple that handles errors from calling
estimate_gpu_battery_impact incorrectly includes subprocess.CalledProcessError;
remove subprocess.CalledProcessError from that tuple (leave OSError,
PermissionError, ValueError, RuntimeError) and also remove the redundant second
generic "except Exception as e" block since estimate_gpu_battery_impact handles
subprocess errors internally—keep the single except that logs via cx_print and
returns 2.
🧹 Nitpick comments (3)
cortex/cli.py (3)
1096-1110: Consider simplifying repetitive type conversion pattern.The try/except blocks for type conversion are defensive but repetitive. Consider extracting a helper function for safe float formatting.
♻️ Proposed refactor
Add a helper function earlier in the class:
def _safe_format_float(self, value: Any, precision: int = 2) -> str: """Safely format a value as a float with given precision.""" try: return f"~{float(value):.{precision}f}" except (TypeError, ValueError): return f"~{value}"Then simplify the calls:
if power_watts is not None: - try: - print(f"- Battery draw: ~{float(power_watts):.2f} W") - except (TypeError, ValueError): - print(f"- Battery draw: ~{power_watts} W") + print(f"- Battery draw: {self._safe_format_float(power_watts)} W") if hours_remaining is not None: - try: - print(f"- Est. time remaining: ~{float(hours_remaining):.2f} h") - except (TypeError, ValueError): - print(f"- Est. time remaining: ~{hours_remaining} h") + print(f"- Est. time remaining: {self._safe_format_float(hours_remaining)} h") if has_nvidia_data: - try: - print(f"- NVIDIA power draw: ~{float(nvidia_watts):.2f} W") - except (TypeError, ValueError): - print(f"- NVIDIA power draw: ~{nvidia_watts} W") + print(f"- NVIDIA power draw: {self._safe_format_float(nvidia_watts)} W")
1153-1155: Defensive guard is redundant (nitpick).The mutual exclusivity between
--dry-runand--executeis already enforced by argparse (line 2418), making this runtime check unnecessary. However, keeping it as defensive programming is acceptable.
1204-1240: Consider documenting the integrated GPU environment unsetting behavior.The logic at lines 1225-1238 builds an
env -ucommand prefix to explicitly unset NVIDIA PRIME variables for integrated GPU runs. This is a subtle but important detail for the integrated mode to work correctly when the parent shell has these variables set. Consider adding a comment explaining why this is necessary.📝 Suggested comment
+ # For integrated GPU mode, we must explicitly unset NVIDIA PRIME + # offload variables that may be present in the parent shell environment. + # run_command_with_env only adds vars, so we use 'env -u' to remove them. if (integrated_case or app_case) and not env: cmd = [ "env",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.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/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (7)
cortex/gpu_manager.py (9)
apply_gpu_mode_switch(301-317)detect_gpu_switch_backend(220-231)get_app_gpu_preference(170-180)get_per_app_gpu_env(193-217)list_app_gpu_preferences(183-190)plan_gpu_mode_switch(234-279)remove_app_gpu_preference(153-167)run_command_with_env(282-298)set_app_gpu_preference(134-150)cortex/hardware_detection.py (2)
detect_gpu_mode(839-858)estimate_gpu_battery_impact(861-939)cortex/branding.py (1)
cx_print(49-69)cortex/kernel_features/llm_device.py (1)
getattr(81-105)cortex/kernel_features/accelerator_limits.py (2)
get(69-72)status(97-105)cortex/kernel_features/model_lifecycle.py (1)
status(135-143)cortex/sandbox/sandbox_executor.py (1)
execute(501-633)
⏰ 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). (4)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (5)
cortex/cli.py (5)
4-4: LGTM!The new imports are properly organized and all are actively used in the GPU management methods below.
Also applies to: 23-34
2295-2296: LGTM!Help table entries are clear and follow the existing formatting pattern.
2856-2859: LGTM!Command routing is properly implemented and follows the existing pattern in the main function.
1225-1238: Unsetting is necessary and correct — no changes needed.The explicit
env -ucommands are justified. These NVIDIA PRIME variables (__NV_PRIME_RENDER_OFFLOAD,__GLX_VENDOR_LIBRARY_NAME,__VK_LAYER_NV_optimus) may be set in the parent shell from previous NVIDIA-mode runs or user environment configuration. Sincerun_command_with_envinherits the current environment and only adds variables (doesn't remove), the only way to prevent these PRIME variables from affecting integrated GPU mode is to explicitly unset them withenv -u. The condition(integrated_case or app_case) and not envcorrectly limits unsetting to cases where no GPU-specific env vars need to be set, making this approach sound.
2397-2453: Well-structured argument parsing with proper constraints.The GPU command parsers are correctly set up with:
- Mutual exclusivity for
--dry-runand--execute- Required subparsers to enforce subcommand selection
- Proper use of
argparse.REMAINDERfor the run commandThe project requires Python 3.10+, which fully supports
required=Trueon subparsers (added in Python 3.7) andargparse.REMAINDER(added in Python 3.5), so all features used here are safe.
|
|
@Anshgrover23 I resolved all bot comment |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@tuanwannafly Have u reviewed this on hybrid system ? Attach a video.
| - Heuristic: `nvidia-smi` may be missing or blocked even on NVIDIA systems. | ||
| - On some hybrid laptops, NVIDIA may be detected but still idle. | ||
| """ | ||
| return bool(_run(["nvidia-smi"])) |
There was a problem hiding this comment.
This only checks whether nvidia-smi exists/returns output. That does not mean the NVIDIA GPU is active or drawing power on Optimus systems.
@tuanwannafly Have u really checked this on a hybrid system ?
|
|
||
| if args.gpu_command == "status": | ||
| backend = detect_gpu_switch_backend() | ||
| mode = detect_gpu_mode() |
There was a problem hiding this comment.
This logic is NVIDIA-centric only and collapses multiple distinct cases (idle NVIDIA, non-NVIDIA dGPU) into misleading modes.
| # Execute only when --execute is set (otherwise plan-only) | ||
| if getattr(args, "execute", False): | ||
| if not getattr(args, "yes", False): | ||
| console.print("\n⚠ This will run GPU switch commands with sudo.") |
There was a problem hiding this comment.
Needs a stronger warning that this can break display sessions and requires reboot/logout.
| return "" | ||
|
|
||
| monkeypatch.setattr(hd, "_run", fake_run) | ||
| assert hd.detect_gpu_mode() == "NVIDIA" |
There was a problem hiding this comment.
Current tests lock in the incorrect assumption that nvidia-smi implies active NVIDIA usage, making future fixes harder.
I use win and Ubuntu wsl so I cann't test it on Hybrid system |
|
@tuanwannafly Thanks for the update. Since this feature targets real hybrid (Optimus) GPU behavior and hasn’t been validated on actual hybrid hardware, we can’t proceed with this PR. |
2 participants
Summary
This PR adds a lightweight Hybrid GPU (Optimus) manager feature focused on reducing latency and stuttering caused by global GPU switching.
It introduces GPU state detection and heuristic battery impact estimation without requiring global mode changes or session restarts.
Related Issue
Closes #454
Type of Change
AI Disclosure
Used ChatGPT as a reasoning assistant to help design the solution, validate edge cases, and improve clarity of the implementation and testing approach.
Testing
cortex gpu-batteryto verify GPU state detection and battery impact estimates.pytest.Checklist
Demo Video
Demo Videos (9-part reviewer walkthrough)
I’m attaching 9 short videos, each mapped 1:1 to the script sections below, to demonstrate full Hybrid GPU Manager scope: state, battery estimation, per-app assignment, env injection, and mode switching (dry-run + execute path).
vid1.mp4
cortexbinary is available and prints versions for Cortex, Python, and Pytest.vid2.mp4
pytest --collect-onlyfiltered bygputo prove relevant tests exist and are discoverable.vid3.mp4
pytest -vv tests/test_gpu_manager.py -rAto validate:vid4.mp4
pytest -vv tests/test_hybrid_gpu_manager.py -rAto validate:vid5.mp4
cortex gpu statusshowing:vid6.mp4
cortex gpu-battery(heuristics + measured fields when available),cortex gpu-battery --measured-onlyto demonstrate safe degradation when measurements are unavailable.vid7.mp4
CORTEX_CONFIG_DIR,blender -> nvidia,get,list),config.yaml,vid8.mp4
cortex gpu runapplying env vars correctly:--integratedoverride,--nvidiaoverride,--app blenderresolves to the saved preference,__NV_PRIME_RENDER_OFFLOADand__GLX_VENDOR_LIBRARY_NAME.vid9.mp4
sudoandprime-selectintoPATH,--execute --yesto prove the execute path calls expected backend commands without requiring real system mutation in the demo environment.Bonus in the same script — Negative CLI Validation
Uploading vid10.mp4…
cortex gpu set turbo ...cortex gpu app set blender turbo ...Summary by CodeRabbit
New Features
New Utilities
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.