Support current link types/link rules (support for workchains and workflows) at the import/export #1764

szoupanos · 2018-07-18T16:36:00Z

This pull request implements the new export rules for nodes and links that take into account the changes for the correct export (and import) of workchains and workfunctions.

More info can be found at issues #1102 #1758

(Please don't merge it until I do some final cleaning - I will remove the BLOCKED flag)

…k what we do about the exported links since not all the predecessors are now exported (e.g. Calc-Calc in the reverse way is not exported)

… parameter format

…he links

…nced by a Calc or if it is exported independently). Tests added

…ceptions that make the import of a bunch of import files to stop

codecov-io · 2018-07-18T16:43:37Z

Codecov Report

Merging #1764 into release_v0.12.2 will increase coverage by 0.02%.
The diff coverage is 62.2%.

@@                 Coverage Diff                 @@
##           release_v0.12.2    #1764      +/-   ##
===================================================
+ Coverage            54.48%   54.51%   +0.02%     
===================================================
  Files                  246      246              
  Lines                32441    32602     +161     
===================================================
+ Hits                 17677    17772      +95     
- Misses               14764    14830      +66

Impacted Files	Coverage Δ
aiida/cmdline/commands/rehash.py	`33.33% <0%> (ø)`	⬆️
aiida/common/archive.py	`18.18% <0%> (-0.38%)`	⬇️
aiida/cmdline/commands/importfile.py	`7.69% <0%> (-0.42%)`	⬇️
aiida/tools/dbexporters/tcod.py	`55.87% <100%> (ø)`	⬆️
aiida/cmdline/commands/exportfile.py	`11.74% <42.85%> (+0.71%)`	⬆️
aiida/orm/importexport.py	`76.21% <66.12%> (-2.65%)`	⬇️
aiida/backends/sqlalchemy/models/group.py	`86.66% <0%> (-3.34%)`	⬇️
aiida/orm/data/base.py	`89.74% <0%> (ø)`	⬆️
aiida/orm/implementation/general/code.py	`65.85% <0%> (+1.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 003caf8...2fcc0e7. Read the comment docs.

…into issue_1758

szoupanos · 2018-07-19T13:41:26Z

This is thoroughly tested in Django. When importing the export files mentioned above to an SQLA profile, I get exceptions similar to those reported at issue #1710

szoupanos · 2018-07-20T05:31:49Z

After the fix for #1710 at PR #1769, import finished correctly at SQLA (without exceptions).
I imported several groups from a Django profile to an SQLA profile and I have to check the results
There are problems with the hashing under SQLA - I will create a separate ticket.

szoupanos · 2018-08-10T15:47:29Z

Any hashing problems found were reported at issue #1771 and solved at the corresponding pull request. Import for SQLA also worked OK.

I exported the sample Django database that I got from Giovanni, and mentioned above, imported it to SQLA and the nodes and links seem to be imported correctly. Rehashing the SQLA database failed so node hashes were not checked.

szoupanos · 2018-08-12T22:36:43Z

Re-hashing problems were fixed and the aforementioned hashes match.

szoupanos · 2018-08-12T22:50:23Z

If there are no objections (i.e. from Leopold - @ltalirz), I would say that this should be merged (since it is a lot of work and starts to be old enough, the default scenario seems to work without any problems under Django and SQLA etc). Any smaller problems found can be fixed in different tickets/issues.

ltalirz · 2018-08-13T08:40:52Z

@szoupanos no objections, your Honor :-)
I can approve but perhaps you want to wait for the approval of someone more familiar with the details?

sphuber

Aside from a few minor comments that should be easy to fix, I think that we should really put some effort soon towards refactoring this code. The import_tree and export_tree are enormous and make it difficult to follow. I wouldn't require it for this PR, but I do think this has to happen soon. Graph traversal based on different sets of rules (and export/import that is just an extension of that) is an integral part of AiiDA and should be robust. Therefore it would serve us well if this is abstracted and defined rigorously in one place, which export/import functionality can then just leverage.

sphuber · 2018-08-13T09:09:04Z

aiida/cmdline/commands/exportfile.py

-    help='Store only the nodes that are explicitly given, without exporting the parents')
-@click.option('-O', '--no-calc-outputs', is_flag=True, default=False,
-    help='If a calculation is included in the list of nodes to export, do not export its outputs')
+@click.option('-IF', '--input_forward', is_flag=True, default=False,


We should try to maintain the convention of only using single letters for single-letter-options.

yes, but it seems that there are overlaps and many of the single letters are already reserved. For example which letter would you give to --call_reversed and to --create_reversed flags. Furthermore, I would like the flags that override the default graph traversal for the export to be as uniform (among them) as possible.

You could use -I, -C, -R and -X for example. Note that they don't overlap with any of the other flags in this command, nor are they necessarily less intuitive, specially given the long form is available. Which also the long form options should not use underscores, i.e. --create_reversed should be --create-reversed

Please also consider adding no single-letter version.
These are just abbreviations, which make sense for the options that appear in many places (and people will thus memorize their meaning)

Agreed, although they have another function besides easy memorization. If an option is used very often, having a short-hand single letter version is a lot faster to use, even with autocomplete functionality

For reference the POSIX cli flag convention that we maintain: https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html

sphuber · 2018-08-13T09:09:46Z

aiida/cmdline/commands/exportfile.py

+@click.option('-IF', '--input_forward', is_flag=True, default=False,
+    help='Follow forward INPUT links (recursively) when calculating the node '
+         "set to export. By default this is switched off.")
+@click.option('-CrR', '--create_reversed', is_flag=True, default=True,


You can use show_default=True to have click automatically generate the default value in the help string

sphuber · 2018-08-13T09:10:16Z

aiida/cmdline/commands/exportfile.py

-    from aiida.backends.utils import load_dbenv
-    load_dbenv()
+    from aiida.backends.utils import load_dbenv, is_dbenv_loaded
+    if not is_dbenv_loaded():


please replace this with the aiida.cmdline.utils.decorators.with_dbenv decorator

Scratch that, that only exists in develop. We should make sure that we update this when we merge 0.12.2 into develop

OK, we will add it when it goes to develop - I will add a comment at that line. Do we have any other ways to remember it?

sphuber · 2018-08-13T09:21:23Z

aiida/orm/data/__init__.py

@@ -97,6 +97,8 @@ def add_link_from(self, src, label=None, link_type=LinkType.UNSPECIFIED):

        if link_type is LinkType.CREATE and \
                        len(self.get_inputs(link_type=LinkType.CREATE)) > 0:
+            for l in self.get_inputs(link_type=LinkType.CREATE):
+                print "=====> ", l, l.dbnode


Is this print supposed to stay?

sphuber · 2018-08-13T09:23:25Z

aiida/backends/tests/export_and_import.py

@@ -1629,20 +1809,27 @@ def test_links_for_workflows(self):
            o1.add_link_from(w1, 'return', link_type=LinkType.RETURN)

            uuids_wanted = set(_.uuid for _ in (w1,o1,i1))
-            links_wanted = [l for l in self.get_all_node_links() if l[3] in ('createlink', 'inputlink')]
+            # links_wanted = [l for l in self.get_all_node_links() if l[3] in


sphuber · 2018-08-13T09:25:18Z

aiida/orm/importexport.py

@@ -381,6 +386,10 @@ def import_data_dj(in_path,ignore_unknown_nodes=False,
                                 "is neither a (possibly compressed) tar file, "
                                 "nor a zip file.")

+        if not folder.get_content_list():
+            print "The provided file/folder is empty. Exiting silently"


"Exiting silently" while printing is a bit weird no? Would it be better to raise an exception and catch it in the caller, who can then decide what to print to the user.

The main idea is to avoid a more "violent" exception that will occur later when the metadata.json will not be found at the folder. The silent exit makes sense when running import in a script and an empty import files, doesn't stop a full script of 100 imports to run.

I can of course raise an exception and catch it and exit silently at import_data or one level above but I don't see a lot of advantages. The only advantage that I see is that I can unify the behaviour of import_data_dj & import_data_sqla.

Since these functions will be called mostly from verdi commands, we need to be able to control how the output is printed. Hard coding print statements in these functions makes that impossible. Just above this line, a ValueError is also raised to signal that the passed export file or folder is unusable. I don't see a reason why not to do the same here. But I notice that there are currently a load more print statements, so unless all of those are refactored, I guess changing just this one line won't accomplish too much

sphuber · 2018-08-13T09:43:50Z

aiida/orm/importexport.py

+        # If it is a Data node
+        else:
+            curr_node_id = to_be_visited[Data].pop()
+            # If it is already visited continue to the next node


duplicate line

sphuber · 2018-08-13T09:44:12Z

aiida/orm/importexport.py

-
-    :param what: a list of Django database entries; they can belong to different
-      models.
+    :param what: a list of Django database entries; they can belong to


This is a backend independent function right?

szoupanos · 2018-08-13T11:40:09Z

@sphuber Thanks for the comments! For the general comment, I would agree if the necessary manpower is found.

ltalirz · 2018-08-13T13:59:41Z

aiida/cmdline/commands/exportfile.py

@@ -41,20 +41,33 @@ def cli(self, *args):
    help='Export the given groups by pk')
 @click.option('-g', '--group_names', multiple=True, type=str,


Could you please also replace underscores with dashes in option names?
This is the way it's done in aiida/cmdline/params/options and we should adopt it aiida-wide.

Done to all the arguments of verdi export

It's not pushed, right?

szoupanos · 2018-08-13T16:38:36Z

OK, I think that I addressed everything.

sphuber · 2018-08-13T21:58:20Z

Thanks a million @szoupanos

szoupanos added 22 commits April 27, 2018 15:07

Reorganising/extending the import/export tests

15a4fa2

More progress on export graph traversal

16b7bc0

Changing the export to accept AiiDA objects instead of Db objects

0fbc0c1

Adding the new graph traversal logic to the export function

f4a8963

Small corrections

708e97b

Various corrections to the graph traversal algorithm. We have to thin…

e1d8ba7

…k what we do about the exported links since not all the predecessors are now exported (e.g. Calc-Calc in the reverse way is not exported)

More progress on link export

4390d89

Link export seems to work?

ae8d436

All tests pass

36e9700

Activating some more tests

1dcd750

Cleaning up

130e99e

Small changes to the make the TCOD tests to use the new import/export…

1cbadfe

… parameter format

Removing not needed code and modifying export_tree signature

b2bbf67

Cleaning the code, making docs to run without warnings

3234389

Adding optinal rules to graph exporation. It remains to be done for t…

3b4de69

…he links

Adding optional rules for link extraction

5f366cb

A Code is treated as a Data node (and can be exported if it is refere…

a0a9590

…nced by a Calc or if it is exported independently). Tests added

Adding correct export of code related links. Tests added

06490d2

Merge remote-tracking branch 'origin/release_v0.12.2' into issue_1102_b

bf1f961

Small changes to make tests work again after the merge of 0.12.2

19d0042

Proper handling of empty import files. There is no need for strict ex…

4e82a78

…ceptions that make the import of a bunch of import files to stop

Minor fix for the previous commit.

831fa18

szoupanos added topic/workflows topic/sharing labels Jul 18, 2018

szoupanos added this to the v0.12.2 milestone Jul 18, 2018

szoupanos self-assigned this Jul 18, 2018

This was referenced Jul 18, 2018

Import/export should support current link types/link rules (support for workchains and workflows) #1758

Closed

importing same aiida export file twice gives error #1710

Closed

szoupanos added 2 commits July 19, 2018 11:11

Merge branch 'issue_1758' of https://github.com/szoupanos/aiida_core …

7668131

…into issue_1758

Small identation changes to make shinx tests pass

bfbc086

szoupanos and others added 3 commits July 20, 2018 12:26

Merge remote-tracking branch 'origin/release_v0.12.2' into issue_1758

d8ba932

Merge branch 'release_v0.12.2' into issue_1758

042029e

Merge remote-tracking branch 'origin/release_v0.12.2' into issue_1758

de90377

Small correction to make rehash command to work under SQLA

bfaa84a

szoupanos changed the title ~~[BLOCKED] Support current link types/link rules (support for workchains and workflows) at the import/export~~ Support current link types/link rules (support for workchains and workflows) at the import/export Aug 12, 2018

szoupanos requested review from sphuber and giovannipizzi August 13, 2018 09:05

sphuber requested changes Aug 13, 2018

View reviewed changes

ltalirz suggested changes Aug 13, 2018

View reviewed changes

szoupanos and others added 2 commits August 13, 2018 18:37

Addressing code review comments

ac085f2

Merge branch 'release_v0.12.2' into issue_1758

ae92abe

ltalirz previously approved these changes Aug 13, 2018

View reviewed changes

Minor correction to fix doc compilation

2fcc0e7

szoupanos dismissed ltalirz’s stale review via 2fcc0e7 August 13, 2018 17:24

sphuber approved these changes Aug 13, 2018

View reviewed changes

sphuber merged commit 915dcf9 into aiidateam:release_v0.12.2 Aug 13, 2018

szoupanos deleted the issue_1758 branch February 9, 2019 14:37

ltalirz mentioned this pull request Mar 8, 2019

verdi export not storing full provenance #2586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support current link types/link rules (support for workchains and workflows) at the import/export #1764

Support current link types/link rules (support for workchains and workflows) at the import/export #1764

szoupanos commented Jul 18, 2018

codecov-io commented Jul 18, 2018 •

edited

Loading

szoupanos commented Jul 19, 2018

szoupanos commented Jul 20, 2018

szoupanos commented Aug 10, 2018

szoupanos commented Aug 12, 2018

szoupanos commented Aug 12, 2018 •

edited

Loading

ltalirz commented Aug 13, 2018

sphuber left a comment

sphuber Aug 13, 2018

szoupanos Aug 13, 2018

sphuber Aug 13, 2018

ltalirz Aug 13, 2018

sphuber Aug 13, 2018

sphuber Aug 13, 2018

szoupanos Aug 13, 2018

sphuber Aug 13, 2018

szoupanos Aug 13, 2018

sphuber Aug 13, 2018

sphuber Aug 13, 2018

szoupanos Aug 13, 2018

sphuber Aug 13, 2018

sphuber Aug 13, 2018

sphuber Aug 13, 2018

szoupanos Aug 13, 2018

sphuber Aug 13, 2018

sphuber Aug 13, 2018

sphuber Aug 13, 2018

szoupanos commented Aug 13, 2018

ltalirz Aug 13, 2018

szoupanos Aug 13, 2018

ltalirz Aug 13, 2018

ltalirz Aug 13, 2018

szoupanos commented Aug 13, 2018

sphuber commented Aug 13, 2018

		@@ -41,20 +41,33 @@ def cli(self, *args):
		help='Export the given groups by pk')
		@click.option('-g', '--group_names', multiple=True, type=str,

Support current link types/link rules (support for workchains and workflows) at the import/export #1764

Support current link types/link rules (support for workchains and workflows) at the import/export #1764

Conversation

szoupanos commented Jul 18, 2018

codecov-io commented Jul 18, 2018 • edited Loading

Codecov Report

szoupanos commented Jul 19, 2018

szoupanos commented Jul 20, 2018

szoupanos commented Aug 10, 2018

szoupanos commented Aug 12, 2018

szoupanos commented Aug 12, 2018 • edited Loading

ltalirz commented Aug 13, 2018

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szoupanos commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szoupanos commented Aug 13, 2018

sphuber commented Aug 13, 2018

codecov-io commented Jul 18, 2018 •

edited

Loading

szoupanos commented Aug 12, 2018 •

edited

Loading