huggingface / lerobot

🤗 LeRobot: Making AI for Robotics more accessible with end-to-end learning
Apache License 2.0
7.67k stars 738 forks source link

PoC: Have a go at fixing class MotorsBus(Protocol) #508

Closed alsuren closed 1 week ago

alsuren commented 1 week ago

In the Robot class, we have the comment:

    # TODO(rcadene, aliberts): Add unit test checking the protocol is implemented in the corresponding classes

(CC: @Cadene @aliberts I guess)

This is a proof of concept that it is possible to get a type checker to check our Protocol implementations.

In my case, I was using the pylance integration in vscode, and ignoring all of the other warnings and errors. I can have a crack at adding a mypy check in CI, but it might be quite invasive, and I haven't thought about how to split the work up and do it incrementally.

Making MotorsBus be an abstract base class would be an alternative way to do this. That would have the advantage of making "go to implementations" work in IDEs (I've not tested that approach yet). The disadvantage of abstract base class is that it becomes tempting to start using the Template Method Pattern and move some logic into the abstract base class (this can get difficult to reason about really quickly). If you want me to have a go at that for compariasson, I can.

If you like this static type checking approach, I can try cleaning this PR up. Things that would need to be done include:

A follow-up PR could then do:

What this does

Explain what this PR does. Feel free to tag your PR with the appropriate label(s).

Examples: Title Label
Proof of concept for using types to reduce the amount of tests we need to write (🔬 Tests)

How it was tested

Explain/show how you tested your changes.

Screenshot 2024-11-16 at 18 28 43

Screenshot 2024-11-16 at 18 28 37

How to checkout & try? (for the reviewer)

(see above vscode example)

This is a very early Proof of Concept, and I didn't spend much time on it. I would appreciate feedback on this approach, and if I should keep going down the static type checking route, or whether we want to just rely on unit tests for this kind of thing. If we don't want to take this approach then feel free to close this PR.

alsuren commented 1 week ago

From discussions on discord, it sounds like we're wanting to rethink the protocol classes anyway (https://discord.com/channels/1216765309076115607/1237737604611571722/1307416358417469460 - https://github.com/huggingface/lerobot/pull/493#discussion_r1833244920). I think my instincts say ABC too, with the caveat about avoiding the Template Method Pattern with complex glue logic in the base class (better to have a wrapper that holds a MotorBus and put shared logic there).

Closing this now, because it's mostly only useful as a reference. Feel free to comment here or on discord if there's anything you want clarification on.