List/Dict: Add value property

[mbercx]

Fixes #4495

Fixes #5313

Add the value property to the List and Dict classes, to make them
more consistent with other base type classes.

[codecov]

Codecov Report

Merging #5297 (318c627) into develop (c743a33) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           develop    #5297      +/-   ##
===========================================
- Coverage    81.51%   81.51%   -0.00%     
===========================================
  Files          530      530              
  Lines        37103    37111       +8     
===========================================
+ Hits         30242    30246       +4     
- Misses        6861     6865       +4     

Flag	Coverage Δ
django	76.97% <50.00%> (-<0.01%)	⬇️
sqlalchemy	75.96% <50.00%> (-<0.01%)	⬇️

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

Impacted Files	Coverage Δ
aiida/orm/nodes/data/dict.py	84.32% <50.00%> (-2.92%)	⬇️
aiida/orm/nodes/data/list.py	90.00% <50.00%> (-1.86%)	⬇️

---

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 c743a33...318c627. Read the comment docs.

[mbercx]

I've left the PR as a draft since I'm not sure if I can replicate the behaviour of value for the base types when modifying the content of the List/Dict node instead of setting it. Currently, the behaviour of setting e.g. the value of an unstored vs stored node:

In [1]: i = Int(2)

In [2]: i
Out[2]: <Int: uuid: 3ef91127-7fb4-47b4-9a7f-57b5a1ede86a (unstored) value: 2>

In [3]: i.value = 4

In [4]: i
Out[4]: <Int: uuid: 3ef91127-7fb4-47b4-9a7f-57b5a1ede86a (unstored) value: 4>

In [5]: i.store()
Out[5]: <Int: uuid: 3ef91127-7fb4-47b4-9a7f-57b5a1ede86a (pk: 18) value: 4>

In [6]: i.value = 6

[...] Removed for brevity

ModificationNotAllowed: the attributes of a stored entity are immutable

Is replicated for the value property of e.g. List:

In [1]: l = List([1, 2])

In [2]: l
Out[2]: <List: uuid: 77e96841-285f-4d19-8802-09b7d0110f13 (unstored) value: [1, 2]>

In [3]: l.value = [3, 4]

In [4]: l
Out[4]: <List: uuid: 77e96841-285f-4d19-8802-09b7d0110f13 (unstored) value: [3, 4]>

In [5]: l.store()
Out[5]: <List: uuid: 77e96841-285f-4d19-8802-09b7d0110f13 (pk: 19) value: [3, 4]>

In [6]: l.value = [5, 6]

[...] Removed for brevity

ModificationNotAllowed: the attributes of a stored entity are immutable

However, the behaviour of modifying the content through the value property is different:

In [1]: l = List([1, 2])

In [2]: l
Out[2]: <List: uuid: b39a4626-d1d0-43f0-8c41-857677dc8ce9 (unstored) value: [1, 2]>

In [3]: l.value.append(3)

In [4]: l
Out[4]: <List: uuid: b39a4626-d1d0-43f0-8c41-857677dc8ce9 (unstored) value: [1, 2, 3]>

In [5]: l.store()
Out[5]: <List: uuid: b39a4626-d1d0-43f0-8c41-857677dc8ce9 (pk: 21) value: [1, 2, 3]>

In [6]: l.value.append(4)

In [7]: l
Out[7]: <List: uuid: b39a4626-d1d0-43f0-8c41-857677dc8ce9 (pk: 21) value: [1, 2, 3]>

I.e. if the node is unstored, the value property returns the underlying base type that is stored in the attributes, and hence modifying this value modifies the content of the node. Once the node is stored, however, a deepcopy of the underlying base type is returned, and hence this copy is modified and the content of the node is left untouched.

I think this behaviour isn't really inconsistent with the other base types in a sense. As far as I know, the other base types don't have methods that modify the content, so there isn't really any behaviour to replicate. Still, the behaviour is inconsistent with how the content is set using the value property when the node is stored. However, since when the node is stored the value property simply returns a deepcopy of the underlying built-in base type, and at no point the setter is used, I'm not sure how to have [6] raise in the last example.

The current behaviour when modifying through the value property is problematic, though, and I'm not sure if the benefit of adding this property weighs up to this. Unless there is a way to make modifications through methods raise when the node is stored, perhaps we should hold off on adding the value property to List and Dict until we decide to have value always return a deepcopy, regardless of whether the node is stored or not (i.e. approach 3 in this comment). Even in this case however, we plan to have the setter raise, so preferably modifying e.g. l.value with append would raise as well.

[giovannipizzi]

Indeed, I don't think there is an easy workaround for this. Lists and dicts are pointers, so they have a different behaviour, sometimes unexpected (see e.g. when setting an empty list as the default value of a function!). Also other languages like JS have similar problems (see e.g. frameworks like vue.js or react, where they have to resort to just documenting the behaviour that changing dicts or lists should not be directly accessed to change them, but you should use special setter/getter methods, otherwise the library does not know that a change happened.

The only other option (that I wouldn't do) would be to return a subclass of list when the node is stored, that overrides certain methods. But I feel this is way too complex and error-prone to be worth it.

So, there are two options:

1. we don't add the .value to these mutable objects, and document why in the docs and in the tutorial
2. we add it, accept the behaviour and document it (with the risk of people not reading the docs and getting surprised)

At this point, I would probably vote for 1 - we've been living without .value for Dict and List and this is I think OK. So I would close this, and instead fix the original issue by adding a paragraph about this:

• in the docs (I don't know what would be the best place though? the design docs? But maybe also in the first tutorial so everybody reads it?)
• in the AiiDA tutorial (now, or at least add an issue there so we don't forget)

[sphuber]

Unless there is a way to make modifications through methods raise when the node is stored

I doubt this is possible because what you return is a list or dict and so you would have to override the implementation of those base types. Definitely not the way to go.

I guess that ultimately users will always have to learn about the concept of mutability in Python. So no matter whether you get the value through a property value or a method get_value(), the user will have to learn that mutating that return value, will not mutate the actual content of the node. However, maybe the method does make this a bit more evident? I can see how

List().value.append(1)

seems more natural and like it would work, compared to

List().get_value().append(1)

although I bet that there will be many people that are not intimately familiar with Python that will expect the second to also update the content of the node.

So I personally think it is still fine to add the properties as there will always be some particular behavior that is not immediately obvious to 100% of users. But then again, I don't think it is crucial either, so fine to not put it in.

[mbercx]

Will close this PR for now, we can continue the discussion in #5313. I'm still in favour of not adding value to Dict/List in the end, and documenting this decision properly.