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

__hash__() is squashed on subclass #2422

Closed
3 tasks done
khaeru opened this issue Feb 27, 2021 · 6 comments · Fixed by #2423
Closed
3 tasks done

__hash__() is squashed on subclass #2422

khaeru opened this issue Feb 27, 2021 · 6 comments · Fixed by #2423
Labels
bug V1 Bug related to Pydantic V1.X
Milestone

Comments

@khaeru
Copy link

khaeru commented Feb 27, 2021

Checks

  • I added a descriptive title to this issue
  • I have searched (google, github) for similar issues and couldn't find anything
  • I have read and followed the docs and still think this is a bug

Bug

The following code worked in pydantic 1.7.3 (version info shown below), but fails with pydantic 1.8:

from pydantic import BaseModel

class Foo(BaseModel):
    x: int

    # Failed attempt to fix; see below
    # __slots__ = ("__hash__",)

    def __hash__(self):
        return self.x ** 2

f = Foo(x=2)
assert hash(f) == 4

class Bar(Foo):
    y: float

b = Bar(x=2, y=1.1)

try:
    assert hash(b) == 4
except TypeError as e:
    # TypeError: unhashable type: 'Bar'
    print(e)

It seems like this code prevents the use of Foo.__hash__ on Bar instances:
https://github.com/samuelcolvin/pydantic/blob/master/pydantic/main.py#L347

I tried to experiment with adding "__hash__" to __slots__, but an error is triggered in ModelMetaclass.__new__:

Traceback (most recent call last):
  File "bug.py", line 4, in <module>
    class Foo1(BaseModel):
  File "pydantic/main.py", line 352, in pydantic.main.ModelMetaclass.__new__
  File "/usr/lib/python3.8/abc.py", line 85, in __new__
    cls = super().__new__(mcls, name, bases, namespace, **kwargs)
ValueError: '__hash__' in __slots__ conflicts with class variable

I would expect that this continues to work as in 1.7.3, or that the documentation suggests how to avoid having these methods squashed.


Output of python -c "import pydantic.utils; print(pydantic.utils.version_info())":
Passing version:

$ python -c "import pydantic.utils; print(pydantic.utils.version_info())"
             pydantic version: 1.7.3
            pydantic compiled: True
                 install path: /home/khaeru/.local/lib/python3.8/site-packages/pydantic
               python version: 3.8.6 (default, Jan 27 2021, 15:42:20)  [GCC 10.2.0]
                     platform: Linux-5.8.0-43-generic-x86_64-with-glibc2.32
     optional deps. installed: ['typing-extensions']

Failing version:

$ python -c "import pydantic.utils; print(pydantic.utils.version_info())"
             pydantic version: 1.8
            pydantic compiled: True
                 install path: /home/khaeru/.local/lib/python3.8/site-packages/pydantic
               python version: 3.8.6 (default, Jan 27 2021, 15:42:20)  [GCC 10.2.0]
                     platform: Linux-5.8.0-43-generic-x86_64-with-glibc2.32
     optional deps. installed: ['typing-extensions']
@PrettyWood
Copy link
Member

Hello @khaeru and thanks for reporting with a code snippet 🙏

I made a quick fix. Feedback welcome :)

@khaeru
Copy link
Author

khaeru commented Feb 27, 2021

Thanks for the quick response!

I checked out the branch but was unable to compile and install it; but the added test case matches what I reported, so it looks fine to me.

@alcrene
Copy link

alcrene commented Apr 28, 2021

Here's a variation of this that still does not work with 1.8.1:

from pydantic import BaseModel

class Foo(BaseModel):
    x: int

    def __hash__(self):
        return self.x ** 2

f = Foo(x=2)
assert hash(f) == 4

class Bar(Foo):
    y: float
        
class Bar2(BaseModel):
    z: float
        
class Baz(Bar2, Bar):
    pass

b = Bar(x=2, y=1.1)
c = Baz(x=2, y=1.1, z=3.1)

# This works now thanks to #2423
assert hash(b) == 4

# But not this
try:
    assert hash(c) == 4
except TypeError as e:
    # TypeError: unhashable type: 'Baz'
    print(e)

Whether or not the current behaviour is desired, it is at least different from the one in 1.7.

To recover the 1.7 behaviour, this seems to work for me: Changing this line
https://github.com/samuelcolvin/pydantic/blob/5261fd05a0374b84ce2602d45990baf480fa2417/pydantic/main.py#L249
to

if base.__hash__ is not None:
    hash_func = base.__hash__

@PrettyWood
Copy link
Member

@alcrene Try with dataclass it will raise the same error and IMO it's expected

@alcrene
Copy link

alcrene commented Apr 29, 2021

@PrettyWood khaeru's original example also fails with dataclass:

  • If Foo and Bar are not frozen, then Bar is not hashable.
  • If Foo and Bar are frozen, then Bar uses the default method which simply hashes the fields as a tuple (and so fails the assertion)

The reasoning seems clear enough: the assumptions underlying the __hash__ method of the parent class may no longer be true for the child, so the only safe thing to do is disable it by default.

To be clear, I have no strong opinion here – I'm just noting a change in behaviour that I had forgotten I was relying upon. IMO this is a corner case, and if people want reliable __hash__ methods, they should probably implement them in each subclass (as I am going to now ;-) ). The previous behaviour of 1.8.0 is perhaps the most consistent with dataclass and the expectations of a __hash__, and that of 1.7 perhaps most consistent with the expectations of a normal method, but I think leaving things as they are is also fine.

@153957
Copy link

153957 commented Aug 25, 2021

IMO this is a corner case, and if people want reliable __hash__ methods, they should probably implement them in each subclass (as I am going to now ;-) )

Thanks for that tip. We encountered the same issue, and this is indeed the way to make it work now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug V1 Bug related to Pydantic V1.X
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants