changes in checksums/results and some fixes (fixes#107) #130

djarecka · 2019-08-17T22:57:36Z

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Summary

removing results_dict and using a new method checksum_states to get all checksums
restarting connections after running workflows
- test for workflows that are run twice (and use results right away)
- closes workflow_as_node does not update the input (if it was run already) #107

Checklist

All tests passing
I have added tests to cover my changes
I have updated documentation (if necessary)
My code follows the code style of this project
(we are using black: you can pip install pre-commit,
run pre-commit install in the pydra directory
and black will be run automatically with each commit)

Acknowledgment

I acknowledge that this contribution will be available under the Apache 2 license.

… it in Task.results(), removing self.results_dict

…after the workflow is addded to another workflow, fixing nipype#107

codecov · 2019-08-17T23:42:03Z

Codecov Report

Merging #130 into master will decrease coverage by <.01%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
- Coverage   87.08%   87.08%   -0.01%     
==========================================
  Files          16       16              
  Lines        2339     2354      +15     
  Branches      574      580       +6     
==========================================
+ Hits         2037     2050      +13     
- Misses        207      208       +1     
- Partials       95       96       +1

Flag	Coverage Δ
#unittests	`87.08% <93.33%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pydra/engine/submitter.py	`91.17% <ø> (-0.17%)`	⬇️
pydra/engine/state.py	`94.95% <100%> (+0.02%)`	⬆️
pydra/engine/core.py	`87.56% <93.1%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5c43ec...5bdbb62. Read the comment docs.

codecov · 2019-08-17T23:42:04Z

Codecov Report

Merging #130 into master will increase coverage by 0.01%.
The diff coverage is 93.93%.

@@            Coverage Diff            @@
##           master    #130      +/-   ##
=========================================
+ Coverage   87.08%   87.1%   +0.01%     
=========================================
  Files          16      16              
  Lines        2339    2358      +19     
  Branches      574     583       +9     
=========================================
+ Hits         2037    2054      +17     
- Misses        207     208       +1     
- Partials       95      96       +1

Flag	Coverage Δ
#unittests	`87.1% <93.93%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
pydra/engine/submitter.py	`91.34% <100%> (ø)`	⬆️
pydra/engine/state.py	`94.95% <100%> (+0.02%)`	⬆️
pydra/engine/core.py	`87.62% <93.33%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5c43ec...2ec3b7f. Read the comment docs.

satra · 2019-08-17T23:55:20Z

pydra/engine/core.py


    def _run(self, **kwargs):
-        self.inputs = dc.replace(self.inputs, **kwargs)
+        # self.inputs = dc.replace(self.inputs, **kwargs) don't need it?


this was there to ensure that inputs could be set via the run command (when it was not a private function) or as a function call. as long as this is the case within __call__ it would be ok to remove this. i.e. a user should be able to set inputs during the call to a task (much like a python function).

if **kwargs is not being used, it should be removed.

oh, i see, we have kwargs in the __call__ that is passing to _run, but had no tests for it, so I couldn't find who is using this. will keep it and add a test

added a test

satra · 2019-08-17T23:58:01Z

pydra/engine/core.py

        """adding a task to the workflow"""
        if not is_task(task):
            raise ValueError("Unknown workflow element: {!r}".format(task))
+        task._reset()


does this need to happen at every add call? or could this be done just prior to run or perhaps more preferably at the end of a run?

satra · 2019-08-17T23:58:28Z

pydra/engine/core.py


    async def _run(self, submitter=None, **kwargs):
-        self.inputs = dc.replace(self.inputs, **kwargs)
+        # self.inputs = dc.replace(self.inputs, **kwargs) don't need it?


same question as before.

we do not allow running a workflow without a submitter, so I believe this can't be used

satra · 2019-08-18T00:01:21Z

overall looks good. the only question i have is whether the reset can be called at the end of a run by the submitter, instead of during add. if so it may also help with the checksum issue.

checksum1  = task.checksum
result = task(param1=2, param2=3)
checksum2 = task.checksum
assert checksum1 == checksum2

djarecka · 2019-08-18T00:09:38Z

@satra - I'm just working on moving the reset to Submitter, so running workflow twice doesn't create a new output_dir, but other tests just started failing... might be related to asyncio, will try to think about it a bit more

…firm that worflowas that run twice do not create additional output_dir

…ly is set only at the beginning of execution after the graph is sorted) and adding it if needed

djarecka · 2019-08-18T03:53:41Z

@satra - at the end I just moved wf._reset to the end of Submitter.__call__. Wanted to put in more specific place but was having some issues.

about comparing the checksum, in addition to the issue with resetting the connections, there was one more thing - we added _graph_checksums to inputs, and this is usually created in the submitter when the graph has all the nodes and it can sort it.
I've added a line to checksum that allow to set this before running.

satra

LGTM. thank you for tracking this down.

adding checksum_state to calculate checksum for all states and use it…

2abdf3c

… it in Task.results(), removing self.results_dict

djarecka changed the title ~~changes in checksums/results and some fixes (fixes#107)~~ [wip] changes in checksums/results and some fixes (fixes#107) Aug 17, 2019

adding reset method to workflow, so all lazy connection can be reset …

5bdbb62

…after the workflow is addded to another workflow, fixing nipype#107

satra reviewed Aug 17, 2019

View reviewed changes

djarecka added 2 commits August 17, 2019 23:32

moving wf._reset to the end of submitter__call__; adding tests to con…

37df2bf

…firm that worflowas that run twice do not create additional output_dir

checking in checksum if inputs._graph_checksums is already set (usual…

ec52c40

…ly is set only at the beginning of execution after the graph is sorted) and adding it if needed

adding test that updates task inputs during a simple call - task()

2ec3b7f

djarecka changed the title ~~[wip] changes in checksums/results and some fixes (fixes#107)~~ changes in checksums/results and some fixes (fixes#107) Aug 18, 2019

satra approved these changes Aug 18, 2019

View reviewed changes

satra merged commit 7ffa49d into nipype:master Aug 18, 2019

djarecka deleted the checksum_resets branch December 30, 2022 20:35

changes in checksums/results and some fixes (fixes#107) #130

changes in checksums/results and some fixes (fixes#107) #130

Uh oh!

Conversation

djarecka commented Aug 17, 2019 • edited by satra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

Summary

Checklist

Acknowledgment

Uh oh!

codecov bot commented Aug 17, 2019

Codecov Report

Uh oh!

codecov bot commented Aug 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

satra Aug 17, 2019

Choose a reason for hiding this comment

Uh oh!

djarecka Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

djarecka Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

satra Aug 17, 2019

Choose a reason for hiding this comment

Uh oh!

satra Aug 17, 2019

Choose a reason for hiding this comment

Uh oh!

djarecka Aug 18, 2019

Choose a reason for hiding this comment

Uh oh!

satra commented Aug 18, 2019

Uh oh!

djarecka commented Aug 18, 2019

Uh oh!

djarecka commented Aug 18, 2019

Uh oh!

satra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

djarecka commented Aug 17, 2019 •

edited by satra

Loading

codecov bot commented Aug 17, 2019 •

edited

Loading