Do not allow the copy or deepcopy of Node, except for Data #1705
Merged
Conversation
sphuber
added
type/bug
priority/critical-blocking
sphuber
force-pushed
the
fix_1699_copy_deepcopy_nodes
branch
3 times, most recently
from
63db99b to
24ca54e
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1705 +/- ##
===========================================
+ Coverage 56.4% 56.41% +0.01%
===========================================
Files 275 275
Lines 33931 33912 -19
===========================================
- Hits 19139 19132 -7
+ Misses 14792 14780 -12
Continue to review full report at Codecov.
|
Contributor
|
Why is this blocked Seb? Do you want a review or should it wait? |
Contributor
Author
|
It was blocked by #1704 but that is now merged, so I will rebase and then it is ready for review |
sphuber
force-pushed
the
fix_1699_copy_deepcopy_nodes
branch
from
24ca54e to
6e332b9
Compare
sphuber
changed the title
sphuber
force-pushed
the
fix_1699_copy_deepcopy_nodes
branch
from
6e332b9 to
80cca3b
Compare
muhrin
previously requested changes
Jul 3, 2018
Contributor
There was a problem hiding this comment.
Can we raise on is_stored asking the user to use clone instead.
Contributor
Author
|
@muhrin I have addressed the issue and its ready for review |
sphuber
force-pushed
the
fix_1699_copy_deepcopy_nodes
branch
from
9abe1c7 to
ced9d11
Compare
We explicitly disallow calling the functions copy and deepcopy from the python copy module on a Node instance. The reason is that the behavior of this for a Calculation node, with respect to what should be returned and how this would affect the graph, is not clear. Rather, to clone a Calculation, the caching mechanism should be used or a new ProcessBuilder could be generated from a completed Process that would recreate the necessary inputs that could then easily be relaunched. For Data nodes, the behavior can be defined a bit better. Therefore we allow to call deepcopy on a Data node, which will call the internal clone method, which will return an identical, but unstored, clone of the original Data node. The clone will have no links and a newly generated UUID.
Deep copying an unstored node is fine however and just pipes through to the clone method.
sphuber
force-pushed
the
fix_1699_copy_deepcopy_nodes
branch
from
ced9d11 to
220c3ea
Compare
Contributor
Author
|
@giovannipizzi could you please give this a look so it can be merged. It can be quite a problem for people running |
giovannipizzi
approved these changes
Jul 6, 2018
Member
giovannipizzi
left a comment
giovannipizzi
left a comment
There was a problem hiding this comment.
Good for me!
Does this need also @muhrin's review to get unlocked?
Contributor
Author
|
He already was agreed provided I made the one update, which I did, so I am pretty sure he is on board_ |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 #1699
We explicitly disallow calling the functions copy and deepcopy
from the python copy module on a Node instance. The reason is that
the behavior of this for a Calculation node, with respect to what
should be returned and how this would affect the graph, is not clear.
Rather, to clone a Calculation, the caching mechanism should be
used or a new ProcessBuilder could be generated from a completed
Process that would recreate the necessary inputs that could then
easily be relaunched.
For Data nodes, the behavior can be defined a bit better. Therefore
we allow to call deepcopy on a Data node, which will call the internal
clone method, which will return an identical, but unstored, clone of
the original Data node. The clone will have no links and a newly
generated UUID.