Apply suggestions from code review 1

Co-authored-by: Simon Alibert <75076266+aliberts@users.noreply.github.com>

.gitattributes

@@ -18,4 +18,4 @@
 *.arrow filter=lfs diff=lfs merge=lfs -text
 *.json !text !filter !merge !diff
 tests/artifacts/cameras/*.png filter=lfs diff=lfs merge=lfs -text
-tests/artifacts/cameras/*.bag filter=lfs diff=lfs merge=lfs -text
+*.bag filter=lfs diff=lfs merge=lfs -text

lerobot/common/cameras/__init__.py

@@ -13,5 +13,5 @@
 # limitations under the License.
 
 from .camera import Camera
-from .configs import CameraConfig
+from .configs import CameraConfig, ColorMode
 from .utils import make_cameras_from_configs

lerobot/common/cameras/camera.py

@@ -33,7 +33,7 @@ class Camera(abc.ABC):
         pass
 
     @abc.abstractmethod
-    def connect(self, do_warmup_read: bool = True) -> None:
+    def connect(self, warmup: bool = True) -> None:
         pass
 
     @abc.abstractmethod
@@ -41,7 +41,7 @@ class Camera(abc.ABC):
         pass
 
     @abc.abstractmethod
-    def async_read(self, timeout_ms: float = 2000) -> np.ndarray:
+    def async_read(self, timeout_ms: float = ...) -> np.ndarray:
         pass
 
     @abc.abstractmethod

lerobot/common/cameras/opencv/configuration_opencv.py

@@ -53,7 +53,6 @@ class OpenCVCameraConfig(CameraConfig):
 
     index_or_path: int | Path
     color_mode: ColorMode = ColorMode.RGB
-    channels: int = 3  # NOTE(Steven): Why is this a config?
     rotation: Cv2Rotation = Cv2Rotation.NO_ROTATION
 
     def __post_init__(self):

lerobot/common/utils/control_utils.py

@@ -38,7 +38,6 @@ from lerobot.common.utils.robot_utils import busy_wait
 from lerobot.common.utils.utils import get_safe_torch_device, has_method
 
 
-# NOTE(Steven): Consider integrating this in camera class
 def log_control_info(robot: Robot, dt_s, episode_index=None, frame_index=None, fps=None):
     log_items = []
     if episode_index is not None:

lerobot/find_cameras.py

@@ -32,7 +32,6 @@ from lerobot.common.cameras.opencv.camera_opencv import OpenCVCamera
 from lerobot.common.cameras.opencv.configuration_opencv import OpenCVCameraConfig
 
 logger = logging.getLogger(__name__)
-# logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(levelname)s - %(module)s - %(message)s")
 
 
 def find_all_opencv_cameras() -> List[Dict[str, Any]]:

tests/cameras/test_opencv.py

@@ -39,7 +39,8 @@ TEST_IMAGE_PATHS = [
 ]
 
 
-def test_base_class_implementation():
+def test_abc_implementation():
+    """Instantiation should raise an error if the class doesn't implement abstract methods/properties."""
     config = OpenCVCameraConfig(index_or_path=0)
 
     _ = OpenCVCamera(config)
@@ -168,7 +169,7 @@ def test_async_read_before_connect():
         Cv2Rotation.ROTATE_270,
     ],
 )
-def test_all_rotations(rotation, index_or_path):
+def test_rotation(rotation, index_or_path):
     filename = os.path.basename(index_or_path)
     dimensions = filename.split("_")[-1].split(".")[0]  # Assumes filenames format (_wxh.png)
     original_width, original_height = map(int, dimensions.split("x"))

tests/cameras/test_realsense.py

@@ -39,13 +39,8 @@ BAG_FILE_PATH = os.path.join(TEST_ARTIFACTS_DIR, "test_rs.bag")
 # NOTE(Steven): Missing tests for depth
 # NOTE(Steven): Takes 20sec, the patch being the biggest bottleneck
 # NOTE(Steven): more tests + assertions?
-if not os.path.exists(BAG_FILE_PATH):
-    print(f"Warning: Bag file not found at {BAG_FILE_PATH}. Some tests might fail or be skipped.")
-
 
 def mock_rs_config_enable_device_from_file(rs_config_instance, sn):
-    if not os.path.exists(BAG_FILE_PATH):
-        raise FileNotFoundError(f"Test bag file not found: {BAG_FILE_PATH}")
     return rs_config_instance.enable_device_from_file(BAG_FILE_PATH, repeat_playback=True)
 
 
@@ -53,7 +48,8 @@ def mock_rs_config_enable_device_bad_file(rs_config_instance, sn):
     return rs_config_instance.enable_device_from_file("non_existent_file.bag", repeat_playback=True)
 
 
-def test_base_class_implementation():
+def test_abc_implementation():
+    """Instantiation should raise an error if the class doesn't implement abstract methods/properties."""
     config = RealSenseCameraConfig(serial_number=42)
     _ = RealSenseCamera(config)
 
@@ -181,7 +177,7 @@ def test_async_read_before_connect():
     ],
 )
 @patch("pyrealsense2.config.enable_device", side_effect=mock_rs_config_enable_device_from_file)
-def test_all_rotations(mock_enable_device, rotation):
+def test_rotation(mock_enable_device, rotation):
     config = RealSenseCameraConfig(serial_number=42, rotation=rotation)
     camera = RealSenseCamera(config)
     camera.connect(do_warmup_read=False)