diff --git a/makermodslab/record.py b/makermodslab/record.py index 6ba612fd..3e748214 100644 --- a/makermodslab/record.py +++ b/makermodslab/record.py @@ -46,6 +46,7 @@ from .utils.config import ( CameraResolutionError, load_robot_cameras, + require_robot_cameras, validate_dataset_repo_id, with_makermodslab_tag, ) @@ -624,8 +625,16 @@ def handle_start_recording(request: RecordingRequest) -> dict[str, Any]: # before the flag is claimed, so a wrong robot name or an ambiguous # camera set is a plain 400 instead of a session that starts and # silently records no video. + # + # `require_robot_cameras` (not `load_robot_cameras`) so a named robot + # whose record holds NO cameras is refused too: that record is the only + # source of session cameras, so an empty one records a whole dataset + # with no video at all — the same silent loss a wrong robot name causes, + # and just as invisible until the dataset is opened. It is reachable + # without anyone choosing it: a record starts life camera-less, and + # external cleanup of stale camera entries can empty one that had them. try: - session_cameras = load_robot_cameras(request.robot_name) + session_cameras = require_robot_cameras(request.robot_name, "recording") except CameraResolutionError as exc: logger.warning("Rejected recording start: %s", exc) return {"success": False, "status_code": 400, "message": str(exc)} diff --git a/makermodslab/rollout.py b/makermodslab/rollout.py index 525e4aae..6b7baf76 100644 --- a/makermodslab/rollout.py +++ b/makermodslab/rollout.py @@ -84,6 +84,7 @@ bimanual_base_id, bind_robot_cameras, list_robot_records, + require_robot_cameras, setup_follower_calibration_file, stage_bimanual_follower_calibrations, ) @@ -1736,7 +1737,16 @@ def _release_slot() -> None: # read, no hardware). A binding that names a camera the record doesn't have # must 4xx in the panel; deferring it to the startup worker would surface # the same mistake as a mid-startup failure after the model download. + # + # The record's own camera count is checked FIRST, and separately, because + # the binding resolution cannot see this case: empty `camera_bindings` + # short-circuit inside bind_robot_cameras before the record is ever read, so + # a camera-less record would otherwise start a run that drives the arm with + # no vision at all. Checking it ahead of the bindings also means the message + # names the real fix ("no cameras configured — add one") instead of the + # binding-shaped "no camera named 'x'; cameras on this robot: none". try: + require_robot_cameras(request.robot_name, "running a policy") _session_cameras(request) except CameraResolutionError as exc: _release_slot() diff --git a/makermodslab/utils/config.py b/makermodslab/utils/config.py index fbcce357..ea5ceea0 100644 --- a/makermodslab/utils/config.py +++ b/makermodslab/utils/config.py @@ -573,6 +573,45 @@ def load_robot_cameras(robot_name: str) -> dict[str, dict]: return record_cameras_by_name(record.get("cameras") or []) +def require_robot_cameras(robot_name: str, action: str) -> dict[str, dict]: + """`load_robot_cameras`, but a NAMED robot that has no cameras is refused. + + The robot record is the only source of a session's cameras, so a record with + an empty camera list means the activity would run blind: recording would + write a whole dataset with no video, and inference would feed a policy none + of the image features it was trained on. Neither surfaces at the time — the + dataset simply turns out to be useless, and the policy simply behaves badly + — so the refusal has to happen at the start. + + An empty list is reachable without anyone choosing it: a record is created + camera-less and stays that way until someone adds one, and external cleanup + of stale entries (removing cameras that predate `unique_id` support, say) + can empty a record that used to have them. "No cameras" is therefore not + evidence that a camera-less session was intended, and must not be read as + one. + + `action` completes the sentence "…before {action}" ("recording", "running a + policy"), so each flow names what it is refusing rather than a generic + "starting". + + A BLANK name is deliberately still allowed through as a camera-less session + ({}), exactly as `load_robot_cameras` documents: there is no record to judge + (and no name to put in the message). Only a robot that IS named and DOES + resolve is held to having at least one camera. + + Callers that only need the cameras — and any read-only lister — should keep + using `load_robot_cameras`; this is for the start paths that must refuse. + Teleoperation and calibration open no cameras at all and must not use it. + """ + cameras = load_robot_cameras(robot_name) + name = (robot_name or "").strip() + if name and not cameras: + raise CameraResolutionError( + f"Robot '{name}' has no cameras configured — add one in Robot settings before {action}." + ) + return cameras + + def _positive_int(value: object) -> int | None: """`value` as a usable pixel dimension, or None. bool is excluded (it is an int subclass, and `True` as a width is always a bug, never a 1-pixel frame).""" diff --git a/tests/test_record.py b/tests/test_record.py index 91b3e7f8..b576d988 100644 --- a/tests/test_record.py +++ b/tests/test_record.py @@ -2073,6 +2073,105 @@ def test_start_recording_rejects_with_400_on_duplicate_camera_names( assert record.recording_active is False +def test_start_recording_rejects_with_400_when_the_robot_has_no_cameras( + monkeypatch: pytest.MonkeyPatch, tmp_lerobot_home +) -> None: + """A record with an EMPTY camera list must refuse too. + + The record is the only source of session cameras, so an empty one records a + whole dataset with no video — the same silent loss a wrong robot name + causes, and just as invisible until the dataset is opened. + """ + import makermodslab.record as record + from makermodslab.utils import config as cfg + + _idle_mutexes(monkeypatch) + robots_dir = tmp_lerobot_home / "robots" + robots_dir.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(cfg, "ROBOTS_PATH", str(robots_dir)) + cfg.save_robot_record("blind", {"cameras": []}, allow_create=True) + + result = record.handle_start_recording(_stub_recording_request(robot_name="blind")) + + assert result["success"] is False + assert result["status_code"] == 400 + assert "blind" in result["message"] + assert "Robot settings" in result["message"] + # Names what it is refusing, not a generic "before starting". + assert "recording" in result["message"] + # Refused BEFORE the active flag is claimed, so there is no session to stop. + assert record.recording_active is False + + +def test_start_recording_refuses_a_robot_saved_without_any_cameras( + monkeypatch: pytest.MonkeyPatch, tmp_lerobot_home +) -> None: + """The motivating case: nobody chose a camera-less session — the record was + saved with its ports and calibrations and simply never had a camera added + (the same shape external cleanup of stale camera entries leaves behind). + The record carries no `cameras` key at all, so this also pins the + missing-key path, not just an explicit empty list. Without this refusal that + robot records silently, and videolessly. + """ + import makermodslab.record as record + from makermodslab.utils import config as cfg + + _idle_mutexes(monkeypatch) + robots_dir = tmp_lerobot_home / "robots" + robots_dir.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(cfg, "ROBOTS_PATH", str(robots_dir)) + cfg.save_robot_record( + "legacy", + { + "leader_port": "/dev/leader", + "follower_port": "/dev/follower", + "leader_config": "teleop", + "follower_config": "robot", + }, + allow_create=True, + ) + + result = record.handle_start_recording(_stub_recording_request(robot_name="legacy")) + + assert result["success"] is False + assert result["status_code"] == 400 + assert "legacy" in result["message"] + assert "Robot settings" in result["message"] + # The camera gate, not the missing-record branch of load_robot_cameras + # (which also says "Robot settings" — the record here exists). + assert "no cameras configured" in result["message"] + assert record.recording_active is False + + +def test_start_recording_still_allows_a_blank_robot_name( + monkeypatch: pytest.MonkeyPatch, tmp_lerobot_home +) -> None: + """The no-cameras refusal is scoped to a NAMED robot. + + A blank name is a camera-less session by definition (see + utils/config.load_robot_cameras) — there is no record to judge and no name + to put in the message — so it must still get past the camera check, or the + gate would quietly become "recording always requires a robot record". + Config construction is stubbed to raise so the start fails immediately + afterwards, without touching hardware; the assertion is on WHICH failure. + """ + import makermodslab.record as record + + _idle_mutexes(monkeypatch) + + def _boom(request, cameras=None): + raise RuntimeError("stop before hardware") + + monkeypatch.setattr(record, "create_record_config", _boom) + + result = record.handle_start_recording(_stub_recording_request(robot_name="")) + + assert result["success"] is False + # It failed at config construction, NOT at the camera gate. + assert "no cameras configured" not in result["message"] + assert "stop before hardware" in result["message"] + + def test_start_recording_ignores_a_stale_cameras_payload( monkeypatch: pytest.MonkeyPatch, tmp_lerobot_home ) -> None: diff --git a/tests/test_rollout.py b/tests/test_rollout.py index 7a0e6f49..940fc65b 100644 --- a/tests/test_rollout.py +++ b/tests/test_rollout.py @@ -682,6 +682,79 @@ def test_handle_start_inference_blocked_when_wiggle_active(monkeypatch) -> None: assert "wiggle" in result["message"].lower() +def _camera_less_robot_record(tmp_lerobot_home, monkeypatch, name: str) -> None: + """Write a robot record whose camera list is empty into a redirected + ROBOTS_PATH — the shape a never-configured record (or one an external + cleanup of stale camera entries emptied) has.""" + from makermodslab.utils import config as cfg + + robots_dir = tmp_lerobot_home / "robots" + robots_dir.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(cfg, "ROBOTS_PATH", str(robots_dir)) + cfg.save_robot_record(name, {"cameras": []}, allow_create=True) + + +def test_handle_start_inference_rejects_a_robot_with_no_cameras(monkeypatch, tmp_lerobot_home) -> None: + """A camera-less record must 400 even when the request binds nothing. + + This is the case bind_robot_cameras structurally cannot catch: empty + `camera_bindings` short-circuit before the record is ever read, so without + its own check the run would start and drive the arm with no vision at all. + """ + from makermodslab import rollout + from makermodslab.rollout import InferenceRequest + + _camera_less_robot_record(tmp_lerobot_home, monkeypatch, "blind") + + result = rollout.handle_start_inference( + InferenceRequest( + follower_port="/dev/ttyUSB0", + follower_config="robot_a", + policy_ref="user/repo@checkpoints/000050", + robot_name="blind", + camera_bindings={}, + ) + ) + + assert result["success"] is False + assert result["status_code"] == 400 + assert "blind" in result["message"] + assert "Robot settings" in result["message"] + # Names what it is refusing, not a generic "before starting". + assert "running a policy" in result["message"] + # The claimed slot is handed back, so this rejection can't wedge the next start. + assert rollout.inference_active is False + + +def test_handle_start_inference_no_cameras_message_wins_over_the_binding_error( + monkeypatch, tmp_lerobot_home +) -> None: + """With bindings set, a camera-less record would already fail inside + bind_robot_cameras — but as "no camera named 'wrist'; cameras on this robot: + none", which reads like a bad binding. The record-level check runs first so + the user is told the actual fix: the robot has no cameras, add one.""" + from makermodslab import rollout + from makermodslab.rollout import InferenceRequest + + _camera_less_robot_record(tmp_lerobot_home, monkeypatch, "blind") + + result = rollout.handle_start_inference( + InferenceRequest( + follower_port="/dev/ttyUSB0", + follower_config="robot_a", + policy_ref="user/repo@checkpoints/000050", + robot_name="blind", + camera_bindings={"front": "wrist"}, + ) + ) + + assert result["success"] is False + assert result["status_code"] == 400 + assert "no cameras configured" in result["message"] + assert "no camera named" not in result["message"] + assert rollout.inference_active is False + + def test_handle_start_inference_pins_return_to_initial_position(monkeypatch, tmp_path) -> None: """The stop dialog promises the follower eases back to its start pose on teardown. That behaviour is lerobot's `return_to_initial_position`, which @@ -1054,7 +1127,9 @@ def test_handle_start_inference_ignores_a_stale_cameras_payload() -> None: assert req.camera_bindings == {} -def test_handle_start_inference_bimanual_builds_bi_so_follower_command(monkeypatch, tmp_path) -> None: +def test_handle_start_inference_bimanual_builds_bi_so_follower_command( + monkeypatch, tmp_path, tmp_lerobot_home +) -> None: """End-to-end (no hardware): a bimanual request stages the two follower calibrations and hands Popen a `bi_so_follower` argv with both ports and two stdin newlines (one prompt per sub-arm's connect()). @@ -1062,9 +1137,15 @@ def test_handle_start_inference_bimanual_builds_bi_so_follower_command(monkeypat Mirrors the pin-test's stub pattern: subprocess, the two preflights, and the staging helper are all replaced so nothing real runs; the startup worker (and its stdout pump) run inline via _SyncThread and HOME is redirected so the log - file lands in tmp.""" + file lands in tmp. + + A real robot record is written for the request's `robot_name` because the + start path now resolves that record's cameras even when `camera_bindings` is + empty (a camera-less record is refused). The record's camera stays UNBOUND, + so the argv this test asserts on is unchanged — no `--robot.cameras` arg.""" from makermodslab import rollout + _robot_record_with_cam(tmp_lerobot_home, monkeypatch, "dual_arm") monkeypatch.setenv("HOME", str(tmp_path)) monkeypatch.setattr(rollout, "bimanual_base_id", lambda name: "dual_arm") monkeypatch.setattr(