Restore proper mutability implementation of Node attributes #1111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sphuber
added
type/bug
priority/critical-blocking
|
Suggested additions/changes:
|
sphuber
force-pushed
the
fix_1109_mutable_attributes
branch
6 times, most recently
from
01514a4
to
04270ee
Compare
This was referenced Feb 22, 2018
The check for node attribute immutability, i.e. when a node is stored its attributes are to be immutable, was moved out of the base Node class and moved to the Data class and the SealableWithUpdatableAttributes mixin, in order to allow for Calculation attributes that needed to be mutable even after storing the node, e.g. the calculation state. However, the logic was improperly implemented and not applied everywhere leaving certain nodes with all attributes mutable even after being stored. To prevent this error from happening in the future, we move the check back to the Node class. The calculation classes will have to override this behavior in the future to allow for updatable attributes
The attribues of the Node class become immutable once the node is stored. However, for some Node subclasses one needs to allow for certain attributes to be updatable even after storing. An example is the Calculation class. For it to run, it needs to be stored, however its running state is stored in an attribute which needs to be updated despite the underlying node already being stored. To solve this we introduce the Sealable mixin, which can defined a tuple of updatable attributes that are mutable even when the node is stored. It also provides the 'sealed' attribute which when set even the updatable attributes become immutable
…utes This was already done in PR aiidateam#652 that was merged into develop but needs to be done in this branch as well for consistency, which will be merged into the v0.11.1 patch release
The 'input_plugin', 'append_text' and 'prepend_text' attributes should not really be updatable as they can really alter the behavior of the code. For that reason we remove them from the updatable_attributes. The 'hidden' attribute can be turned into an extra, as it really is more meta-data that is only pertinent to the user that owns the code. That leaves no updatable attributes for this class, which means the Sealable mixin can be removed.
sphuber
force-pushed
the
fix_1109_mutable_attributes
branch
from
04270ee
to
45869f2
Compare
sphuber
force-pushed
the
fix_1109_mutable_attributes
branch
2 times, most recently
from
ce87800
to
1f074c6
Compare
The -a switch for verdi code list was broken because it filtered on the hidden attribute in a hardcoded way and just checked if the hidden property was not True. However if it is not set at all, which is the default, then it should also be ignored.
sphuber
force-pushed
the
fix_1109_mutable_attributes
branch
from
1f074c6
to
03cd3cb
Compare
To make sure that it properly extends the mutable attributes from AbstractCalculation, we subclass from AbstractCalculation instead of just object
giovannipizzi
approved these changes
Feb 22, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1109
The concept of attribute immutability has been properly reinstated. The current conception of node attribute mutability is the following.
Nodeis storedSealablemixin are immutable when the node issealedor whenstoredand the attribute is not in the_updatable_attributestupleTo enforce this behavior, the
is_storedcheck is moved back into the_set_attrand_del_attrmethods of theAbstractNodeclass. TheSealablemixin, allows theAbstractCalculation,WorkCalculationandCodesubclasses ofNodeto define a tuple of_updatable_attributesthat can be mutated as long as the node is not sealed.The updatable attributes that were defined for the
Codeclass,input_plugin,prepend_text,append_textandhiddenhave been removed, allowing theSealablemixin to be removed. The first three really should be immutable as changing them can really affect the outcome of calculations run with that code. Thehiddenattribute is more a meta data attribute, specific to the user that owns the code and therefore better belongs in the extras of the node