aiida-core
aiida-core copied to clipboard
👌 IMPROVE: Hide dev/non-public API methods
Is your feature request related to a problem? Please describe
When using tab-completion in an IPython kernel (console or notebook), the user gets a very long list of methods which are largely not really part of what one might consider to be the public API:
![Screenshot 2021-06-06 at 13 31 42](https://user-images.githubusercontent.com/25607960/120922810-89f30d00-c6cb-11eb-97e8-7525c612cb2a.png)
This can be overwhelming for new users, and makes it very hard to explore the features of a Node
class without reading the documentation.
Describe the solution you'd like
While he was working on #4975, @chrisjsewell explained to me that the __dir__()
method can be used to override the list of methods that are shown when using tab-completion. We then considered that this might also be used to achieve a cleaner list of methods for the user for already existing Node
classes.
Developing in an IDE with intellisense shouldn't be affected, and we could consider writing some IPython magic to reactivate tab-completion for all methods, or even make this configurable with verdi config
.
Additional context
This issue was also raised by @louisponet during his code show-and-tell, as well as by other users during tutorials etc.
So I just want to expand on this a bit:
The number of attributes/methods on Node
is way too high, and this is exacerbated further on subclasses, that have their own attributes/methods:
In [1]: node_props = {n for n in dir(Node) if not n.startswith('_')}
In [2]: len(node_props)
Out[2]: 84
In [3]: dict_props = {n for n in dir(Dict) if not n.startswith('_')}
In [4]: len(dict_props)
Out[4]: 99
Not only that, but they are also not particularly helpfully named for tab completion, e.g. you could argue that get_attribute
/set_attribute
, would be easier as attribute_get
/attribute_set
.
One problem here is that, in general programming, you will have two types of methods; private and public. For nodes though, we essentially want three:
- Ones that developers of aiida-core use
- Ones that developers of a data plugin use
- Ones that users of a data plugin use
As a pseudo example
# aiida-core developers
class Entity:
def __init__(self, backend_entity):
self._backend_entity = backend_entity
@property
def backend_entity(self):
return self._backend_entity
class Node(Entity):
def set_attribute(self, key: str, value: Any):
if self.is_stored:
raise exceptions.ModificationNotAllowed('the attributes of a stored entity are immutable')
self.backend_entity.set_attribute(key, value)
# plugin developers
class Dict(Node):
def __setitem__(self, key, value):
self.set_attribute(key, value)
# users
dict = Dict()
dict["key"]
In this example:
- Plugin developers should never access
Node.backend_entity
- Users should never access
Dict.set_attribute
We could override the __dir__()
method, but I think this is somewhat a band-aid and not a solution.
One specific problem I see, is the use of mixins:
class Node(Entity, NodeRepositoryMixin, EntityAttributesMixin, EntityExtrasMixin):
IMO, mixins are never usually a good solution, and one should always prefer composition over mixins (see https://en.wikipedia.org/wiki/Composition_over_inheritance), e.g.
class Node(Entity):
def __init__(self):
self.repo = NodeRepository(self)
self.attributes = EntityAttributes(self)
self.extras = EntityExtras(self)
One would then use e.g. node.attributes.set()
, rather than node.set_attributes()
, which
- Makes tab-completion a lot more user-friendly, by grouping methods/attributes under namespaces
- Implementing just this would already reduce the
Node
method/attributes count to 50 (i.e. reducing by 34)
(note this is adapted https://github.com/aiidateam/aiida-core/pull/5088#issuecomment-906304560)
Phase 1 😉 https://github.com/aiidateam/aiida-core/pull/5439
Phase 2 #5442
Phase 3 #5445
Phase 4 #5446 (and #5447)
Down to 43 public attributes/methods on Node
['Collection', 'add_incoming', 'attrs', 'backend', 'backend_entity', 'class_node_type', 'clear_hash', 'comments', 'computer', 'ctime', 'description', 'get', 'get_all_same_nodes', 'get_cache_source', 'get_description', 'get_hash', 'get_incoming', 'get_outgoing', 'get_stored_link_triples', 'has_cached_links', 'id', 'initialize', 'is_created_from_cache', 'is_stored', 'is_valid_cache', 'label', 'logger', 'mtime', 'node_type', 'objects', 'pk', 'process_type', 'rehash', 'repo', 'store', 'store_all', 'user', 'uuid', 'validate_incoming', 'validate_outgoing', 'validate_storability', 'verify_are_parents_stored', 'xtras']