Add mypy and static type checking to Habitat Lab #492
Conversation
facebook-github-bot
added
the
CLA Signed
|
@erikwijmans @mathfac a huge pain problem here is the our core classes do not defined any call signatures which is making overloading them quite annoying as I have to add ignores all for every single sensor or measure. We should standardized a Sensor / Measurement API to fix this. See incompatible overrides for more info: https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides |
|
I added enough updated typehints and ignore comments for the bare minimum of it to stop complaining. We tend to extend classes in very non-pythonic ways around Habitat-Lab. We also should really fix the call signature of the base Sensor and Measure class so that it's more consistent. |
Skylion007
changed the title
There was a problem hiding this comment.
I get the motivation of doing this, but right now there are so many # type: ignore's floating around that I am not sure if the value add vs. PITA ratio is right.
Maybe we start with typing and enforcing typing in at least habitat/core and maybe habitat and slowly expand it?
|
@mosra Really confused how this broke our doc build. Any ideas? Was it the abstract class constructor? |
|
Heh. Looks like the newly added hints hit some corner case in the doc generator, and it might be some Py3.6-specific thing, even. I suppose you don't have the doc build running locally so you could run I'll check this tomorrow, please ping me on Slack in case I forget ;) |
|
Found the culprit and made a workaround in m.css, it's something deep inside undocumented Python |
|
@Skylion007 Fix is in mosra/m.css@0b94575. Pull the |
|
@erikwijmans Any further reduction in type ignores requires us to remove most of the args / kwargs passing in |
|
@erikwijmans Great news, a version of MyPy just dropped yesterday that allows me to do global/module level ignores just like Flake8 does. This should solve the issue. I also went back and removed most of the type ignores and replaced with explicit PEP528 casting where I could or defined the missing attributes on the parent objects. |
Co-authored-by: Erik Wijmans <etw@gatech.edu>
This reverts commit acf9fd8.
|
Whoops, didn't mean to push that. |
|
Grr, just realized a bug where all the Habitat_Sim type imports now block a soft install of habitat_lab without habitat_sim. We need to create a CI Test for having habitat-lab work without habitat_sim. Throughts @erikwijmans @mathfac ? |
|
Fixed in #498 |
* Add SceneNode attachment option to addObject and makeRigidObject. * Added RigidObject::visualNode_ to clearly separate Drawable branch of a physical object's SceneNode hierarchy. * Enabled optional deletion of visualNode_ and object SceneNode hierarchy when removing objects from PhysicsManager. * Added python bindings and unit tests in python and C++.
* typed Habitat Core * Fix more habitat typing issues * Fix objnav typecheck * Fix incorrect config type * complete habitat lab mypy conversion * Migrate mypy config to mypy.ini * Remove unused mypy config values * Re-enable episodes setter * Finish typing habitat_baselines * Stricten type hints in habitat.core.simulator * Fix registry type add constructor to Simulator * Fix import error * Make max singleton * Remove list copy * Update mypy with global ignores * Do better list casting * Update pre-commit hooks * unify pre-commit style * Add check-yaml pre-commit hook * Bugfix and optimize ci tests * Remove one more type ignore * Update habitat_baselines/rl/ppo/ppo.py * Fix ppo imports * remove dead code * Revert "remove dead code" This reverts commit acf9fd8.
* typed Habitat Core * Fix more habitat typing issues * Fix objnav typecheck * Fix incorrect config type * complete habitat lab mypy conversion * Migrate mypy config to mypy.ini * Remove unused mypy config values * Re-enable episodes setter * Finish typing habitat_baselines * Stricten type hints in habitat.core.simulator * Fix registry type add constructor to Simulator * Fix import error * Make max singleton * Remove list copy * Update mypy with global ignores * Do better list casting * Update pre-commit hooks * unify pre-commit style * Add check-yaml pre-commit hook * Bugfix and optimize ci tests * Remove one more type ignore * Update habitat_baselines/rl/ppo/ppo.py * Fix ppo imports * remove dead code * Revert "remove dead code" This reverts commit acf9fd8.
Motivation and Context
Actually uses all the TypeHints we have in Habitat-Lab by adding a pre-commit hook for them.
Before doing that, there are quite a lot of type signature errors that need to be fixed in habitat.
This actually required quite a lot of refactoring just to get the base habitat to use properly use these type hints and to fix existing errors in typing.
I had to add quite a few "ignores" to fix some pretty serious issues we have with inheritance and overloading variables / types of Python classes.I disabled the inheritance errors with a brand new feature of mypy, we should fix it longterm, but this solution should suffice for now.As of this typing, I have fixed all except the eqa_task and tasks/nav/nav.py. These two classes alone account for 84 typing errors in Habitat, currently.The only area I haven't touched is the slam agents in the habitat_baselines since we don't interface a lot with that baseline code.How Has This Been Tested
With PyTest and MyPy
Like with Habitat-Sim I used monkeytype to help out, but since so many existing type hints were wrong and the fact that we have class names that are the same throughout habitat, it required quite a large number of manual changes so far.
Types of changes
Checklist