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

Engine: fix bug when caching from process with nested outputs #5538

Merged
merged 1 commit into from
May 23, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented May 23, 2022

Fixes #5536

When the execution of a process is being skipped because it has a valid
cache source and caching is enabled, the process would except if the
process has nested output namespaces. Since the nesting cannot be
literally preserved in the database, the nesting is representing by
using double underscores between each level of nesting in the link
label.

The problem occurs when Process._create_and_setup_db_record would be
called which would create the outputs of the cloned node, however, it
would use these "collapsed" link labels containing the double
underscores. Passing these to self.out would cause the validation to
fail because the process port validation expects the actual nesting.

The solution is to restore the database nesting representation to the
one expected by Process.out, which amounts to replacing the namespace
separators.

@sphuber sphuber requested a review from ltalirz May 23, 2022 14:38
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

fantastic, thanks a lot @sphuber !

_, node_clone = run.get_node(NestedOutputsProcess, a=Int(1))

assert node_clone.is_created_from_cache
assert node_clone.get_cache_source() == node_original.uuid
Copy link
Member

@ltalirz ltalirz May 23, 2022

Choose a reason for hiding this comment

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

do you maybe want to check here that the structure of the cloned node outputs is as expected?
I guess it's fine, just to be safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test

When the execution of a process is being skipped because it has a valid
cache source and caching is enabled, the process would except if the
process has nested output namespaces. Since the nesting cannot be
literally preserved in the database, the nesting is representing by
using double underscores between each level of nesting in the link
label.

The problem occurs when `Process._create_and_setup_db_record` would be
called which would create the outputs of the cloned node, however, it
would use these "collapsed" link labels containing the double
underscores. Passing these to `self.out` would cause the validation to
fail because the process port validation expects the actual nesting.

The solution is to restore the database nesting representation to the
one expected by `Process.out`, which amounts to replacing the namespace
separators.
@sphuber sphuber force-pushed the fix/5536/cache-nested-outputs branch from 609c4ec to b629ef3 Compare May 23, 2022 15:19
@sphuber sphuber requested a review from ltalirz May 23, 2022 15:40
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@sphuber sphuber merged commit b936da3 into aiidateam:main May 23, 2022
@sphuber sphuber deleted the fix/5536/cache-nested-outputs branch May 23, 2022 20:38
sphuber added a commit that referenced this pull request Sep 22, 2022
When the execution of a process is being skipped because it has a valid
cache source and caching is enabled, the process would except if the
process has nested output namespaces. Since the nesting cannot be
literally preserved in the database, the nesting is representing by
using double underscores between each level of nesting in the link
label.

The problem occurs when `Process._create_and_setup_db_record` would be
called which would create the outputs of the cloned node, however, it
would use these "collapsed" link labels containing the double
underscores. Passing these to `self.out` would cause the validation to
fail because the process port validation expects the actual nesting.

The solution is to restore the database nesting representation to the
one expected by `Process.out`, which amounts to replacing the namespace
separators.

Cherry-pick: b936da3
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.

Adding to dynamic output_namespace results in ValidationError
2 participants