[`mypy`] Implicit re-imports in `trieste.acquisition.__init__`
Opened this issue · 4 comments
See this:
from abc import ABC
from trieste.acquisition import AcquisitionFunctionClass
class MyAF(AcquisitionFunctionClass, ABC):
pass
Running mypy --strict
on this yields:
error: Module "trieste.acquisition" does not explicitly export attribute "AcquisitionFunctionClass" [attr-defined]
error: Class cannot subclass "AcquisitionFunctionClass" (has type "Any") [misc]
Changing the import to
from trieste.acquisition.interface import AcquisitionFunctionClass
solves the problem, but then the trieste.acquisition.__init__
is pointless.
How to solve
Add __all__ = ["AcquisitionFunctionClass", ...]
(and everything that needs to be reimported) to the trieste.acquisition.__init__
.
Need to decide whether this is worthwhile or not.
Pros
- makes
mypy --strict
work out of the box.
Cons
- requires lots of code duplication (1000s of
__init__.py
reexports must be specified twice, once as a string) - no obvious code quality benefit (what sort of error are we actually protecting against here?)
- easy to work around by disabling
--no-implicit-reexport
, including just for__init__.py
files
requires lots of code duplication (1000s of init.py reexports must be specified twice, once as a string)
This is not true - this behaviour is not code duplication, this is being explicit on what trieste
wants to reexport.
easy to work around by disabling --no-implicit-reexport, including just for init.py files
Where would that flag go? Not sure how a user of trieste
would solve that for trieste
's init.
no obvious code quality benefit (what sort of error are we actually protecting against here?)
See this:
error: Class cannot subclass "AcquisitionFunctionClass" (has type "Any") [misc]
which originates from the same issue.
We'll probably want to fix this in gpflux before fixing it in trieste. AFAICT gpflow already uses __all__
.
Also we'd probably want to generate and verify __all__
programmatically as there are 100s or 1000s of API definitions reexported from the modules they're implemented in. Adding no_implicit_reexport = true
to mypy.ini will catch the ones we use internally but people will inevitably sometimes forget to update __all__
when adding new definitions and fixing this problem by making the code less maintainable is not obviously a net gain.