Decide behavior when storing Data sub classes within __main__

[sphuber]

Consider the following:

class SubData(Data):
    pass

data = SubData().store()

This will store the node just fine, but its node_type will be __main__.SubData. if performed in a script or interactive shell. Loading the node will fail though, without a fallback:

load_node(data.pk)

will raise

EntryPointError                           Traceback (most recent call last)
<ipython-input-33-0a20738a6046> in <module>()
----> 1 load_node(d.pk)

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/utils/__init__.py in load_node(identifier, pk, uuid, label, sub_classes, query_with_dashes)
    207         label=label,
    208         sub_classes=sub_classes,
--> 209         query_with_dashes=query_with_dashes
    210     )

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/utils/__init__.py in load_entity(entity_loader, identifier, pk, uuid, label, sub_classes, query_with_dashes)
     81 
     82     return entity_loader.load_entity(
---> 83         identifier, identifier_type, sub_classes=sub_classes, query_with_dashes=query_with_dashes
     84     )
     85 

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/utils/loaders.py in load_entity(cls, identifier, identifier_type, sub_classes, query_with_dashes)
    218 
    219         try:
--> 220             entity = builder.one()[0]
    221         except MultipleObjectsError:
    222             error = 'multiple {} entries found with {}<{}>'.format(classes, identifier_type, identifier)

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/querybuilder.py in one(self)
   2127         from aiida.common.exceptions import MultipleObjectsError, NotExistent
   2128         self.limit(2)
-> 2129         res = self.all()
   2130         if len(res) > 1:
   2131             raise MultipleObjectsError('More than one result was found')

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/querybuilder.py in all(self, batch_size)
   2202         :returns: a list of lists of all projected entities.
   2203         """
-> 2204         return list(self.iterall(batch_size=batch_size))
   2205 
   2206     def dict(self, batch_size=None):

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/querybuilder.py in iterall(self, batch_size)
   2162             # Convert to AiiDA frontend entities (if they are such)
   2163             for i, item_entry in enumerate(item):
-> 2164                 item[i] = self.get_aiida_entity_res(item_entry)
   2165 
   2166             yield item

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/querybuilder.py in get_aiida_entity_res(value)
   2054         """
   2055         try:
-> 2056             return convert.get_orm_entity(value)
   2057         except TypeError:
   2058             return value

/home/sphuber/.virtualenvs/aiida_dev/lib/python3.6/functools.py in wrapper(*args, **kw)
    805                             '1 positional argument')
    806 
--> 807         return dispatch(args[0].__class__)(*args, **kw)
    808 
    809     funcname = getattr(func, '__name__', 'singledispatch function')

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/convert.py in _(backend_entity)
     81 def _(backend_entity):
     82     from .utils.node import load_node_class
---> 83     node_class = load_node_class(backend_entity.node_type)
     84     return node_class.from_backend_entity(backend_entity)
     85 

/home/sphuber/code/aiida/env/dev/aiida-core/aiida/orm/utils/node.py in load_node_class(type_string)
     81         return load_entry_point('aiida.node', entry_point_name)
     82 
---> 83     raise exceptions.EntryPointError('unknown type string {}'.format(type_string))
     84 
     85 

EntryPointError: unknown type string __main__.SubData.

[greschd]

I don't think there's a way to make this work from interactive scripts, right?

When it's just a script, in principle the script could be imported as a module - but that assumes it's properly segregated into the "script" and "module" parts with if __name__ == "__main__".

If we try to get the module name in that case with

os.path.splitext(os.path.basename(sys.modules[type(data).__module__].__file__))[0]

you get the right file when running it with bare python, but runaiida gives you verdi. Same with inspect.getmodule or inspect.getfile. There is probably some way to inspect around this, but it seems like a bit of a hack.

Maybe the better option is to check that a class is importable when storing?

[sphuber]

I think the question should rather be exactly whether we allow even storing this. If we do, then we should add a fall-back in the loader, to just load a base Data class and emit a warning. In any case I don't think a user can expect this type of usage to be fully compatible with all of AIiDA's functionality

[greschd]

In general, is there a fallback to Data when the entry point doesn't exist (e.g., plugin not installed, changed its entry points, ...)? If so, why doesn't it catch this as well?

If it's just about this specific case, I'd probably favor not allowing the store.

[sphuber]

We cannot fallback on Data in general, because we might not now that the node was actually a sub class of Data in principle. In the given example, there is no way to know for sure that __main__.SubData came from a Data sub class. It could have been called __main__.SomeSubClass. The best we could do is fall back on the base Node class and return that. That change would be easy, we would just have to change the raise in aiida.orm.utils.node.load_node_class to a return Node. However, we really should also issue a warning here, because there might be some cases where the type string is perfectly valid but the corresponding class could just not be resolved/imported.

[greschd]

Isn't the problem similar to what happens to a node when trying to open it in an AiiDA installation that does not have that particular plugin installed / if the class was renamed? What happens in that case?

[greschd]

I think we should be as lenient as possible when loading (with possibly a warning), but can be more strict when storing.

When you already have data that doesn't have a matching class it's better to have some access to it than not being able to load it at all, but creating such data should be strongly discouraged.

[sphuber]

The difference is that any node that was generated with a class that was valid at the time and had a proper entry point, will have a correct type string, e.g. will start with data. or process.. For both those cases the loader has the correct fallback mechanism. It is really for nodes that were stored with unusable classes that there is a problem. So as soon as we prohibit those from being stored, I think we should be good. We can of course still turn the exception into a warning and always just fall back on a node.

[greschd]

We can of course still turn the exception into a warning and always just fall back on a node.

Since the current version allows creating such nodes, I think the warning + fallback would be the better choice.