Add fallback equality relationship based on uuid

[ramirezfranciscof]

Related to #1917 (I wouldn't necessarily say that it fixes it since there seem to be other aspects that still merit discussion, perhaps we could implement some kind of "long term discussion" tag or something like that?)

Basically adds the behaviour summarized in this comment:

As a last fallback, compare by UUID instead of just returning False. If there is an existing implementation in terms of values, we leave it as is (for now!). Of course, any implementation that compares by value needs to compare the same node as equal if it's to be any use.

I'm tagging @greschd for review since this is basically a re-upload of my previous PR that he already saw and I am also tagging @giovannipizzi for confirmation that it is ok for now to add the fallback and then (at a later time maybe) do a more thorough check with users to see if this should be the case for all types of node.

[ramirezfranciscof]

Sorry @greschd, I didn't see the failing test before due to the problem with test_pause_play_kill. I'll add you as reviewer again when I solve this.

[ramirezfranciscof]

Oooook, I may need a bit of help with this now @greschd @sphuber @giovannipizzi because somehow setting the __eq__ messes with the serializer, but I don't understand how that part of the code works.

The failing test is pytest tests/engine/test_ports.py::TestPortNamespace:

======================================================= FAILURES =======================================================
_____________________________________ TestPortNamespace.test_serialize_type_check ______________________________________
TypeError: unhashable type: 'Dict'

During handling of the above exception, another exception occurred:

self = <tests.engine.test_ports.TestPortNamespace testMethod=test_serialize_type_check>

    def test_serialize_type_check(self):
        """Test that `serialize` will include full port namespace in exception message."""
        base_namespace = 'base'
        nested_namespace = 'some.nested.namespace'
        port_namespace = PortNamespace(base_namespace)
        port_namespace.create_port_namespace(nested_namespace)
    
        with self.assertRaisesRegex(TypeError, f'.*{base_namespace}.*{nested_namespace}.*'):
>           port_namespace.serialize({'some': {'nested': {'namespace': {Dict()}}}})
E           AssertionError: ".*base.*some.nested.namespace.*" does not match "unhashable type: 'Dict'"

tests/engine/test_ports.py:95: AssertionError
=============================================== short test summary info ================================================
FAILED tests/engine/test_ports.py::TestPortNamespace::test_serialize_type_check - AssertionError: ".*base.*some.neste...

Any tips?

[greschd]

I'm OOF until next week. Maybe someone else can help in the meantime, otherwise feel free to ping me again Monday.

[ramirezfranciscof]

otherwise feel free to ping me again Monday

@greschd Helloo there...

[greschd]

General Ramirez!

Hmm, it's a tricky one. The immediate problem here is that defining an __eq__ function "removes" the default __hash__ function:

In [1]: class Foo:
   ...:     pass
   ...:

In [2]: hash(Foo())
Out[2]: 8786701317289

In [3]: class Bar:
   ...:     def __eq__(self, other):
   ...:         return True
   ...:

In [4]: hash(Bar())
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-d413494abdfb> in <module>
----> 1 hash(Bar())

TypeError: unhashable type: 'Bar'

The reason for that is that __eq__ and __hash__ need to be consistent, in that if __eq__ returns True the __hash__ must be the same.

What I'm not sure about is.. do we actually need Node to have a __hash__ (in the Python sense, not our own hashing mechanism)? To me, the test that fails looks like a typo: {Dict()} is a set, but should probably be a dictionary {'key': Dict()}. Maybe @sphuber can comment on that. But then, if we remove the __hash__ it could have unintended consequences.. maybe someone somewhere was relying on that?
We could just decide to implement a __hash__ alongside the __eq__ -- but of course the classes which already have an __eq__ never had a __hash__, and for those it'd be a bit complicated to fulfill that consistency requirement.

[greschd]

Unrelated to the above, I think the __eq__ implementation should only return True when the two UUIDs match, and refer to the super() method in all other cases. In other words, it shouldn't explicitly return False.

[ramirezfranciscof]

What I'm not sure about is.. do we actually need Node to have a __hash__ (in the Python sense, not our own hashing mechanism)? To me, the test that fails looks like a typo: {Dict()} is a set, but should probably be a dictionary {'key': Dict()}. Maybe @sphuber can comment on that. But then, if we remove the __hash__ it could have unintended consequences.. maybe someone somewhere was relying on that?

If I understand correctly, the __hash__ method needs to be implemented in order to be able to use nodes as keys in dictionaries or (perhaps more important) inside sets, correct? I think we at least would want to support that second use case.

We could just decide to implement a __hash__ alongside the __eq__ -- but of course the classes which already have an __eq__ never had a __hash__, and for those it'd be a bit complicated to fulfill that consistency requirement.

But if implementing an __eq__ method overrides the __hash__, why didn't the derived types have this problem, if they also were missing and explicit __hash__?

Unrelated to the above, I think the __eq__ implementation should only return True when the two UUIDs match, and refer to the super() method in all other cases. In other words, it shouldn't explicitly return False.

Mmm, I'm not an expert on how the super() works; do you mean something like this?

    def __eq__(self, other):
        """Fallback equality comparison by uuid (can be overwritten by specific types)"""
        if isinstance(other, Node):
            return self.uuid == other.uuid
        return super(Node, self).__eq__(other)

This is just a formality though, I assume. The last fallback is "they are the same variable", which if they were they would have the same UUID...no?

[greschd]

If I understand correctly, the __hash__ method needs to be implemented in order to be able to use nodes as keys in dictionaries or (perhaps more important) inside sets, correct?

Yes, that's right.

I think we at least would want to support that second use case.

Well, I'm not sure. You'll find that currently that works for "plain" Node, but for example Int does not support it.

But if implementing an eq method overrides the hash, why didn't the derived types have this problem, if they also were missing and explicit hash?

It doesn't override the default __hash__, it removes it. The default __hash__ exists only if there is no explicitly defined __eq__. That's because the default (hash by object id, basically) doesn't necessarily fulfill that "a == b implies hash(a) == hash(b)" requirement anymore.

Mmm, I'm not an expert on how the super() works; do you mean something like this?

Almost, but the self.uuid != other.uuid case should also go to the super():

    def __eq__(self, other):
        """Fallback equality comparison by uuid (can be overwritten by specific types)"""
        if isinstance(other, Node) and self.uuid == other.uuid:
            return True
        return super().__eq__(other)

This is just a formality though, I assume. The last fallback is "they are the same variable", which if they were they would have the same UUID...no?

Hmm... maybe? The super() call could also return NotImplemented, in which case other gets the chance to decide if they are equal. Just checked for built-in int and float, and that's indeed how it works there: The float knows how to compare to int, but not the other way around.

In [1]: x = 1.                                                                  

In [2]: y = 1                                                                   

In [3]: x.__eq__(y)                                                             
Out[3]: True

In [4]: y.__eq__(x)                                                             
Out[4]: NotImplemented

So clearly that's a correct way of doing things, but would break if our __eq__ fallback doesn't forward to super(). We could also just return NotImplemented instead of the super() call, just because we know that this is the highest point in the class hierarchy where we implement __eq__. I like the super() better because that knowledge isn't required.

[sphuber]

What I'm not sure about is.. do we actually need Node to have a __hash__ (in the Python sense, not our own hashing mechanism)? To me, the test that fails looks like a typo: {Dict()} is a set, but should probably be a dictionary {'key': Dict()}. Maybe @sphuber can comment on that.

It must indeed be a typo, since port namespaces do not allow sets as values, they need keys. So anyway this should probably be fixed by adding a simple key.

[greschd]

Thanks @sphuber - do you have an opinion on getting rid of the __hash__ for nodes?

Quick summary, if we add an __eq__ the Python built-in __hash__ is no longer defined; so this would be a change in behavior for all nodes that didn't previously have an explicitly-defined __eq__.

[sphuber]

As far as I understand, we don't really have much of a choice here though do we? If we want to add __eq__ as proposed in this PR, we have to add a __hash__ that always agrees with __eq__. The easiest would be to give it the identical implementation. This would indeed change the behavior because previously only two nodes with identical object id would match, and hereafter they would match as long as their AiiDA UUID match. I guess this change in behavior is acceptable since this is anyway what we are introducing effectively with the __eq__ implementation, so if that isn't a problem, the hash shouldn't be either I guess?

[greschd]

Yeah, the other option would be dropping the __hash__.

One subtlety to keep in mind here is that for an existing __eq__ creating a __hash__ based only on the UUID is incorrect: For example, two Int with the same value need to have the same __hash__. For the BaseType we could implement that by just deferring to Python's implementation.
For the general case it's more difficult.. we could come up with some black magic that detects if there's a "higher-level" __eq__, but I'm not sure that's a great idea.
Another possible option is to just e.g. return 0 in __hash__. That makes dictionaries, etc. inefficient because all objects go into one "bucket" and it needs to use comparison to distinguish them. But it is a correct implementation of __hash__.

[sphuber]

One subtlety to keep in mind here is that for an existing __eq__ creating a __hash__ based only on the UUID is incorrect: For example, two Int with the same value need to have the same __hash__.

Is that conceptually or merely because we implemented __eq__ based on the underlying value? Because if two data nodes with the same "content" should have the same hash, then this is already not the case now. For example:

d1 = Dict(dict={'a': 1})
d2 = Dict(dict={'a': 1})
hash(d1) != hash(d2)

I am also not sure whether people would expect {d1, d2} to contain 1 or 2 nodes. I guess this comes back the original discussion of when a node is supposed to be considered equal to another.

[codecov]

Codecov Report

Merging #4753 (0a03f65) into develop (d27e22d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4753      +/-   ##
===========================================
+ Coverage    79.55%   79.56%   +0.01%     
===========================================
  Files          515      515              
  Lines        36824    36831       +7     
===========================================
+ Hits         29292    29300       +8     
+ Misses        7532     7531       -1     

Flag	Coverage Δ
django	74.25% <100.00%> (+0.01%)	⬆️
sqlalchemy	73.15% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/orm/nodes/node.py	83.08% <100.00%> (+0.27%)	⬆️
aiida/transports/plugins/local.py	81.80% <0.00%> (+0.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d27e22d...295c92f. Read the comment docs.

[greschd]

Is that conceptually or merely because we implemented __eq__ based on the underlying value?

Only for the cases where __eq__ takes into account the content: The relation "a == b implies hash(a) == hash(b)" needs to hold. From the docs :

The only required property is that objects which compare equal have the same hash value

Reading that paragraph, I just noticed:

If a class that overrides __eq__() needs to retain the implementation of __hash__() from a parent class, the interpreter must be told this explicitly by setting __hash__ = <ParentClass>.__hash__.

Which means things are actually not as complicated as I had imagined: The __hash__ for e.g. BaseType object is still implicitly set to None even if we implement it in Node. So we can just use the UUID in the __hash__ implementation after all!

In [1]: class Foo:
   ...:     def __eq__(self, other):
   ...:         return True
   ...:     def __hash__(self):
   ...:         return True
   ...:

In [2]: class Bar(Foo):
   ...:     def __eq__(self, other):
   ...:         return True
   ...:

In [3]: Foo() == Bar()
Out[3]: True

In [4]: {Foo()}
Out[4]: {<__main__.Foo at 0x7f44067211d0>}

In [5]: {Bar()}
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-5-bb872cf7de42> in <module>
----> 1 {Bar()}

TypeError: unhashable type: 'Bar'

[ramirezfranciscof]

So, do you mean the following?

class Node(...):

    def __hash__(self):
        return self.uuid
        
class BaseType(Data):

    __hash__ = Data.__hash__

Edit: mmm no wait, that hash for the BaseType nodes would be inconsistent with the equality. I'm not sure why you mention the inheritance issue of the __hash__ then...

[greschd]

    def __hash__(self):
        return self.uuid

It needs to be converted to an int, but yes. I think the conversion can be done by uuid.UUID(self.uuid).int.

class BaseType(Data):
    __hash__ = Data.__hash__

Edit: mmm no wait

Exactly, we shouldn't add this. I hadn't realized initially that this only happens when specified explicitly -- __hash__ is different from "normal" Python inheritance in that regard.

[ramirezfranciscof]

Exactly, we shouldn't add this. I hadn't realized initially that this only happens when specified explicitly -- __hash__ is different from "normal" Python inheritance in that regard.

Haha, ok, I'm not sure I fully understand then how the __hash__ inheritance work or why is it ok to leave our intrinsic python nodes with __eq__ but no __hash__ (and let them fail when trying to make sets with these?), but ok. I think this is now ready to review.

[greschd]

why is it ok to leave our intrinsic python nodes with __eq__ but no __hash__

Simply because that's the status quo, so there's no harm in leaving it that way. If we decide they should be hashable it would be easy enough to just use hash(self.value) for those.

[greschd]

Thanks @ramirezfranciscof! Looks good, just some minor comments.

[ramirezfranciscof]

Ah, I was just about to merge it, you beat me while I was writing the commit message, haha!

Thanks @greschd and @sphuber ! Learnt some interesting stuff about python with this!

[greschd]

Ah, I was just about to merge it, you beat me while I was writing the commit message, haha!

Oops, I guess yours was more descriptive then 😅

[chrisjsewell]

Oops, I guess yours was more descriptive then 😅

Ha yeh @greschd I'm gonna have to give you a metaphorical slap on the wrist here 😆. Because I'm just writing the changelog for v1.6.0, and I note that all of your commit messages are leavng a bit to be desired 😜 (31cbd59, dffff84, 1c48d71)

[ramirezfranciscof]

@chrisjsewell mmm I don't know about 1c48d71 because I was not involved in that one, but for this one (31cbd59) and dffff84 I think the one liners would be ok for the changelog, no?

[31cbd59] Add fallback equality relationship based on node uuid.
[dffff84] Fix: pre-store hash for -0. and 0. is now the same

If you think they need more fleshing out, let me know what kind of information is missing and I could try to give you some one-liner replacements.

[chrisjsewell]

I think the one liners would be ok for the changelog, no?

short answer, no they are not. You'll see in my changelog soon 😉

[greschd]

Yeah, point taken. 😬

The SinglefileData commit is actually pretty straightforward. Most of it is converting the tests from AiidaTestCase to bare pytest - shoulda mentioned that in the commit message of course..

[greschd]

Guess each of these should've included some discussion as to why that change was done.

[chrisjsewell]

Guess each of these should've included some discussion as to why that change was done

https://github.com/aiidateam/aiida-core/wiki/Git-style-requirements#commit-messages 😄

[chrisjsewell]

not the end of the world, but no point falling at the final hurdle

[greschd]

More detailed explanatory text, if necessary.

Taking some liberty with the "if necessary" definition here 😅