-
Notifications
You must be signed in to change notification settings - Fork 192
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
Fix copying of the folder in caching #1745
Conversation
folder of the cached node.
self.add_path(cache_node.get_abs_path(path), path) | ||
self.folder.replace_with_folder( | ||
cache_node.folder.get_abs_path(''), | ||
overwrite=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explicitly set move=False? Just in case we change inadvertently the default.
also, in the test above
- just as a note, raw_input will be there only for JobCalculations
- maybe you can check that 'raw_input' is still there also in the node you cached from, to avoid that by mistake we remove the files?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense.
- I knew that, but put it explicitly in the test now.
- Also done.
I guess Node.copy
was already broken for a while, but wasn't really used for much.
Cheers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: Node.copy, now removed from develop, was used to create a "copy" of the calculation, to possibly modify the inputs and resubmit - so it was intended that it didn't copy the raw_inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see.. well then I guess I saw that method and used it in a wrong way 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, doesn't that mean #1746 is wrong because it changes the behavior of copy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ops... yes! I didn't realize there you changed copy, I thought it was the implementation of caching only... could you fix?
Codecov Report
@@ Coverage Diff @@
## develop #1745 +/- ##
===========================================
- Coverage 57.16% 57.16% -0.01%
===========================================
Files 275 275
Lines 33912 33911 -1
===========================================
- Hits 19386 19385 -1
Misses 14526 14526
Continue to review full report at Codecov.
|
Fixes #1744 for the develop branch.