User unfriendly behavior of attributes.all when stored

[agoscinski]

Description of problem

Setting the attribute associated with a structure can be changed by accessing struc.base.attributes.all before the storing of the node. That results in confusing behavior

from aiida import orm
struc = orm.StructureData(pbc=False)
print(struc.base.attributes.all)
# Output:
#{'cell': [[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]],
# 'pbc1': True,
# 'pbc2': True,
# 'pbc3': True}
struc.base.attributes.all["magnetic_momentum"] = 0.
print(struc.base.attributes.all)
# Output:
#{'cell': [[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]],
# 'pbc1': True,
# 'pbc2': True,
# 'pbc3': True,
# 'magnetic_momentum': 0.0}
struc.store()
struc.base.attributes.all["magnetic_momentum"] = 1.
print(struc.base.attributes.all)
# Output:
#{'cell': [[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]],
# 'pbc1': True,
# 'pbc2': True,
# 'pbc3': True,
# 'magnetic_momentum': 0.0}
# Ooops! I have set it to 1.0 but it is still 0.0. No change. But no one told me

Behavior makes sense if considering the logic of all https://github.com/aiidateam/aiida-core/blob/589a3b2c03d44cebd26e88243ca34fcdb0e23ff4/src/aiida/orm/nodes/attributes.py#L41-L60

In contrary using struc.base.attributes.set("magnetic_momentum", 1.0) gives this nice error message

File ~/micromamba/envs/aiida-dev/lib/python3.11/site-packages/aiida/orm/nodes/node.py:223, in Node._check_mu
tability_attributes(self, keys)
    216 """Check if the entity is mutable and raise an exception if not.
    217 
    218 This is called from `NodeAttributes` methods that modify the attributes.
    219 
    220 :param keys: the keys that will be mutated, or all if None
    221 """
    222 if self.is_stored:
--> 223     raise exceptions.ModificationNotAllowed('the attributes of a stored entity are immutable')

ModificationNotAllowed: the attributes of a stored entity are immutable

Suggestion

In a discussion with @mikibonacci @khsrali @GeigerJ2 we agreed that it would be more user friendly to always return a copy, so the user is forced use the set function, and will stumble on this different behavior depending on if the node is stored.

One could replace the all property with a get function (can be called get_dict, as_dict, to_dict or something else, the name should just be transparently convey that a copy is returned) that returns always a copy, so the user is forced to use the set function to set attributes.

[sphuber]

This is not the only place in the API where this gotcha occurs. Throw it on the pile! 😅 See this discussion for reference: https://github.com/orgs/aiidateam/discussions/5976

I agree that changing it from a property to a method would help a bit, but experience learns that this will still not be immune to the problem. A lot of users will still do:

struc.base.attributes.get_dict()["magnetic_momentum"] = 1.

and be surprised all the same.

I agree that the proposed changes would make things a bit clearer. But we cannot actually replace all because of backwards compatibility. At best we can add the new method and deprecate all.