List: register the class with the to_aiida_type serializer

[sphuber]

Fixes #5141

This will allow users to pass a plain list as input for a port that
accepts List nodes and specified the to_aiida_type dispatch as
serializer. The value will be automatically converted to a List node.
This brings the functionality for List on par with the other base type
data node classes.

To make the newly added test work, the Dict class needed a change to allow comparing to normal dictionaries. This is added in a separate commit.

[mbercx]

Nice! Love the fact that you don't have to worry about passing in a List anymore. However, won't this change the way that Dicts compare? I.e. won't:

Dict(dict={'a': 1}) == Dict(dict={'a': 1})

Now return True? I'm all for it, but I do think this behaviour is different from before (also see the discussion in #4495).

[chrisjsewell]

Yeh cheers, but just stick to the list addition for this PR

[sphuber]

Forgot about that long open discussion about node equality. Agree that I shouldn't change that here. Still I think that the assert dict_node == dict_node.value should be non-controversial, right? I have updated the implementation to allow for this case, but to leave the original behavior when comparing node instances.

[mbercx]

Forgot about that long open discussion about node equality. Agree that I shouldn't change that here. Still I think that the assert dict_node == dict_node.value should be non-controversial, right? I have updated the implementation to allow for this case, but to leave the original behavior when comparing node instances.

I'm still in favour of actually making Dict nodes the same as List nodes (and other BaseTypes) with regard to comparisons. ^^ Maybe not change that here, but we should try and hash this out before v2.0, perhaps? If we can get everyone to pitch in at the next AiiDA team meeting, I'm happy to open a PR to fix #4495 after, based on whatever we decide.

[sphuber]

I'm still in favour of actually making Dict nodes the same as List nodes (and other BaseTypes) with regard to comparisons. ^^

I would have to revise the discussion once again and make a summary of the pro's and con's. I am tempted to agree with you but I remember there being strong con's that I cannot remember right now.

Maybe not change that here, but we should try and hash this out before v2.0, perhaps?

Whether we change it or not, I think it would be good either way to come to a decision. So indeed good to try and make an executive summary and get a poll going with a final decision.

[codecov]

Codecov Report

Merging #5142 (daecc11) into develop (35b5c34) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head daecc11 differs from pull request most recent head 2c8f7f3. Consider uploading reports for the commit 2c8f7f3 to get more accurate results

@@             Coverage Diff             @@
##           develop    #5142      +/-   ##
===========================================
+ Coverage    80.88%   80.90%   +0.03%     
===========================================
  Files          536      536              
  Lines        37073    37076       +3     
===========================================
+ Hits         29982    29993      +11     
+ Misses        7091     7083       -8     

Flag	Coverage Δ
django	75.75% <100.00%> (+0.03%)	⬆️
sqlalchemy	74.85% <100.00%> (+0.03%)	⬆️

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

Impacted Files	Coverage Δ
aiida/orm/nodes/data/dict.py	87.24% <100.00%> (+1.19%)	⬆️
aiida/orm/nodes/data/list.py	84.71% <100.00%> (+5.44%)	⬆️
aiida/engine/daemon/client.py	75.26% <0.00%> (-1.01%)	⬇️
aiida/orm/nodes/data/array/bands.py	76.99% <0.00%> (-0.47%)	⬇️
aiida/transports/plugins/local.py	81.41% <0.00%> (-0.25%)	⬇️
aiida/orm/nodes/process/calculation/calcjob.py	78.10% <0.00%> (+0.96%)	⬆️
aiida/engine/processes/calcjobs/tasks.py	65.78% <0.00%> (+1.01%)	⬆️
aiida/engine/processes/calcjobs/manager.py	86.73% <0.00%> (+3.54%)	⬆️

---

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 35b5c34...2c8f7f3. Read the comment docs.

[mbercx]

Whether we change it or not, I think it would be good either way to come to a decision. So indeed good to try and make an executive summary and get a poll going with a final decision.

Alright, I'll try to go through the whole discussion again and write a summary by Wednesday. Then we can schedule a small meeting after the group meeting with everyone interested in discussing this (probably won't have enough time during the AiiDA meeting).

[chrisjsewell]

Yeh I’m for whatever makes the user experience best, which is most likely comparing base types as there value. Just a pain, because there will probably be no way to deprecate in a nice way, and it will likely break someone’s code/plugin who has been relying on previous behaviour. I think perhaps a good way would be like sqlalchemy has done, you can add an environmental variable to turn on certain warnings. We could have this turn on a warning for any code that calls these __eq__ methods, so people can quickly identify what code they need to change, then turn it off again

[chrisjsewell]

(and this warning could be backported to v1)

[sphuber]

Alright, I'll try to go through the whole discussion again and write a summary by Wednesday. Then we can schedule a small meeting after the group meeting with everyone interested in discussing this (probably won't have enough time during the AiiDA meeting).

Would be good, if you manage in time, to send the summary by email, such that those who are interested can have a look before. It is not really possible to do this during the meeting. Not that we should have the discussion during the meeting, but it can be helpful for people to have the context beforehand.

[sphuber]

@chrisjsewell are there any objections left after I changed the __eq__ implementation to not change comparison of nodes but only with plain dicts?

@mbercx would also be good to make sure #4495 gets in soon, since we likely also want to add this in 2.0 now that we are on a campaign to make all base types equal. Did you want to do this yourself, or can I also go ahead and implement it?

[chrisjsewell]

Will have a double check soon, bare with me, I’m just in the middle of Croatia waiting for a bus lol

[mbercx]

@mbercx would also be good to make sure #4495 gets in soon, since we likely also want to add this in 2.0 now that we are on a campaign to make all base types equal. Did you want to do this yourself, or can I also go ahead and implement it?

I think I should find time to do this on Friday, or next week at the latest. Note that we should probably first agree on what the equality comparison between base types should be though, because making List and Dict BaseTypes changes the equality comparison behavior for Dict.

[sphuber]

@mbercx would addressing some of the feature requests really change the equality behavior? For example, adding the value property and changing the signature of the constructor wouldn't affect it, would it? Worst come to worst, we can always add these changes and leave the equality question for what it is if we cannot come to an agreement. In any case, we should probably change these things in separate commits if not also separate PRs.

[sphuber]

Will have a double check soon, bare with me, I’m just in the middle of Croatia waiting for a bus lol

pinging @chrisjsewell

[chrisjsewell]

yep will do after lunch

[chrisjsewell]

Errr, I still don't feel the equality change needs to be present in this PR 😬
But anyhow, it's very likely it will be changed again before v2 releases, so I'll leave it up to you 🤷

[sphuber]

Errr, I still don't feel the equality change needs to be present in this PR grimacing

Well I guess it is not strictly necessary, but I don't think it hurts. And it allows the tests in the follow-up commit to use the same assert statement assert converted == value in the parametrized test. Without this addition, I would have to resort to making an exceptional case for the dictionary test.

[chrisjsewell]

Oh I see, in BaseType there is already:

    def __eq__(self, other):
        if isinstance(other, BaseType):
            return self.value == other.value
        return self.value == other

thats why Dict was previously different @sphuber

This then raises the question, should Dict also inherit from BaseType?
Or, at a minimum, we should also implement the __ne__ method on Dict

[sphuber]

The reason I added the __eq__ is indeed exactly because Dict doesn't inherit from BaseType. I didn't want to change the base class for Dict as that could have had even further consequences to the behavior and I explicitly wanted the change to be minimal and just have the node content compare equal to the base type of its value. I added the self is other as the fall-through comparison as that is the base implementation of object. However, I realize now, that I probably should have called super().__eq__(self, other) since Node defines a fall-through based on UUID. We should correct this therefore I think.

Then on whether __ne__ should be specified: I don't think this is necessary. The docs specify that __ne__ is automatically defined as the inverse of __eq__.

[chrisjsewell]

yeh super sounds a good idea 👍
I guess the inverse (pun intended) of adding __ne__ to Dict, would then be removing it from BaseType?

[mbercx]

@mbercx would addressing some of the feature requests really change the equality behavior? For example, adding the value property and changing the signature of the constructor wouldn't affect it, would it? Worst come to worst, we can always add these changes and leave the equality question for what it is if we cannot come to an agreement. In any case, we should probably change these things in separate commits if not also separate PRs.

Sorry, missed this reply! Will work on adding the value property and changing the constructor today.