-
Notifications
You must be signed in to change notification settings - Fork 40
fix: Windows stability issues (console encoding, camera enumeration, … #72
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| from pydantic import BaseModel | ||
|
|
||
| from lerobot.configs.dataset import DatasetRecordConfig | ||
| from lerobot.configs.video import RGBEncoderConfig | ||
| from lerobot.datasets import LeRobotDataset | ||
| from lerobot.robots.so_follower import SO101FollowerConfig | ||
|
|
||
|
|
@@ -183,6 +184,10 @@ def create_record_config(request: RecordingRequest) -> RecordConfig: | |
| tags=with_lelab_tag(request.tags) if request.push_to_hub else None, | ||
| private=request.private, | ||
| streaming_encoding=request.streaming_encoding, | ||
| # libsvtav1 (lerobot's default) is CPU-heavy enough on this machine to starve | ||
| # the motor bus read loop mid-recording ("no status packet" ConnectionError, | ||
| # see 2026-07-15 crashes). h264/ultrafast measured ~11x faster to encode here. | ||
| rgb_encoder=RGBEncoderConfig(vcodec="h264", preset="ultrafast"), | ||
| ) | ||
|
|
||
| # Create the main record config | ||
|
|
@@ -309,11 +314,15 @@ def recording_worker(): | |
| "robot_type": getattr(dataset.meta, "robot_type", "Unknown robot"), | ||
| } | ||
| except Exception as e: | ||
| logger.exception("Recording session failed") | ||
| logger.exception("Recording session failed: %r", e) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
| current_phase = "error" | ||
| if recording_start_time: | ||
| session_end_elapsed_seconds = int(time.time() - recording_start_time) | ||
| last_recording_info = {"success": False, "error": str(e)} | ||
| last_recording_info = { | ||
| "success": False, | ||
| "error": str(e) or repr(e), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| "dataset_repo_id": request.dataset_repo_id, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: this is an unintended regression. Also, this dict carries Suggest gating on success: if last_recording_info and last_recording_info.get("success") \
and last_recording_info.get("dataset_repo_id") == request.dataset_repo_id: |
||
| } | ||
| finally: | ||
| if current_phase != "error": | ||
| current_phase = "completed" | ||
|
|
@@ -419,7 +428,11 @@ def handle_recording_status() -> dict[str, Any]: | |
| "rerecord_episode": recording_active | ||
| and current_phase == "recording", # Only during recording phase | ||
| }, | ||
| "message": "Recording session failed with error - check logs" | ||
| "message": ( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The global is read twice, guard then subscript, and info = last_recording_info
error = info.get("error") if info else NoneThen build the message above the |
||
| f"Recording failed: {last_recording_info['error']}" | ||
| if last_recording_info and last_recording_info.get("error") | ||
| else "Recording session failed with error - check logs" | ||
| ) | ||
| if current_phase == "error" | ||
| else ( | ||
| "Recording session has ended - stop polling" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,15 @@ | |
| from starlette.responses import Response | ||
| from starlette.types import Scope | ||
|
|
||
| # Windows consoles often use a legacy code page (cp1252) that can't encode the | ||
| # emoji used in log/print output; an unhandled UnicodeEncodeError inside an | ||
| # endpoint then turns into a 500 and hides the real error from the frontend. | ||
| # Replace unencodable characters instead of crashing. | ||
| for _stream in (sys.stdout, sys.stderr): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: this statement sits between the stdlib imports and the local ones, so every Suggest moving it into |
||
| if _stream is not None and hasattr(_stream, "reconfigure"): | ||
| with contextlib.suppress(Exception): | ||
| _stream.reconfigure(errors="replace") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Alternatively, dropping the emoji from the log lines in |
||
|
|
||
| from . import datasets as dataset_browser | ||
|
|
||
| # Import our custom calibration functionality | ||
|
|
@@ -998,6 +1007,18 @@ def _windows_cameras() -> list[dict[str, Any]]: | |
| frontend match each index to the browser's ``MediaDeviceInfo.label`` for the | ||
| live preview. Falls back to generic names if pygrabber is unavailable. | ||
| """ | ||
| # pygrabber uses COM, which must be initialized per-thread. FastAPI serves | ||
| # sync endpoints from a thread pool whose threads never call CoInitialize, | ||
| # so enumeration fails intermittently depending on which thread handles the | ||
| # request. Initialize COM here and balance with CoUninitialize. | ||
| import ctypes | ||
|
|
||
| com_initialized = False | ||
| try: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fix is right, including the Two things though: |
||
| hr = ctypes.windll.ole32.CoInitialize(None) | ||
| com_initialized = hr in (0, 1) # S_OK or S_FALSE (already initialized) | ||
| except Exception: | ||
| pass | ||
| try: | ||
| from pygrabber.dshow_graph import FilterGraph | ||
|
|
||
|
|
@@ -1007,6 +1028,9 @@ def _windows_cameras() -> list[dict[str, Any]]: | |
| import cv2 | ||
|
|
||
| return _generic_cv2_cameras(cv2.CAP_DSHOW) | ||
| finally: | ||
| if com_initialized: | ||
| ctypes.windll.ole32.CoUninitialize() | ||
| return [{"index": i, "name": name, "available": True} for i, name in enumerate(names)] | ||
|
|
||
|
|
||
|
|
||
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.
Blocking, and the main discussion point. This changes the default encoder for every platform, not just Windows.
lerobot v0.6.0 already has better levers for this exact problem:
vcodec="auto"probesh264_videotoolbox/h264_nvenc/h264_vaapi/h264_qsvand falls back tolibsvtav1. Hardware encoding is near zero CPU, which is the actual root cause, and it does not degrade Mac and Linux.DatasetRecordConfig.encoder_threadscaps encoder threads directly, which is the surgical fix for "encoder starves the motor bus read loop".Risk worth checking before merge:
RGBEncoderConfig.__post_init__callsresolve_vcodec(), which raisesValueError: Unsupported video codec: h264when the installed PyAV has no libx264 encoder. That throws insidecreate_record_config, so recording would die before starting on machines that work today. Please confirm on Linux and macOS with the pinned lerobot v0.6.0, not only on the Windows box.Also,
crf=30andg=2are inherited whilepreset="ultrafast"disables most compression tools, so datasets get noticeably larger. The PR measures encode speed (~11x) but not file size.Last, the comment references "this machine" and "2026-07-15 crashes", which a future reader cannot verify. If the setting stays, expose it as a
RecordingRequestfield (same asvideoandstreaming_encodingalready are) or gate it onsys.platform == "win32".