latest overrides package breaks allennlp

[david-waterworth]

The version of overrides dependency seems to have been bumped rapidly from 3.1.0 on 18 Jun 2020 to 6.1.0 5 days ago (https://github.com/mkorpela/overrides/tags) with a release almost weekly. This seems to have added functionality to verify correctness of type annotations, which in turn throws exceptions on simple imports (see MRE below) which aren't annotated correctly such as ArrayField.empty_field has no return type annotation (should be the string 'ArrayField' since it returns an instance of it's class).

I had to downgrade overrides to 3.1.0, I "fixed" the issue below by adding the correct return type to the ArrayField.empty_field property (-> 'ArrayField' works) and a few other trivial examples but then it quickly snowballed so I gave up and manually rolled back.

from allennlp.data.fields import ArrayField

Traceback (most recent call last):
File "", line 1, in
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/allennlp/data/init.py", line 1, in
from allennlp.data.dataset_readers.dataset_reader import DatasetReader
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/allennlp/data/dataset_readers/init.py", line 10, in
from allennlp.data.dataset_readers.ccgbank import CcgBankDatasetReader
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/allennlp/data/dataset_readers/ccgbank.py", line 9, in
from allennlp.data.dataset_readers.dataset_reader import DatasetReader
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/allennlp/data/dataset_readers/dataset_reader.py", line 8, in
from allennlp.data.instance import Instance
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/allennlp/data/instance.py", line 3, in
from allennlp.data.fields.field import DataArray, Field
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/allennlp/data/fields/init.py", line 7, in
from allennlp.data.fields.array_field import ArrayField
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/allennlp/data/fields/array_field.py", line 10, in
class ArrayField(Field[numpy.ndarray]):
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/allennlp/data/fields/array_field.py", line 50, in ArrayField
@OVERRIDES
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/overrides/overrides.py", line 88, in overrides
return _overrides(method, check_signature, check_at_runtime)
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/overrides/overrides.py", line 114, in _overrides
_validate_method(method, super_class, check_signature)
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/overrides/overrides.py", line 135, in _validate_method
ensure_signature_is_compatible(super_method, method, is_static)
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/overrides/signature.py", line 93, in ensure_signature_is_compatible
ensure_return_type_compatibility(super_type_hints, sub_type_hints, method_name)
File "/home/david/.pyenv/versions/wiser/lib/python3.7/site-packages/overrides/signature.py", line 288, in ensure_return_type_compatibility
f"{method_name}: return type {sub_return} is not a {super_return}."
TypeError: ArrayField.empty_field: return type None is not a <class 'allennlp.data.fields.field.Field'>.

[david-waterworth]

Also see mkorpela/overrides#77

[epwalsh]

Thanks for bringing this up and doing the digging @david-waterworth. Sounds like upgrading overrides will be a little tedious 😒

I'm putting "Contributions welcome" on this for now because I don't think any of us will be able to get to it in the immediate future.

[david-waterworth]

In the interim would it make sense to update setup.py to require overrides<=3.1.0? Otherwise, anyone who installs allennlp from pip gets a broken install atm.

It also looks like this error only occurs on methods that are overrides, and it appears you can suppress the type checking by adding check_signature=False so an interim step may be to grep all @overides decorators and update them.

[epwalsh]

Oh, you must be using a really old version of allennlp then? We've had it pinned since 1.0.

[david-waterworth]

Yes I'm using a third-party library that had pinned to 0.8.4, which is part of the reason for my confusions as the library is fairly new so I'm not sure why they pinned such an old version of allennlp. I've asked them to update but it's code linked to a published paper so I think they probably wont want to.

I only just realised that the latest release of allennlp does pin overrides so ignore my comment.

[ogunoz]

@david-waterworth Same here. I had to use allennlp 0.8.3 and i had the latest overrides version by default. Downgrading overrides library to 3.1.0 solved it. Thanks for your helpful comments

[HOZHENWAI]

Hello, I'd like to contribute to this issue. I just have some questions about the scope of the changes that are "authorized".
I'm pretty we don't just want a check_signature=False flag on the overriders decorator.

Are change on parent classes authorized or not? (for example for the dataset_reader class)

def _read(self, file_path) -> Iterable[Instance]:
        """
        # NOTE: `file_path` is left untyped here on purpose.
        # Technically the type should be `DatasetReaderInput`, but many subclass
        # implementations of `DatasetReader` define their `_read()` method to take a more
        # specific type, such as just `str`. But that would be a type error
        # according to mypy: https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
        """

Without modifying the parent signature, the only solution would be to remove the type declaration for child classes or forcing the flag.

[epwalsh]

IMO DatasetReader should really be a Generic class, so you'd have something like this:

class DatasetReader(Generic[T]):
    ...
    def _read(self, input: T) -> Iterable[Instance]:
        pass

That would probably resolve typing/overrides issues.

[ksteimel]

Can this be closed in light of #5490 being merged?

[epwalsh]

Yes, thanks @ksteimel