Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

alsuren
Copy link

@alsuren alsuren commented Nov 16, 2024

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:

  • clean up the values type and return type for apply_calibration() and revert_calibration()
    • maybe make a helper type to avoid repeating NDArray[np.int64] | NDArray[np.float64] | list[int] | list[float] everywhere
  • make mypy (or some other tool) actually check the relevant files, and make it easy to gradually grow the mypy coverage to more of the codebase

A follow-up PR could then do:

  • apply the pattern to the other places that implement the MotorsBus protocol implicitly
  • also apply the pattern to the Robot protocol

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.

  • select the lerobot conda envrionment in vscode, if you haven't already (might need to enable some settings to get all of the type checking turned on. Please tell me if this doesn't work for you and I will go digging)
  • open feetech.py and revert some of the type fixes that I made (in this example, I removed the return type annotation from apply_calibration(), so the type checker thinks it returns None.
  • scroll to the bottom and notice that the type assertion has an error

Screenshot 2024-11-16 at 18 28 43

  • hover over it to see the description of the error

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.

In the Robot class, we have the comment:

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

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:

- [ ] clean up the `values` type and return type for apply_calibration() and revert_calibration()
  - [ ] maybe make a helper type to avoid repeating `NDArray[np.int64] | NDArray[np.float64] | list[int] | list[float]` everywhere
- [ ] make mypy (or some other tool) actually check the relevant files, and make it easy to gradually grow the mypy coverage to more of the codebase

A follow-up PR could then do:
- [ ] apply the pattern to the other places that implement the MotorsBus protocol implicitly
- [ ] also apply the pattern to the Robot protocol
@alsuren alsuren changed the title POC: Have a go at fixing class MotorsBus(Protocol) PoC: Have a go at fixing class MotorsBus(Protocol) Nov 16, 2024
@alsuren
Copy link
Author

alsuren commented Nov 16, 2024

From discussions on discord, it sounds like we're wanting to rethink the protocol classes anyway (https://discord.com/channels/1216765309076115607/1237737604611571722/1307416358417469460 - #493 (comment)). 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.

@alsuren alsuren closed this Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant