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

Make Groups great again #2329

Merged
merged 2 commits into from
Dec 13, 2018
Merged

Make Groups great again #2329

merged 2 commits into from
Dec 13, 2018

Conversation

yakutovicha
Copy link
Contributor

@yakutovicha yakutovicha commented Dec 7, 2018

Fixes #2075 and fixed #160

  • aiida.orm.importexport

    • While importing data from the export file allow to specify user-defined
      group and to put all the imported data in this group
  • aiida.common.utils

    • Remove get_group_type_mapping function which was mapping
      machine-specific group names with the user-friendly ones
    • Add escape_for_sql_like that escapes % or _ symbols provided by user
  • aiida.orm.groups

    • Add GroupTypeString enum which contains all allowed group types:
      data.upf (was data.upf.family)
      auto.import (was aiida.import)
      auto.run (was autogroup.run)
      user (was empty string)
    • Remove Group.query and Group.group_query methods, as they are redundant
  • aiida.orm.data.upf:

    • Set UPFGROUP_TYPE to GroupTypeString.UPFGROUP_TYPE
    • Replace the usage of Group.query by QueryBuilder in get_upf_groups
      and get_upf_family_names methods
  • aiida.orm.autogroup:

    • set VERDIAUTOGROUP_TYPE to GroupTypeString.VERDIAUTOGROUP_TYPE
  • aiida.cmdline.commands.cmd_group

    • Add verdi group copy
    • Add option to show all available group types
    • Add defaulf for group_type option
    • Replace Group.query with QueryBuilder in verdi group list
    • Remove usage of the get_group_type_mapping() function
  • aiida.cmdline.commands.cmd_import

    • Add the possibility to define a group that will contain all imported nodes.
  • aiida.cmdline.params.types.group

    • Add the possibility to the GroupParamType to create groups if they don't exist
  • aiida.backend*:

    • Rename type and name to type_string and label for the database models
  • Improve documentation for django and sqla backends migration

@yakutovicha
Copy link
Contributor Author

Also fixes #2075.

Group documentation is still missing

@yakutovicha
Copy link
Contributor Author

This commit should also fix the ambiguity of Group type as reported in #160 since we now rename type with type_string

@yakutovicha
Copy link
Contributor Author

yakutovicha commented Dec 8, 2018

@giovannipizzi I looked at all Group-related issues and commented them in this PR. What is still missing is:

  • add group documentation. Btw @ltalirz what is the right place for it? Looks like now it is quite significantly reshaped.

  • Fix all the tests that fail because of the group name->label and type->type_string change

@ltalirz
Copy link
Member

ltalirz commented Dec 9, 2018

@yakutovicha I suggest to add a section Grouping Data under Working with AiiDA that explains how one should use groups, labels, etc. in AiiDA.

Btw, you can now inspect the docs of the provenance-redesign branch here:
https://aiida-core.readthedocs.io/en/provenance_redesign/

@yakutovicha
Copy link
Contributor Author

@yakutovicha I suggest to add a section Grouping Data under Working with AiiDA that explains how one should use groups, labels, etc. in AiiDA.

Btw, you can now inspect the docs of the provenance-redesign branch here:
https://aiida-core.readthedocs.io/en/provenance_redesign/

thanks @ltalirz

@yakutovicha
Copy link
Contributor Author

yakutovicha commented Dec 10, 2018

As we discussed today with @giovannipizzi I should also:

  • add name and type as deprecated options to ensure the back-compatibility

@coveralls
Copy link

coveralls commented Dec 11, 2018

Coverage Status

Coverage increased (+0.005%) to 68.191% when pulling 6c525c8 on yakutovicha:provenance_redesign into f9b9b8b on aiidateam:provenance_redesign.

@yakutovicha
Copy link
Contributor Author

thanks @sphuber, your trick with auto groups did work

@yakutovicha
Copy link
Contributor Author

I think it is now done. @sphuber would you like to review the PR? I am not sure whether @giovannipizzi will have time for that.

@yakutovicha
Copy link
Contributor Author

yakutovicha commented Dec 11, 2018

I am not sure why travis fails from time to time. I restarted the test and everything went fine.

@sphuber
Copy link
Contributor

sphuber commented Dec 12, 2018

@yakutovicha we merged migrations yesterday, so there will be conflicts if we update the branch. Please rebase your branch on the current provenance_redesign and squash your commits, then I will have a look. Thanks

@yakutovicha
Copy link
Contributor Author

@yakutovicha we merged migrations yesterday, so there will be conflicts if we update the branch. Please rebase your branch on the current provenance_redesign and squash your commits, then I will have a look. Thanks

it is done now

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Very nice work @yakutovicha , I have a few comments, but should not take too long to address them

aiida/cmdline/commands/cmd_group.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_group.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_group.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_group.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_group.py Outdated Show resolved Hide resolved
aiida/orm/groups.py Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
1) aiida.orm.importexport: while importing data from the export
file allow to specify user-defined group and to put all the imported
data in this group

2) aiida.common.utils:
   a) remove get_group_type_mapping function which was mapping
machine-specific group names with the user-friendly ones

3) aiida.orm.groups
   a) Add GroupTypeString (Enum) which contains all allowed group
types: data.upf (previously was 'data.upf.family') , auto.import
(previously was 'aiida.import'), auto.run (previously was
'autogroup.run'), user (previously was '')
   b) Remove Group.query and Group.group_query functions, as they are
redundant. Their functionality can be replaced by QueryBuilder()

3) aiida.orm.data.upf:

   a) Set UPFGROUP_TYPE from GroupTypeString.UPFGROUP_TYPE.value
defined in aiida.common.utils
   b) Replace the usage of Group.query() by QueryBuilder() in
get_upf_groups and get_upf_family_names functions

4) aiida.orm.autogroup: set VERDIAUTOGROUP_TYPE from
GroupTypeString.VERDIAUTOGROUP_TYPE.value defined in aiida.common.utils

5) aiida.common.utils: add escape_for_sql_like that escapes
% or _ symbols provided by user

6) aiida.cmdline.commands.cmd_group.py

   a) Add copy option
   b) "list": replace Group.query() with QueryBuilder
   c) Add option to show all available group types
   d) Add defaulf for group_type option
   e) remove usage of the get_group_type_mapping() function

7) aiida.cmdline.commands.cmd_import.py: add a posibility to chose the
group where all the imported nodes will be put in.

8) aiida.cmdline.params.types.group.py add a possibility to the group
parameter to create new group

9) dj and sqla backends: in Group (and tests, resapi) replace name
with label and type with type_string

10) Add Groups documentation

11) Keep name and type for back-compatibility

12) Improve documentation for django and sqla backends migration
@yakutovicha
Copy link
Contributor Author

@sphuber I implemented your suggestions, please review it again.

@sphuber sphuber merged commit 65dfecb into aiidateam:provenance_redesign Dec 13, 2018
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.

4 participants