Multiple RETURN links import now possible

[CasperWA]

Fixes #3353

[sphuber]

So if I understand correctly, the only real change was that the if-condition was comparing to LinkType.RETURN instead of LinkType.RETURN.value and therefore was always false, causing an unnecessary exception being raised. Is that correct @CasperWA ?

Unfortunately, looking at the current code, there are a lot of problems that really should be addressed sooner rather than later. The whole point of #1762 was to get to a state where we know what we export and import, but seeing this code I am afraid we do not actually have much of an idea. But I agree that we cannot tackle that right now. So for me also fine to merge this now

[CasperWA]

So if I understand correctly, the only real change was that the if-condition was comparing to LinkType.RETURN instead of LinkType.RETURN.value and therefore was always false, causing an unnecessary exception being raised. Is that correct @CasperWA ?

This is correct.

Unfortunately, looking at the current code, there are a lot of problems that really should be addressed sooner rather than later. The whole point of #1762 was to get to a state where we know what we export and import, but seeing this code I am afraid we do not actually have much of an idea. But I agree that we cannot tackle that right now. So for me also fine to merge this now

Yeah, I have been trying slowly to take out each step of the export and import functions, where possible, trying to create backend-agnostic utility functions. This should hopefully make each step of the process easier to deal with and understand, however, it is taking forever to do.

[sphuber]

I understand and already your work has made the organization of the code and the code itself a lot better, but eventually small steps won't solve this particular problem. At some point we will have to face the music and properly design an interface for export/import that matches ORM implementation but that allows for efficient bulk inserts. This is a big challenge, but is the only way to once and for all fix it.

[CasperWA]

I completely agree.

[CasperWA]

@sphuber @giovannipizzi this is ready for review. I have re-implemented the validate_link function from aiida.orm.utils.links. Note that for "unique_triple" Links, this is essentially similar to stating "this Link already exists", so if the util function link_triple_exists returns True, the import simply skips the Link in question instead of raising.

It is not the fastest, nor is it the prettiest fix, but it should be more rigorous in testing the created Links from import.

[CasperWA]

I got insecure whether the ad hoc rules would actually work, feeling there is something missed between the usage of .get_outgoing and .get_incoming in the actual validate_links() function.
It seemed to me that if the Link type and label was the same for an existing incoming Link for a Node and we are only concerned with the existing outgoing Links being similar (possible only for CALL_WORK, I think), then the ad hoc rules here would not catch the distinction and simply raise, while it didn't need to.

However, when writing the test, I came to the realization that there would never be a situation where this is a possible case, due to a combination of the fixed Link rule to follow CALL_WORK forward and the fact that only sealed Nodes may be exported.
I.e., a graph:

work1 -> work2 -> work3

cannot be exported, following an export of

work1 -> work2 -> work3
           |
           +-> work4

with the same Node UUIDS, since they must be sealed - which means the Link between work2 and work4 cannot be created after export of the first graph.
On the other hand, the sub-graph equal to the first graph cannot be exported from the second graph due to the Link follow rules (non-changeable call_work_forward=True, changeable call_work_backward=False).

[sphuber]

One more thing to fix and then this is good to go

[sphuber]

The link checking itself now seems correct, very nice. There is just one thing we realized. In this logic we should not rely on export logic, i.e. what export can legally export. So for example, we should not assume that the contents of an archive are necessarily consistent. This means we have to check that the links within an archive are also consistent. To do this, we simply have to update the existing_ sets at this point. Then in the next iterations, the added link will be included in the existence check and if the archive itself contains duplicate links, it will be noticed.

[CasperWA]

Done. I have done the same for the SQLAlchemy backend, however, the only way to do this was to not "bulk create" the Links after the for loop, but instead add the new Link in every run using session.add(Link).

[CasperWA]

Btw, also for the SQLAlchemy import, I did a check that the new Links are correctly continuously added and can be found using the QueryBuilder in the subsequent run of the for loop.

[sphuber]

Thanks a million @CasperWA great stuff ! 👍