Enable CI for robot devices with mocked versions

[Cadene]

What this does

• Rework robot devices tests to increase "granularity"
  • Add make_camera and make_motor to instantiate specific camera and motor
  • Add @require_camera and @require_motor that try to instantiate, and skip test if not possible (before we were using @require_motor only)
  • Use these functions in test_cameras.py and test_motors.py instead of relying only on make_robot and @require_robot
• Add tests for cameras, motors and robots that do not require physical robot devices:
  • Add mock=False argument to classes and functions dynamically import mocked classes, functions, variables
  • Add a new mock argument to test functions

TODO:

• Test DynamixelMotorsBus without a dynamixel
• Test ManipulatorRobot without a robot
• Test control robot without a robot

How it was tested

• Ran mocked unit tests manually
• CI

How to checkout & try? (for the reviewer)

pytest -sx tests/test_cameras.py
pytest -sx tests/test_motors.py
pytest -sx tests/test_robots.py
pytest -sx tests/test_control_robot.py

Altogether:

pytest -sx tests/test_cameras.py tests/test_control_robot.py tests/test_motors.py tests/test_robots.py

[aliberts]

First round, very cool!
I'm wondering how to test robots like Stretch that already have a built-in api. We'll probably need to mock the api itself and test that in test_robots.py and test_control_robot.py rather than its low level components. Let me know what you think.

[Cadene]

I think I addressed all comments!

[Cadene]

It seems that self.stop_event is None was not required.

[jess-moss]

Overall it is looking good! I think in the future we may want to remove the testing/mock still from the actual classes (like dynamixel.py, etc). I feel like that should be kept separate eventually but for now I think it will work!

[jess-moss]

Nit: In the if...else statement above you say from pyrealsense2 import context as RSContext # noqa: N812 I presume you do this so this line can become for device in context.query_devices():. To do that you would need to import the mock values as follows:

        from tests.mock_intelrealsenseimport camera_info as RSCameraInfo  # noqa: N812
        from tests.mock_intelrealsenseimport context as RSContext  # noqa: N812

However, if you don't want to do that and keep this line as is then I would change the named imports above since they aren't being used anyways, i.e.

        from pyrealsense2 import (
            RSCameraInfo,
            RSContext,
        )

[Cadene]

I am sorry I didnt understand.

This works:

from tests.mock_intelrealsense import RSCameraInfo
RSCameraInfo(SERIAL_NUMBER_INDEX)

This works:

from pyrealsense2 import camera_info as RSCameraInfo
RSCameraInfo(SERIAL_NUMBER_INDEX)

This works:

from pyrealsense2 import camera_info
camera_info(SERIAL_NUMBER_INDEX)

But this doesnt work:

from pyrealsense2 import camera_info as RSCameraInfo
camera_info(SERIAL_NUMBER_INDEX)

[jess-moss]

Nit: Same comment as above, I would either import these values as named variables like you do when it is not mock, i.e., config, format, pipeline, stream, or leave them all unnamed like you do when it is mock.

[Cadene]

I am sorry I didnt understand

The original element name is config but I rename it to RSConfig to fit python CamelCase format for naming classes:

from pyrealsense2 import config as RSConfig

[jess-moss]

Nit: I don't think you need this line... but I could be wrong ;)

import cv2

[Cadene]

Removed import cv2 and imported setNumThreads to clarify:

from cv2 import (
    CAP_PROP_FPS,
    CAP_PROP_FRAME_HEIGHT,
    CAP_PROP_FRAME_WIDTH,
    VideoCapture,
    setNumThreads,
)
# Use 1 thread to avoid blocking the main thread. Especially useful during data collection
# when other threads are used to save the images.
setNumThreads(1)

[jess-moss]

Nit: By the way, @samzapo is really amazing with the multi-threading stuff. I can help a bit too but he's the expert if you need a second set of eyes.

[jess-moss]

Nit: What happens here if this is mock? Isn't cv2 only imported if it is not mock?

[Cadene]

Before my big change, cv2 was monkeypatched, and this would be an issue.

After my big change, cv2 isn't monkeypatched anymore, but we have this mock boolean variable that imports it or not. setNumThreads is only executed when mock=False.

if self.mock:
    from tests.mock_opencv import (
        CAP_PROP_FPS,
        CAP_PROP_FRAME_HEIGHT,
        CAP_PROP_FRAME_WIDTH,
        VideoCapture,
    )
else:
    from cv2 import (
        CAP_PROP_FPS,
        CAP_PROP_FRAME_HEIGHT,
        CAP_PROP_FRAME_WIDTH,
        VideoCapture,
        setNumThreads,
    )
    # Use 1 thread to avoid blocking the main thread. Especially useful during data collection
    # when other threads are used to save the images.
    setNumThreads(1)

[jess-moss]

Nit: I think you have some commented out code to delete:

# if num_tries > self.fps and (self.thread.ident is None or not self.thread.is_alive()):
            #     raise Exception(
            #         "The thread responsible for `self.async_read()` took too much time to start. There might be an issue. Verify that `self.thread.start()` has been called."
            #     )

[Cadene]

Good catch. It was from Simon Alibert

[jess-moss]

Nit: I don't love that COMM_SUCCESS is hardcoded here instead of being imported from dynamixel_sdk....

[Cadene]

Good catch! Removed this line. Added dynamic import everywhere

if self.mock:
    from tests.mock_dynamixel import GroupSyncWrite, COMM_SUCCESS
else:
    from dynamixel_sdk import GroupSyncWrite, COMM_SUCCESS
    ```

[jess-moss]

Nit: It seems like what is being "mocked" is the dynamixel_sdk... not the dynamixel class itself. Should this class be called mock_dynamixel_sdk?

[Cadene]

Indeed!

• Renamed mock_dynamixel.py to mock_dynamixel_sdk.py
• Renamed mock_intelrealsense.py to mock_pyrealsense2.py
• Renamed mock_opencv.py to mock_cv2.py

[Cadene]

Addressed Jess comments

[Cadene]

I am sorry I didnt understand.

This works:

from tests.mock_intelrealsense import RSCameraInfo
RSCameraInfo(SERIAL_NUMBER_INDEX)

This works:

from pyrealsense2 import camera_info as RSCameraInfo
RSCameraInfo(SERIAL_NUMBER_INDEX)

This works:

from pyrealsense2 import camera_info
camera_info(SERIAL_NUMBER_INDEX)

But this doesnt work:

from pyrealsense2 import camera_info as RSCameraInfo
camera_info(SERIAL_NUMBER_INDEX)

[Cadene]

I am sorry I didnt understand

The original element name is config but I rename it to RSConfig to fit python CamelCase format for naming classes:

from pyrealsense2 import config as RSConfig

[Cadene]

Removed import cv2 and imported setNumThreads to clarify:

from cv2 import (
    CAP_PROP_FPS,
    CAP_PROP_FRAME_HEIGHT,
    CAP_PROP_FRAME_WIDTH,
    VideoCapture,
    setNumThreads,
)
# Use 1 thread to avoid blocking the main thread. Especially useful during data collection
# when other threads are used to save the images.
setNumThreads(1)

[Cadene]

Before my big change, cv2 was monkeypatched, and this would be an issue.

After my big change, cv2 isn't monkeypatched anymore, but we have this mock boolean variable that imports it or not. setNumThreads is only executed when mock=False.

if self.mock:
    from tests.mock_opencv import (
        CAP_PROP_FPS,
        CAP_PROP_FRAME_HEIGHT,
        CAP_PROP_FRAME_WIDTH,
        VideoCapture,
    )
else:
    from cv2 import (
        CAP_PROP_FPS,
        CAP_PROP_FRAME_HEIGHT,
        CAP_PROP_FRAME_WIDTH,
        VideoCapture,
        setNumThreads,
    )
    # Use 1 thread to avoid blocking the main thread. Especially useful during data collection
    # when other threads are used to save the images.
    setNumThreads(1)

[Cadene]

Good catch. It was from Simon Alibert

[Cadene]

Good catch! Removed this line. Added dynamic import everywhere

if self.mock:
    from tests.mock_dynamixel import GroupSyncWrite, COMM_SUCCESS
else:
    from dynamixel_sdk import GroupSyncWrite, COMM_SUCCESS
    ```

[Cadene]

Indeed!

• Renamed mock_dynamixel.py to mock_dynamixel_sdk.py
• Renamed mock_intelrealsense.py to mock_pyrealsense2.py
• Renamed mock_opencv.py to mock_cv2.py

[Cadene]

TODO(rcadene)

[Cadene]

@aliberts with self.lock causing slow fps here

Before fix:

with self.lock:
    if self.color_image is not None:
        return self.color_image

After fix:

if self.color_image is not None:
    return self.color_image