Fix camera streamer reconnection with re-exec#239
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces in-process garbage collection retry logic with a full Python process re-execution using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
nv-mhaselton
force-pushed
the
mhaselton/fix-camera-streaming-reconnect
branch
from
63a5510 to
ccc5fbe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/camera_streamer/teleop_camera_app.py (1)
15-15:⚠️ Potential issue | 🟡 MinorRemove unused
gcimport.The
gcimport is no longer used since garbage collection cleanup was replaced withos.execv(). This is causing the pipeline failure.🧹 Proposed fix
import argparse -import gc import os import sys import time🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/camera_streamer/teleop_camera_app.py` at line 15, Remove the unused gc import from teleop_camera_app.py: delete the line importing gc (the unused module symbol "gc") at the top of the file so the pipeline/linter no longer fails; then run the test/linter to confirm no other references to gc remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/camera_streamer/teleop_camera_app.py`:
- Around line 179-182: Replace the list concatenation used in the re-exec call
with iterable unpacking for readability: change the os.execv call that currently
uses os.execv(sys.executable, [sys.executable] + sys.argv) (near
logger.info("Re-executing for clean XR state...")) to use a list literal with
the spread/unpacking syntax e.g. os.execv(sys.executable, [sys.executable,
*sys.argv]); keep the logger.info line and the os.execv invocation otherwise
unchanged and leave the S606 static-analysis warning acknowledged as safe in
this context.
---
Outside diff comments:
In `@examples/camera_streamer/teleop_camera_app.py`:
- Line 15: Remove the unused gc import from teleop_camera_app.py: delete the
line importing gc (the unused module symbol "gc") at the top of the file so the
pipeline/linter no longer fails; then run the test/linter to confirm no other
references to gc remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 483b7fb6-49fb-4ad7-bac4-d503964c3211
📒 Files selected for processing (1)
examples/camera_streamer/teleop_camera_app.py
| # Re-exec for a clean process — in-process GC cannot break the | ||
| # C++ shared_ptr cycles holding the OpenXR instance alive. | ||
| logger.info("Re-executing for clean XR state...") | ||
| os.execv(sys.executable, [sys.executable] + sys.argv) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sound approach for resetting C++ state; consider using spread operator for cleaner syntax.
Using os.execv() to reset the process is appropriate here since Python's GC cannot break C++ shared_ptr cycles holding the OpenXR instance. The static analysis warning S606 about "starting a process without a shell" is expected and safe in this context—os.execv intentionally avoids shell invocation.
Per RUF005, prefer the spread operator for readability:
✨ Minor style improvement
logger.info("Re-executing for clean XR state...")
- os.execv(sys.executable, [sys.executable] + sys.argv)
+ os.execv(sys.executable, [sys.executable, *sys.argv])🧰 Tools
🪛 Ruff (0.15.5)
[error] 182-182: Starting a process without a shell
(S606)
[warning] 182-182: Consider [sys.executable, *sys.argv] instead of concatenation
Replace with [sys.executable, *sys.argv]
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/camera_streamer/teleop_camera_app.py` around lines 179 - 182,
Replace the list concatenation used in the re-exec call with iterable unpacking
for readability: change the os.execv call that currently uses
os.execv(sys.executable, [sys.executable] + sys.argv) (near
logger.info("Re-executing for clean XR state...")) to use a list literal with
the spread/unpacking syntax e.g. os.execv(sys.executable, [sys.executable,
*sys.argv]); keep the logger.info line and the os.execv invocation otherwise
unchanged and leave the S606 static-analysis warning acknowledged as safe in
this context.
1 participant
Summary by CodeRabbit