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

Bug in BidirectionalMapping.__subclasshook__(): Any class with an inverse attribute is considered a collections.abc.Mapping #111

Closed
amol447 opened this issue Jul 21, 2020 · 8 comments

Comments

@amol447
Copy link

amol447 commented Jul 21, 2020

Importing bidict causes the tfp.sts.Autoregressive class to not work at all. Simplest code to reproduce the problem is as follows
Not sure if the issue is with tfp or bidict.

    import bidict  
    import tensorflow_probability as tfp  
    a=tfp.sts.Autoregressive(order=2)  
AttributeError: 'LinearOperatorDiag' object has not attribute 'keys'

bidict :0.19.0
tf : 2.2.0
tfp:0.10.0
see tensorflow/probability#1016

@amol447
Copy link
Author

amol447 commented Jul 21, 2020

I think I found the issue. It is in https://github.com/jab/bidict/blob/master/bidict/_abc.py in the subclasshook method.
NotImplemented is somehow not None so the check on line 108 fails to do what it should be doing and any class with an inverse method defined on it becomes a subclass of collections.abc.Mapping.

import bidict
import collections.abc as cabc
class DummyWithInverse(object):
  def inverse(self, x):
    return x
issubclass(DummyWithInverse,cabc.Mapping)
True

jab added a commit that referenced this issue Jul 22, 2020
jab added a commit that referenced this issue Jul 22, 2020
...that caused classes with an `inverse` attribute to be incorrectly
considered subclasses of `collections.abc.Mapping`.

Fixes #111.
@jab
Copy link
Owner

jab commented Jul 22, 2020

Sorry for the bug and thanks so much for the helpful repro! Pushed a fix to the abcfix branch. Would you be willing to test your code with this version to confirm the fix works for you?

@amol447
Copy link
Author

amol447 commented Jul 22, 2020

While this does fix the issue for me, I think the fix is potentially dangerous because we never actually explicitly check if C is a subclass of collections.abc.Mapping so it might rear it's ugly head again?(not sure)
I tried

If NotImplemented==Mapping.__subclasshook__(C):
instead of what's in the original code
if not Mapping.__subclasshook__(C):

and that also seems to work for me but ofc may be even that isn't real fix since I don't have the whole bidict project running with unit tests etc.

@jab
Copy link
Owner

jab commented Jul 22, 2020

Thanks for confirming my fix!

The alternative changes you're proposing[1] are insufficient. Consider the following:

>>> from collections.abc import Mapping
>>> class MyMapping(Mapping):
...     pass
...
>>> Mapping.__subclasshook__(MyMapping)
NotImplemented
>>> issubclass(MyMapping, Mapping)
True

So we can't bail early if the Mapping.__subclasshook__(C) check returns NotImplemented. (And we can't actually call issubclass(C, Mapping) there because it triggers RecursionError for several cases.)

In particular, this would break the following case:

>>> class VirtualBidirectionalMapping(collections.abc.Mapping):
...    def inverse(self):
...        return ...
...
>>> issubclass(VirtualBidirectionalMapping, BidirectionalMapping)
True

In other words, if we want to preserve the existing behavior, where any Mapping with an inverse attribute is considered a BidirectionalMapping, we have to take the approach in my fix: check for all the required attributes of the Mapping interface, plus an inverse attribute.

This is the same approach taken by Iterator.__subclasshoook__(). See:
https://github.com/python/cpython/blob/v3.8.5/Lib/_collections_abc.py#L243-L275

Note that Iterator inherits from Iterable. But rather than calling Iterable.__subclasshook__() from Iterator.__subclasshook__(), Iterator.__subclasshook__() checks for the required attributes of Iterable (namely, __iter__), plus the additional required attributes of Iterator (namely, __next__).

I have lots of test cases exercising this fixed BidirectionalMapping.__subclasshook__() implementation, and added more to cover this bug. Can you please take a look at the following test cases and let me know if you can think of any I'm missing: https://github.com/jab/bidict/blob/abcfix/tests/test_class_relationships.py
(Note all those tests pass with the fix in my branch.)

Thanks!

[1] I think a complete version of what you proposed would actually be something like this:

diff --git a/bidict/_abc.py b/bidict/_abc.py
index 8f1b690..ce2dd98 100644
--- a/bidict/_abc.py
+++ b/bidict/_abc.py
@@ -105,8 +105,9 @@ class BidirectionalMapping(Mapping):  # pylint: disable=abstract-method,no-init
         """
         if cls is not BidirectionalMapping:  # lgtm [py/comparison-using-is]
             return NotImplemented
-        if not Mapping.__subclasshook__(C):
-            return NotImplemented
+        super_subclasshook = super().__subclasshook__(C)
+        if super_subclasshook in (NotImplemented, False):
+            return super_subclasshook
         mro = C.__mro__
         if not any(B.__dict__.get('inverse') for B in mro):
             return NotImplemented
         return True

jab added a commit that referenced this issue Jul 22, 2020
...that caused classes with an `inverse` attribute to be incorrectly
considered subclasses of `collections.abc.Mapping`.

Fixes #111.
jab added a commit that referenced this issue Jul 22, 2020
...that caused classes with an `inverse` attribute to be incorrectly
considered subclasses of `collections.abc.Mapping`.

Fixes #111.
@amol447
Copy link
Author

amol447 commented Jul 22, 2020

Makes sense. Thanks for fixing this fast.

@jab
Copy link
Owner

jab commented Jul 23, 2020

My pleasure, and thank you again for the helpful bug report.

Another option I’m considering here is to just remove the subclasshook. From polling users in the bidict Gitter chat, as well as a code search on GitHub, so far it appears to be unused, and so is just pure maintenance burden (especially since it requires such fiddly, subtle code to implement properly, and causes crazy bugs like this one if you don’t get it quite right).

I’m still thinking about it, but in the meantime, any objection from your side to dropping support for virtual BidirectionalMapping subclasses?

@amol447
Copy link
Author

amol447 commented Jul 23, 2020

+1 for removing subclasshook.

@jab jab changed the title Strange interaction between bidict and tensorflow Bug in BidirectionalMapping.__subclasshook__(): Any class with an inverse attribute is considered a collections.abc.Mapping Jul 23, 2020
jab added a commit that referenced this issue Jul 23, 2020
...due to lack of use and maintenance cost.

Fixes #111.
jab added a commit that referenced this issue Jul 23, 2020
...due to lack of use and maintenance cost.

Fixes #111.
@jab jab closed this as completed in 6232a67 Jul 23, 2020
jab added a commit that referenced this issue Jul 23, 2020
...due to lack of use and maintenance cost.

Fixes #111.
@jab
Copy link
Owner

jab commented Jul 23, 2020

I just released v0.20.0 to PyPI with a fix for this issue (removing BidirectionalMapping.__subclasshook__ altogether). Please update and let me know if you have any issues.

Thanks again, @amol447!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants