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

Numpy-related type errors are hidden by the absence of numpy stubs #347

Closed
apaleyes opened this issue Sep 13, 2021 · 2 comments · Fixed by #521
Closed

Numpy-related type errors are hidden by the absence of numpy stubs #347

apaleyes opened this issue Sep 13, 2021 · 2 comments · Fixed by #521
Labels
bug Something isn't working

Comments

@apaleyes
Copy link
Collaborator

Describe the bug
There is a bunch of type errors in the code base that mypy runs never reveal because Trieste is relying on numpy 1.19.x which isn't typed yet. If you run mypy with numpy >= 1.20, 46 new type errors surface.

Ideal fix would be to move to newer version of numpy. The errors will then fail the builds and force us to fix them. Unfortunately numpy 1.19.x seems to be forced by Tensorflow and using one numpy version for type checks while using older one for everything else feels weird.

We could also consider installing numpy stubs separately via numpy-stubs or data-science-types, but both of these projects are archived, so it's probably best not to rely on them.

To reproduce
We will be using tox here.

  1. Start with a new fresh clone of Trieste, make sure there is no .tox cache. Your venv should be running python3.7 and only have tox installed. This is how GitHub Action for type checking is setup.
  2. Add numpy and matplotlib to tox requirements:
    echo -e "numpy>=1.20\nmatplotlib" >> common_build/types/requirements.txt
  3. run type checks with tox
    tox -e types
  4. 46 errors show up: mypy_errors.txt

Expected behaviour
No errors

System information

  • OS: Ubuntu 18
  • Python version: 3.7.5
  • Trieste version: git clone
@apaleyes apaleyes added the bug Something isn't working label Sep 13, 2021
@uri-granta
Copy link
Collaborator

Due to TF version restriction, it might be a little while before we upgrade. Looking at the errors there doesn't seem to be anything worrying. By far the most common error is using a TensorType result (e.g. from sample) as if it was necessarily a tf.Tensor (which it is in practice, but that's not what the type says). I've raised a PR to discuss a possible fix.

@uri-granta
Copy link
Collaborator

uri-granta commented Feb 16, 2022

Looking at this again, as we'll need to fix these typing issues at some point to upgrade tf and numpy.

Fixed some of the simpler errors in #507. However, the main problem will be our use of TensorType = Union[np.ndarray, tf.Tensor], especially in AcquisitionFunction = Callable[[TensorType], TensorType]. Without numpy and tensorflow stubs this is essentially just TensorType = Any, meaning we get away with:

  1. not always handling inputs that are ndarrays
  2. assuming outputs are always tensors (which they are in practice)

There are a few ways to handle this:

  1. Always use helper functions such as to_numpy on outputs rather than calling .numpy() directly
  2. Redefine all of our interfaces so that they only accept and return tensors. Note that the current absence of tensorfllow stubs means that redefining TensorType = tf.Tensor would bring us back to the current TensorType = Any scenario.
  3. Redefine our interfaces to only return tensors, but allow some to still accept ndarrays (WHICH?)

@uri-granta uri-granta linked a pull request Feb 28, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants