♻️ REFACTOR: `EntityAttributesMixin` -> `NodeAttributes` #5442

chrisjsewell · 2022-03-14T12:19:26Z

Part of addressing #4976 and #5465

Similar to #5439, it moves the node's attributes methods to a separate "namespace", in a back compatible manner, and is another step towards #4976 (comment): removing another 10 methods (i.e. #5439 + #5442, goes from 83 -> 58)

the following deprecations are made:

    _deprecated_attr_methods = {
        'attributes': 'all',
        'get_attribute': 'get',
        'get_attribute_many': 'get_many',
        'set_attribute': 'set',
        'set_attribute_many': 'set_many',
        'reset_attributes': 'reset',
        'delete_attribute': 'delete',
        'delete_attribute_many': 'delete_many',
        'clear_attributes': 'clear',
        'attributes_items': 'items',
        'attributes_keys': 'keys',
    }

e.g. Node.get_attribute to Node.base.attributes.get

It also actually fixes a bug with the Sealable mixin of ProcessNode, whereby it only overrode set_attribute and delete_attribute, to change mutability behaviour, but not any other mutation methods, like set_attribute_many, etc

sphuber

Thanks @chrisjsewell , just some minor things.

aiida/orm/nodes/attributes.py

sphuber · 2022-04-07T07:43:37Z

aiida/orm/nodes/attributes.py

+        self._entity = node
+        self._backend_entity = node.backend_entity


Any reason to keep the name entity here when it only ever applies to a node? Would _node and _backend_node not be clearer?

sphuber · 2022-04-07T07:44:27Z

aiida/orm/nodes/attributes.py

+    def __contains__(self, key: str) -> bool:
+        """Check if the node contains an attribute with the given key."""
+        return key in self._backend_entity.attributes


This is an addition I think? Don't think I have seen an added test for this, correct?

I've removed the method for now

re-instated because there is actually a test, hence why they were failing

Note @sphuber, it's my intention to look into making attributes and extras a subclass of MutableMapping (https://docs.python.org/3/library/collections.abc.html?highlight=mutablemapping#collections-abstract-base-classes), which would actually remove this method.

But this change is just additive, i.e. is just adding extra methods and not breaking anything. So don't want to hold up this initial re-structuring with it

aiida/orm/nodes/node.py

sphuber · 2022-04-07T07:51:16Z

aiida/orm/nodes/node.py

@@ -194,6 +200,16 @@ def base(self) -> NodeBase:
        """Return the node base namespace."""
        return NodeBase(self)

+    def _check_mutability_attributes(self, keys: Optional[List[str]] = None) -> None:  # pylint: disable=unused-argument


Why do we need an explicit method for mutability of attributes? I don't think there are degrees of mutabillity are there? If it doesn't already exist, I would just have a _check_mutability. And really, since you are calling it from outside of the class, it should be public. I know that you probably do this to keep the namespace clean, but this seems to be a fundamental property. Alternatively, put it in Node.base.check_mutability?

I see now, this is to do with the Sealable aspect. I also notice that you essentially fixed a bug. I think that set_attribute_many would have been possible on sealed nodes because Sealable didn't override that method. Same goes for delete_attribute_many. Anyway, I don't have a better solution for this at this point, so fine to keep for now.

Co-authored-by: Sebastiaan Huber <[email protected]>

sphuber · 2022-04-07T12:50:25Z

@chrisjsewell just to let you know that tests are failing, so cannot merge yet

aiida/orm/nodes/attributes.py

sphuber · 2022-04-07T16:28:25Z

You forgot to apply the backend_entity -> backend_node in __contains__ I think @chrisjsewell

aiida/orm/nodes/attributes.py

chrisjsewell · 2022-04-07T17:09:47Z

You forgot to apply the backend_entity -> backend_node in contains I think @chrisjsewell

Third times a charm 🤞

chrisjsewell · 2022-04-07T17:33:00Z

All tests passing @sphuber

sphuber

There are still uses of the deprecated methods:

 tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
  /home/runner/work/aiida-core/aiida-core/tests/tools/archive/test_complex.py:181: AiidaDeprecationWarning: `Dict.attributes` is deprecated, use `Dict.base.attributes.all` instead. (this will be removed in v3)
    item['param']['*'].attributes,

tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
  /home/runner/work/aiida-core/aiida-core/tests/tools/archive/test_complex.py:186: AiidaDeprecationWarning: `CalculationNode.attributes` is deprecated, use `CalculationNode.base.attributes.all` instead. (this will be removed in v3)
    item['calc']['*'].attributes,

tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
tests/tools/archive/test_complex.py::test_reexport
  /home/runner/work/aiida-core/aiida-core/tests/tools/archive/test_complex.py:187: AiidaDeprecationWarning: `ArrayData.attributes` is deprecated, use `ArrayData.base.attributes.all` instead. (this will be removed in v3)
    item['array']['*'].attributes,

tests/tools/archive/test_simple.py::test_calc_of_structuredata
  /home/runner/work/aiida-core/aiida-core/tests/tools/archive/test_simple.py:66: AiidaDeprecationWarning: `StructureData.attributes` is deprecated, use `StructureData.base.attributes.all` instead. (this will be removed in v3)
    for k in node.attributes.keys():

tests/tools/archive/test_simple.py::test_calc_of_structuredata
  /home/runner/work/aiida-core/aiida-core/tests/tools/archive/test_simple.py:66: AiidaDeprecationWarning: `CalcJobNode.attributes` is deprecated, use `CalcJobNode.base.attributes.all` instead. (this will be removed in v3)
    for k in node.attributes.keys():

tests/tools/archive/test_specific_import.py::test_cycle_structure_data
  /home/runner/work/aiida-core/aiida-core/tests/tools/archive/test_specific_import.py:124: AiidaDeprecationWarning: `CalculationNode.attributes` is deprecated, use `CalculationNode.base.attributes.all` instead. (this will be removed in v3)
    assert isinstance(calculation.attributes, dict)

tests/tools/archive/test_specific_import.py::test_cycle_structure_data
  /home/runner/work/aiida-core/aiida-core/tests/tools/archive/test_specific_import.py:125: AiidaDeprecationWarning: `CalculationNode.attributes` is deprecated, use `CalculationNode.base.attributes.all` instead. (this will be removed in v3)
    assert len(calculation.attributes) != 0

tests/tools/groups/test_paths.py::test_walk_nodes
  /home/runner/work/aiida-core/aiida-core/tests/tools/groups/test_paths.py:137: AiidaDeprecationWarning: `Data.attributes` is deprecated, use `Data.base.attributes.all` instead. (this will be removed in v3)
    assert [(r.group_path.path, r.node.attributes) for r in group_path.walk_nodes()] == [('a', {'i': 1, 'j': 2})]

Would be good to fix those, then we can merge

chrisjsewell · 2022-04-07T20:24:37Z

There are still uses of the deprecated methods:

How helpful are those deprecation warnings though 😉

sphuber · 2022-04-07T20:54:25Z

How helpful are those deprecation warnings though 😉

very 👍

for more information, see https://pre-commit.ci

sphuber

Thanks @chrisjsewell good to go

chrisjsewell mentioned this pull request Mar 14, 2022

👌 IMPROVE: Hide dev/non-public API methods #4976

Closed

chrisjsewell changed the title ~~♻️ REFACTOR: NodeRepositoryMixin -> NodeRepository~~ ♻️ REFACTOR: EntityAttributesMixin -> NodeAttributes Mar 14, 2022

chrisjsewell force-pushed the orm-node-attributes branch 2 times, most recently from e71d94d to 5e8f824 Compare March 14, 2022 22:20

chrisjsewell mentioned this pull request Mar 15, 2022

♻️ REFACTOR: EntityExtrasMixin -> EntityExtras #5445

Merged

chrisjsewell force-pushed the orm-node-attributes branch from 5e8f824 to d815e49 Compare March 21, 2022 23:39

ramirezfranciscof added this to the v2.0.0 milestone Apr 6, 2022

♻️ REFACTOR: EntityAttributesMixin -> NodeAttributes

9e67375

chrisjsewell force-pushed the orm-node-attributes branch from d815e49 to 9e67375 Compare April 7, 2022 05:06

chrisjsewell requested a review from sphuber April 7, 2022 05:09

sphuber requested changes Apr 7, 2022

View reviewed changes

chrisjsewell and others added 2 commits April 7, 2022 11:40

Apply suggestions from code review

68298d6

Co-authored-by: Sebastiaan Huber <[email protected]>

Update attributes.py

da5320e

chrisjsewell requested a review from sphuber April 7, 2022 09:57

chrisjsewell commented Apr 7, 2022

View reviewed changes

aiida/orm/nodes/attributes.py Show resolved Hide resolved

Update aiida/orm/nodes/attributes.py

f77ca6a

chrisjsewell commented Apr 7, 2022

View reviewed changes

aiida/orm/nodes/attributes.py Outdated Show resolved Hide resolved

Update aiida/orm/nodes/attributes.py

9ec19ba

sphuber requested changes Apr 7, 2022

View reviewed changes

Fix deprecation warnings

fb1b17b

chrisjsewell requested a review from sphuber April 8, 2022 07:54

[pre-commit.ci] auto fixes from pre-commit.com hooks

c3f9ef9

for more information, see https://pre-commit.ci

sphuber approved these changes Apr 8, 2022

View reviewed changes

chrisjsewell merged commit 8bb6c3f into aiidateam:develop Apr 8, 2022

chrisjsewell deleted the orm-node-attributes branch April 8, 2022 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ REFACTOR: `EntityAttributesMixin` -> `NodeAttributes` #5442

♻️ REFACTOR: `EntityAttributesMixin` -> `NodeAttributes` #5442

chrisjsewell commented Mar 14, 2022 •

edited

Loading

sphuber left a comment

sphuber Apr 7, 2022

chrisjsewell Apr 7, 2022

sphuber Apr 7, 2022

chrisjsewell Apr 7, 2022

chrisjsewell Apr 7, 2022

chrisjsewell Apr 7, 2022

sphuber Apr 7, 2022

sphuber commented Apr 7, 2022

sphuber commented Apr 7, 2022

chrisjsewell commented Apr 7, 2022

chrisjsewell commented Apr 7, 2022

sphuber left a comment

chrisjsewell commented Apr 7, 2022

sphuber commented Apr 7, 2022

sphuber left a comment

		self._entity = node
		self._backend_entity = node.backend_entity

♻️ REFACTOR: EntityAttributesMixin -> NodeAttributes #5442

♻️ REFACTOR: EntityAttributesMixin -> NodeAttributes #5442

Conversation

chrisjsewell commented Mar 14, 2022 • edited Loading

sphuber left a comment

Choose a reason for hiding this comment

sphuber Apr 7, 2022

Choose a reason for hiding this comment

chrisjsewell Apr 7, 2022

Choose a reason for hiding this comment

sphuber Apr 7, 2022

Choose a reason for hiding this comment

chrisjsewell Apr 7, 2022

Choose a reason for hiding this comment

chrisjsewell Apr 7, 2022

Choose a reason for hiding this comment

chrisjsewell Apr 7, 2022

Choose a reason for hiding this comment

sphuber Apr 7, 2022

Choose a reason for hiding this comment

sphuber commented Apr 7, 2022

sphuber commented Apr 7, 2022

chrisjsewell commented Apr 7, 2022

chrisjsewell commented Apr 7, 2022

sphuber left a comment

Choose a reason for hiding this comment

chrisjsewell commented Apr 7, 2022

sphuber commented Apr 7, 2022

sphuber left a comment

Choose a reason for hiding this comment

♻️ REFACTOR: `EntityAttributesMixin` -> `NodeAttributes` #5442

♻️ REFACTOR: `EntityAttributesMixin` -> `NodeAttributes` #5442

chrisjsewell commented Mar 14, 2022 •

edited

Loading