[mypy] Implicit re-imports in trieste.acquisition.__init__

[Corwinpro]

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__.

[uri-granta]

Need to decide whether this is worthwhile or not.

Pros

1. makes mypy --strict work out of the box.

Cons

1. requires lots of code duplication (1000s of __init__.py reexports must be specified twice, once as a string)
2. no obvious code quality benefit (what sort of error are we actually protecting against here?)
3. easy to work around by disabling --no-implicit-reexport, including just for __init__.py files

[Corwinpro]

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.

[uri-granta]

We'll probably want to fix this in gpflux before fixing it in trieste. AFAICT gpflow already uses __all__.

[uri-granta]

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.