Fix bug in upload_calculation when running with store_provenance=False
#3513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…alse` When running a `CalcJob` in dry-run mode, it is often advised to also set `store_provenance=False` which means nodes will not be stored, so input nodes can be unstored. However, in `upload_calculation` to process the `local_copy_list` the `load_node` function was used to load the corresponding node, which would fail in this scenario. The only possible solution is to pass the full mapping of inputs to the function and if the loading of the node fails, try to fish out the corresponding node from the mapping. If all fails, we emit a warning and skip the rule.
ltalirz
reviewed
Nov 5, 2019
|
|
||
| try: | ||
| data_node = load_node(uuid=uuid) | ||
| except exceptions.NotExistent: | ||
| logger.warning('failed to load Node<{}> specified in the `local_copy_list`'.format(uuid)) | ||
| data_node = find_data_node(inputs, uuid) |
There was a problem hiding this comment.
do we want to allow this in general or only when dry_run=True?
Also, maybe good to add a comment line to both 188 and 190
There was a problem hiding this comment.
Yeah, I was wondering about that too, but I just kept the current behavior for now where it just emits a warning log. We might want to change this to actually raise. If the plugin specified that a the contents of a node need to be copied over and it is not there, that should be a problem and most likely the calculation should not work. I am not sure in what use case a mere warning is the desired alternative.
ltalirz
approved these changes
Nov 7, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3511
When running a
CalcJobin dry-run mode, it is often advised to also setstore_provenance=Falsewhich means nodes will not be stored, so inputnodes can be unstored. However, in
upload_calculationto process thelocal_copy_listtheload_nodefunction was used to load thecorresponding node, which would fail in this scenario. The only possible
solution is to pass the full mapping of inputs to the function and if
the loading of the node fails, try to fish out the corresponding node
from the mapping. If all fails, we emit a warning and skip the rule.