google-deepmind / mujoco

Multi-Joint dynamics with Contact. A general purpose physics simulator.
https://mujoco.org
Apache License 2.0
8.2k stars 819 forks source link

Python bindings do not work well with type checkers #244

Open ikamensh opened 2 years ago

ikamensh commented 2 years ago

Hello,

Problem

in the intro page of python bindings, example has following code: model = mujoco.MjModel.from_xml_string(XML, ASSETS)

It triggers a warning in pycharm type checker: "Parameter 'assets' unfilled", because as of mujoco 2.1.5 the type hint for from_xml_string thinks it's not a static method:

    def from_xml_string(self, xml, assets, Dict=None, p_str=None, bytes=None, *args, **kwargs): # real signature unknown; NOTE: unreliably restored from __doc__ 
        """
        from_xml_string(xml: str, assets: Optional[Dict[str, bytes]] = None) -> mujoco._structs.MjModel

        Loads an MjModel from an XML string and an optional assets dictionary.
        """
        pass

Motivation for fixing this

To avoid bugs in the code, I want to use type checkers with mujoco python bindings.


Is there a way to ship correct signatures?

saran-t commented 2 years ago

We've had mixed success with auto-generated type stubs for internal usage. The .pyi stubs were generated using mypy, but the tool gets confused in a few places and we have a separate script that performs specific textual replacements on the output.

Perhaps try mypy and let us know how well that works for you?

ikamensh commented 2 years ago

Not sure I've installed it right, but it fails to find stubs for mujoco:

(venv) ~/P/Mujee $ mypy -c "import mujoco"                                                                                                         
<string>:1: error: Cannot find implementation or library stub for module named "mujoco"
<string>:1: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 1 source file)

I also don't see how mypy can accept signature containing self (non-static method) being used as a static method.

ikamensh commented 2 years ago

if you intend for type checkers to use sources for types, you should include py.typed file in the source tree. https://blog.whtsky.me/tech/2021/dont-forget-py.typed-for-your-typed-python-package/

My IDE resolves to mujoco/_structs/MjModel.py -> class MjModel when finding signature for mujoco.MjModel.from_xml_string

saran-t commented 2 years ago

Technically we currently don't support type checkers, so it would be inappropriate to include a py.typed. If we do add .pyi files with our future releases we'll make sure that the package is appropriately structured to enable type checking.

mypy does often get confused when it comes to static methods, however that's not what's causing the error above. You're calling mypy which is used to check type rather than to generate type stubs. You need to use stubgen instead (which comes with mypy).