Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move base Data types to their own files to respect plugin type convention #1192

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Feb 25, 2018

Fixes #1098

Note that this fixes the problem for Bool, Float, Int and Str but I couldn't move the List base type as putting that in its own file list.py would cause imports to conflict with the list builtin. Not sure how to fix this and whether having its plugin type incorrect is a big problem. In general, I think the convention that node types are based on their module path is very fragile and clearly limits how we can organize our source code. We might need to find a better solution. To be discussed I would say.

The plugin_type column of the node class is used to store the class that
should be used to load the node from the database. But more importantly
this type string, which is a period separated string is used in querying
to determine class hierarchy. Nodes with similar leading parts of their
plugin_type strings fall under the same class, and when that class is
used in querying, all classes that fall below that umbrella will also be
considered.

Having the base data types Bool, Float, Int and Str not be in their own
separate file, whose module path is what determines the plugin_type, will
break this paradigm. For example, when a user queries for all nodes of
type Int, this will also return all Bool, Float and Str objects as they
all have exactly the same plugin_type string prefix. Moving these classes
back to their own file will restore this original functionality even
though it might not be the best idea to have the plugin_type string be
determined by the module path of the class itself.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we preserve in some way back-compatibility for people loading from the old place?

@sphuber
Copy link
Contributor Author

sphuber commented Feb 27, 2018

I am not sure, because that would require importing the Int classes etc. into the aiida.orm.data.base file, but that would lead to a circular import. One way would be to move BaseType and NumericType to aiida.orm.data.__init__.py and have the Int class etc. import from there and then just use aiida.orm.data.base as a file to import the various classes. But that would still break the import for BaseType, NumericType and List.

…tion

The plugin_type column of the node class is used to store the class that
should be used to load the node from the database. But more importantly
this type string, which is a period separated string is used in querying
to determine class hierarchy. Nodes with similar leading parts of their
plugin_type strings fall under the same class, and when that class is
used in querying, all classes that fall below that umbrella will also be
considered.

Having the base data types Bool, Float, Int and Str not be in their own
separate file, whose module path is what determines the plugin_type, will
break this paradigm. For example, when a user queries for all nodes of
type Int, this will also return all Bool, Float and Str objects as they
all have exactly the same plugin_type string prefix. Moving these classes
back to their own file will restore this original functionality even
though it might not be the best idea to have the plugin_type string be
determined by the module path of the class itself.
@sphuber sphuber force-pushed the fix_1098_base_types_plugin_type_string branch from 141bb73 to 9ee390b Compare February 27, 2018 17:22
The old path to import the base types Bool, Float, Int and Str
was `aiida.orm.data.base`. To keep backwards compatibility we
need to import the classes in this file from their new individual
module files. However, that means that the BaseType class needs
to be moved out of that file, in order to prevent a circular
import dependency. Moving it to the __init__.py file of the
aiida.orm.data module works
The new ipython extension tries to import it and so the builds
on read the docs were failing
@sphuber sphuber force-pushed the fix_1098_base_types_plugin_type_string branch from 9ee390b to ab64d07 Compare February 27, 2018 17:53
@sphuber sphuber merged commit 86fe566 into aiidateam:workflows Feb 27, 2018
@sphuber sphuber deleted the fix_1098_base_types_plugin_type_string branch February 27, 2018 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants