Setting attributes for unstored Dict nodes fails silently

[borellim]

I'm confused by a behaviour of the Dict class. I want to update an unstored node d = Dict(), so I do the following:

d.x = 1
d.dict.x = 1

Neither of these commands raises an error, but the dictionary is still empty if I do print(d.get_dict()). I would expect them to either work or raise an error.

For context, the following gives an explicit error instead (which I'm also not sure why):

d['x'] = 1 # TypeError: 'Dict' object does not support item assignment

While the following actually works:

d.update_dict({'x':1})

[ramirezfranciscof]

@borellim So, the problem with d.x = 1 is that you are actually setting an attribute of the node this way, not changing a value in the dict that it has stored. So that part isn't really silently failing, just not doing what you intended; see the following example:

In [10]: from aiida import orm                                                                                          
In [11]: mydict = orm.Dict(dict={'x': 1})                                                                               
In [12]: mydict.x = 2                                                                                                   
In [13]: mydict.x                                                                                                       
Out[13]: 2
In [14]: mydict.dict.x                                                                                                  
Out[14]: 1

I understand how this case could be confusing, but I really don't think there is much to be done except maybe adding some emphasis in the documentation that Dict.x and Dict.dict.x are actually two different things and the dictionary element is the second one.

In the other two cases, yes, you are referencing the dict that is stored inside. The problem with d.dict.x = 1 is that there was no __setattr__ defined in the AttributeManager returned by d.dict. Is there any specific reason why this is the case @sphuber ? The only thing I needed to modify to set this up was the way the internal node was defined in the __init__ of the class:

class AttributeManager:

    def __init__(self, node):
        self.__dict__['_node'] = node
        #self._node = node

    def __setattr__(self, key, value):
        self._node.set_attribute(key, value)

Finally, d['x'] = 1 was not possible because there was no __setitem__ method defined in the Dict node class. This I fixed simply as defining:

class Dict(Data):

    def __setitem__(self, key, item):
        self.update_dict({key:item})

No tests failed for me after these modification. In both cases I am using the intrinsic update_dict and set_attribute so that they will check if the node is stored first; but I think eventually it could actually be better to do it the other way around and add the checks directly to the __methods__, and use these instead of the update_dict and set_attribute.

@sphuber let me know what you think of these modifications, if they are ok I will add some tests to try these direct accesses and make a PR.

[sphuber]

The reason you cannot set dictionary content through attributes is that there would be a potential clash in properties that are generic to the Node class. Example, let's say you want to set the key label to test. You can do it with node.update_dict({'label': 'test'}). Through the attribute setter, however, this would become node.label = 'test'. Now the problem is apparent. Does label need to be interpreted as an internal key of the Dict node, or the label property of the base Node class. Ultimately, setting and getting dictionary keys through attributes is not possible, because of this. The question is if we can rewrite the code to warn the user. Except I don't think we can, for the very same reason, we have to simply document this.

This is also the very reason for the existence of the dict property. It stands to reason that this should then also allow to set attributes, as long as the mutability rules are respected. The implementation of __setattr__ you propose looks fine, because it calls set_attribute which contains all the mutability checks.

With respect to __setitem__. I don't see a reason why we shouldn't allow this. If we have it go properly through set_attribute also here mutability rules will be respected. In addition, __getitem__ can be made a little more efficient by using get_attribute directly. This prevents loading the entire dictionary from the database and will give a small efficiency increase.

[ramirezfranciscof]

Just a self reminder for my own understanding: get_attribute refers to the backend database concept of attribute (which for a dict node it is just its dict content), not the python concept of the attribute of an object (setting "property-like" attributes in nodes such as the first of the examples here will only keep it in memory, it doesn't get stored in the database).