Proposal to reorganize the Node namespace

[sphuber]

The Node class has many methods and attributes in its public namespace. This has been noted as being a problem for users of an interactive shell as it makes it difficult to find what they are looking for and to intuit what is supposed to be part of the public API. Here is a suggestion for reorganizing the namespace by

• Deprecating/removing methods that are superfluous
• Making methods private that are only supposed to be used internally
• Grouping related methods in a sub-namespace

The main idea is to take all methods that are not directly relevant to the properties of the node itself, but rather have to do with its context in the provenance graph, and move them to a nested namespace. For the sake of argument, let's call this namespace base for now. Any methods that are related can be grouped into further sub-namespaces where appropriate. With this approach the top level namespace should be significantly de-cluttered. One potential downside of moving methods from the top-namespace to sub-namespaces is that discoverability may be slightly reduced. Users will now have to "know" for example to go to Node.base.attributes.get to get an attribute. Still the changes may be a net gain in terms of clarity and user friendliness.

Below is a table with the proposed name changes for each currently public attribute and method:

Current name	Proposed name	Notes
Collection	Deprecate	Use NodeCollection directly
add_comment	Node.base.comments.add
add_incoming	Node.base.links.add_incoming
attributes	Node.base.attributes.all
attributes_items	Node.base.attributes.items
attributes_keys	Node.base.attributes.keys
backend	-
backend_entity	-
check_mutability	Node._check_mutability_attributes
class_node_type	-
clear_attributes	Node.base.attributes.clear
clear_extras	Node.base.extras.clear
clear_hash	Node.base.caching.clear_hash
computer	-
copy_tree	Node.base.repository.copy_tree
ctime	-
delete_attribute	Node.base.attributes.delete
delete_attribute_many	Node.base.attributes.delete_many
delete_extra	Node.base.extras.delete
delete_extra_many	Node.base.extras.delete_many
delete_object	Node.base.repository.delete_object
description	-
erase	Node.base.repository.erase
extras	Node.base.extras.all
extras_items	Node.base.extras.items
extras_keys	Node.base.extras.keys
from_backend_entity	-	Proposed to move to stand-alone function in PR #5447
get	Deprecate for Node.objects.get
get_all_same_nodes	Node.base.caching.get_all_same_nodes
get_attribute	Node.base.attributes.get
get_attribute_many	Node.base.attributes.get_many
get_cache_source	Node.base.caching.get_cache_source
get_comment	Node.base.comments.get
get_comments	Node.base.comments.all
get_description	-	Ideally this becomes well defined as to what it should do to distinguish it from the description property that just fetches the similarly named column from the database mode. Might require a name change for clarity.
get_extra	Node.base.extras.get
get_extra_many	Node.base.extras.get_many
get_hash	Node.base.caching.get_hash
get_incoming	Node.base.links.get_incoming
get_object	Node.base.repository.get_object
get_object_content	Node.base.repository.get_object_content
get_outgoing	Node.base.links.get_outgoing
get_stored_link_triples	Node.base.links.get_stored_link_triples
glob	Node.base.repository.glob
has_cached_links	Remove	(only used in execmanager.upload_calculation and CalcJob.presubmit, should essentially be replaced with is_stored because in that case all links should be stored and the cache is empty.)
id	Deprecate	One should use pk instead
initialize	-
is_created_from_cache	Node.base.caching.is_created_from_cache
is_stored	-
is_valid_cache	Node.base.caching.is_valid_cache
label	-
list_object_names	Node.base.repository.list_object_names
list_objects	Node.base.repository.list_objects
logger	-
mtime	-
node_type	-
objects	collection
open	Node.base.repository.open
pk	-
process_type	-
put_object_from_file	Node.base.repository.put_object_from_file
put_object_from_filelike	Node.base.repository.put_object_from_filelike
put_object_from_tree	Node.base.repository.put_object_from_tree
rehash	Node.base.caching.rehash
remove_comment	Node.base.comments.remove
repository_metadata	Node.base.repository.metadata
repository_serialize	Node.base.repository.serialize
reset_attributes	Node.base.attributes.reset
reset_extras	Node.base.extras.reset
set_attribute	Node.base.attributes.set
set_attribute_many	Node.base.attributes.set_many
set_extra	Node.base.extras.set
set_extra_many	Node.base.extras.set_many
store	-
store_all	-
update_comment	Node.base.comments.update
user	-
uuid	-
validate_incoming	Node.base.links.validate_incoming
validate_outgoing	Node.base.links.validate_outgoing
validate_storability	Node._validate_storability
verify_are_parents_stored	Node._verify_are_parents_stored
walk	Node.base.repository.walk

If these changes were to be adopted, this would be the resulting namespace:

backend
backend_entity
class_node_type
computer
ctime
description
from_backend_entity
get_description
id
is_stored
label
logger
mtime
node_type
objects
pk
process_type
store
store_all
user
uuid
base:
    caching:
        clear_hash
        get_all_same_nodes
        get_cache_source
        get_hash
        is_created_from_cache
        is_valid_cache
        rehash
    attributes:
        all
        clear
        delete
        delete_many
        get
        get_many
        items
        keys
        reset
        set
        set_many
    extras:
        all
        clear
        delete
        delete_many
        get
        get_many
        items
        keys
        reset
        set
        set_many
    comments:
        add
        all
        get
        remove
        update
    repository:
        copy_tree
        delete_object
        erase
        get_object
        get_object_content
        glob
        list_object_names
        list_objects
        metadata
        open
        put_object_from_file
        put_object_from_filelike
        put_object_from_tree
        serialize
        walk
    links:
        add_incoming
        get_incoming
        get_outgoing
        get_stored_link_triples
        validate_incoming
        validate_outgoing

If we can agree on the new naming, the idea would be to get this in v2.0 in a backwards-compatible way. Deprecation warnings could potentially be hidden by default and enabled with a config option or environment variable (see this PR for details). With the development of aiida-upgrade we can even automate the update of existing code. These kinds of changes would be easy to implement in aiida-upgrade so the change should be relatively painless for users.

[chrisjsewell]

Thanks for summarising @sphuber, obviously I can adapt #5439, #5442, #5445 and #5446 to these, when agreed on.

---

Some initial responses:

id: Should we deprecate this in favor of pk?

yes

initialize: This is not actually used and can be removed?

It is used, in from_backend_entity, where nodes are not initialised via __init__

These kinds of changes would be easy to implement in aiida-upgrade

I would just note that this is not facile, since with static analysis it's not foolproof to identify instances of Node.

---

Some initial notes:

• Collection I would just deprecate and use NodeCollection directly
• objects I would rename to collection (to not be confused with repository objects)
  • The behaviour of this is also a little weird to be, for example, it is a classmethod, so does not automatically get the same backend as the node instance
  • Perhaps it could also be moved onto ctx
• Group should also be changed, for extras methods, in a similar way
  • This is what is done in ♻️ REFACTOR: EntityExtrasMixin -> EntityExtras #5445

[sphuber]

It is used, in from_backend_entity, where nodes are not initialised via init

A yeah true, but it really just serves to initialize _incoming_cache to an empty list. I am not sure if it was ever intended that data plugins also use this. If not, then I would be tempted to remove it and then initialize the _incoming_cache manually in constructor and in from_backend_entity. Or we can simply not initialize it and in places where we access it just check if set and if not make it an empty list. Anyway, for the sake of typing there is assert self._incoming_cache is not None, 'incoming_cache not initialised'. Might as well turn this into actually initializing.

I would just note that this is not facile, since with static analysis it's not foolproof to identify instances of Node.

True, but we should be able to get a long way.

Collection I would just deprecate and use NodeCollection directly

I think this would be fine as well. Did a quick search and I don't think anything does Node.Collection, at least not in aiida-core.

objects I would rename to collection (to not be confused with repository objects)

Think collection makes sense. Would be happy with that rename.

[chrisjsewell]

Also to note, for SingleFileData and FolderData you would probably want to essentially "duplicate" some of the Node.ctx.repository methods back up to the top-level

[chrisjsewell]

For the actual implementation, you would do something like:

from functools import cached_property

class NodeAttributes:
    def __init__(self, node):
        self._node = node

    def get(self, key):
        return self._node.backend_entity.get_attribute(key)

class NodeContext:
    def __init__(self, node):
        self._node = node

    @cached_property
    def attributes(self):
        return NodeAttributes(self._node)

class Node:
    @cached_property
    def ctx(self):
        return NodeContext(self)

You'll note, this means you now have to initialise more objects than just the Node.
But cached_property allows you to do this lazily, reducing the overhead

[sphuber]

Looks good in principle, we just have to be a little bit careful with the NodeAttributes. It seems like you are going straight to the `BackendNode. I think that currently there might be some slight difference in the behavior of the attribute/extras related methods on the front-end and back-end nodes. Things like providing a default for empty values etc. Minor stuff but should keep this in mind to check.

[chrisjsewell]

Yeh that's not the full code, it's just a simplified example

[chrisjsewell]

FYI and added bonus of this composition, it that you can also add dunder methods specific to the attributes and extras mappings, e.g. node.ctx.attributes['a'].
You could even have them implement https://docs.python.org/3/library/collections.abc.html#collections.abc.MutableMapping

[ltalirz]

First I should mention that I am fully on board with tidying up method namespace of Nodes - it is a nuisance in everyday use and great to see this being tackled.

In deciding which methods to keep at the top-level and which to group under sub-namespaces, however, I think besides architectural considerations we should consider which methods are most often used interactively.

We could at some point try come up with some sort of statistics, e.g. looking at our tutorial material, but short of that my feeling is that

• .attributes and .extras are some of the most-used methods of all
• several other methods that are top-level in this proposal are rarely used in interactive mode

The main idea is to take all methods that are not directly relevant to the properties of the node itself, but rather have to do with its context in the provenance graph, and move them to a nested namespace.

While the database layout may differ, I think most AiiDA users will consider attributes and extras "properties of the node".

Rather than proposing a full layout, here is my list of methods that I believe should remain at the top level of the node in order to avoid unnecessary typing (more can be added for consistency reasons):

# Must have
description
id
label
pk
store
uuid
attributes
extras

# Good to have but less frequently used
repository
process_type
node_type
is_stored

[sphuber]

Thanks for the feedback @ltalirz . We discussed the question of attributes and extras. I also think they are really part of the node's direct data and not so much contextual metadata. Ideally I would also have kept that in the top-level namespace. However, there is a technical problem and that is that the attributes and extras names are currently already in use to return all attributes and extras. This means we cannot repurpose it as an entry point to a namespace without breaking things. This is why we would be forced to (at least for now) move it into ctx.attributes and ctx.extras. The alternative would be too use a different name like Node.attrs and Node.xtras, but I think this hurts the intuitiveness rather than help make things more clear. Note that if we for the time being put them under ctx we can in the near future, after a reasonable deprecation period, move or expose them also at the top level. I think that might be the best solution.

[sphuber]

Another argument voiced by @chrisjsewell was that although attributes are part of a Node's data, they really should have plugin specific properties on the top-level namespace. For example, the Int node has an attribute value, but instead of having a user do Int().get_attribute('value') it should simply be possible to do Int().value, which in the implementation then uses Node.ctx.attributes.get('value'). I think he has a point there and the reason people now need to use the attribute methods directly is that not all data plugins consistently make use of this concept. That being said, I don't really think this applies to extras` as much, so for those the argument maybe stands that extras might be needed on the top level.

[chrisjsewell]

they really should have plugin specific properties on the top-level namespace.

Yes, the way data nodes actually store provenance data, in the attributes or repository, should not really be exposed to the user. There should be separate methods for the that.

[ltalirz]

Putting attributs/extras aside for a moment, after reading

The main idea is to take all methods that are not directly relevant to the properties of the node itself, but rather have to do with its context in the provenance graph, and move them to a nested namespace.

I expected the ctx sub-namespace to focus on relations to other objects in the provenance graph.

That would lead me to an arrangement like (just showing what changes):

repository
caching
ctx:
    computer:
    user:    
    comments:
    links:

Just pointing this out as another possible grouping.

[chrisjsewell]

repository should definitely not be at the top-level

I wouldn't be adverse to computer and user being under a namespace though

[sphuber]

I expected the ctx sub-namespace to focus on relations to other objects in the provenance graph.

I used that phrasing as a loose description of the rule-of-thumb used to determine what goes under ctx and what stays top-level. I don't think this should be taken too literally. The main point should be what makes most sense. I just observed that for the majority of changes what seemed to make sense is to keep what is directly a property of the node should be top-level and the rest under ctx, but this shouldn't necessarily be a black-and-white rule.

[giovannipizzi]

Thanks, I agree with the division and the following concepts (e.g. rename objects to collection).

Only important comment for me is: I wouldn't use the name ctx: it's the same name used in workchains for a very different purpose, and I'm sure this will lead to big misunderstandings to users.

I'm not sure of a good suggestion, but I think it should not be ctx (or any other specific term we use elsewhere for a different goal).
Maybe properties or props?

[chrisjsewell]

Basically trying to think of three-letter words lol, perhaps dev would be good? As in, it is a namespace primarily of use for plugin developers

[sphuber]

But it is definitely not just for methods for developers. Getting the User or Computer is not really restricted to just developers. I personally don't think reusing ctx should pose much of a problem. Yes, I agree that there might be a handful of people that might initially get confused, but I think that is preferable to a name that is not as intuitive and useful. Still if we can find another that is better than ctx then that would be good. The downside of properties is that not all the methods are about properties of the node, so it is not really correct. There are plenty of methods that are about related entities that are not really the properties of the node itself.

[sphuber]

During the group meeting of April 6, a number of candidates where discussed:

• properties: too long and not semantically correct, not all members are actual properties of the node
• ctx: refused for fear of confusion with the WorkChain.ctx attribute
• core: problem is that many "core" properties are not part of this namespace but at the top level
• env: too generic?
• base: this got the most votes

So instead of ctx, the subnamespace will be named base.

[chrisjsewell]

👍 with the idea that base contains operations originating from the "base" Node, available to all subclasses (i.e. plugins), and which they should not override