Skip to content

Make Groups great again#2329

Merged
sphuber merged 2 commits into
aiidateam:provenance_redesignfrom
yakutovicha:provenance_redesign
Dec 13, 2018
Merged

Make Groups great again#2329
sphuber merged 2 commits into
aiidateam:provenance_redesignfrom
yakutovicha:provenance_redesign

Conversation

@yakutovicha
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Also fixes #2075.

Group documentation is still missing

@yakutovicha
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor Author

thanks @sphuber, your trick with auto groups did work

@yakutovicha
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread aiida/cmdline/commands/cmd_group.py Outdated
Comment thread aiida/cmdline/commands/cmd_group.py Outdated
Comment thread aiida/cmdline/commands/cmd_group.py Outdated
Comment thread aiida/cmdline/commands/cmd_group.py Outdated
Comment thread aiida/cmdline/commands/cmd_group.py Outdated
Comment thread aiida/orm/groups.py Outdated
Comment thread .pre-commit-config.yaml Outdated
Comment thread aiida/backends/djsite/db/migrations/0021_dbgroup_name2label_type2type_string.py Outdated
Comment thread aiida/backends/djsite/db/migrations/0022_dbgroup_type_string_change_content.py Outdated
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
Copy Markdown
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