Rerun errored #483

chasejohnson3 · 2021-06-22T05:20:01Z

Acknowledgment

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

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

When a workflow or task errors and its result is stored in cache, rerunning the same workflow/task again should rerun all
errored tasks and workflows (as discussed with @satra in #482 )

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)

codecov · 2021-06-22T05:24:04Z

Codecov Report

Merging #483 (1917c7b) into master (0b04846) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
+ Coverage   82.85%   82.99%   +0.14%     
==========================================
  Files          20       20              
  Lines        4035     4035              
  Branches     1116     1116              
==========================================
+ Hits         3343     3349       +6     
+ Misses        494      491       -3     
+ Partials      198      195       -3

Flag	Coverage Δ
unittests	`82.92% <100.00%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pydra/engine/core.py	`89.10% <100.00%> (+0.65%)`	⬆️
pydra/engine/specs.py	`88.24% <0.00%> (+0.47%)`	⬆️

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 0b04846...1917c7b. Read the comment docs.

satra · 2021-06-22T12:56:16Z

pydra/engine/core.py

                if result is not None:
-                    return result
+                    if not result.errored:
+                        return result


perhaps change to:

if result is not None and not result.errored: return result

both here and in the next patch.

That is a good fix. It does the same thing and is more concise.

satra · 2021-06-22T12:59:13Z

pydra/engine/tests/test_task.py

+
+    task = pass_odds(name="pass_odds", x=[1, 2, 3, 4, 5], cache_dir=tmpdir).split("x")
+
+    task()


shouldn't this be removed?

Correct, the extra task() should be removed.

satra · 2021-06-22T12:59:33Z

pydra/engine/tests/test_task.py

+
+    with pytest.raises(Exception, match="even error") as exinfo:
+        task()
+    print("...")


remove print.

I kept the print("...") just to make a distinction in the stdout of running the task the first time versus the second time. print("...") could be removed, but I based the assert statements on stdout, so the assert statements would have to be adjusted.

if a print is needed, a logger instead of print so that it gets captured better by pytest.

satra · 2021-06-22T12:59:55Z

pydra/engine/tests/test_workflow.py

+
+    with pytest.raises(Exception) as exinfo:
+        wf()
+    print("...")


remove print.

I kept the print("...") just to make a distinction in the stdout of running the task the first time versus the second time. print("...") could be removed, but I based the assert statements on stdout, so the assert statements would have to be adjusted.

pydra/engine/tests/test_task.py

Also cleaned up testing code as recommended by PR comments.

chasejohnson3 · 2021-06-23T04:33:38Z

@satra I implemented most of the recommendations you made for this PR. However, I ran into issues trying to track the output of a logger inside pydra tasks, so I kept the print statements. To attempt to avoid picking up print statements from elsewhere, I count print output only containing a string used in these tests.

djarecka · 2021-06-23T16:48:21Z

pydra/engine/tests/test_workflow.py

+    wf.add(pass_odds(name="pass_odds", x=[1, 2, 3, 4, 5]).split("x"))
+    wf.set_output([("out", wf.pass_odds.lzout.out)])
+
+    with pytest.raises(Exception) as exinfo:


Suggested change

with pytest.raises(Exception) as exinfo:

with pytest.raises(Exception):

djarecka · 2021-06-23T16:48:34Z

pydra/engine/tests/test_workflow.py

+
+    with pytest.raises(Exception) as exinfo:
+        wf()
+    with pytest.raises(Exception) as exinfo:


Suggested change

with pytest.raises(Exception) as exinfo:

with pytest.raises(Exception):

djarecka · 2021-06-23T16:48:54Z

pydra/engine/tests/test_workflow.py

+        if "(error)" in line:
+            errors_found += 1
+
+    # There should have been 5 messages of the form "x%2 = XXX" after calling task() the second time


Suggested change

# There should have been 5 messages of the form "x%2 = XXX" after calling task() the second time

# There should have been 5 messages of the form "x%2 = XXX" after calling task() the first time

djarecka · 2021-06-23T16:51:06Z

pydra/engine/tests/test_workflow.py

+    # There should have been 5 messages of the form "x%2 = XXX" after calling task() the second time
+    # There should have been 2 messages of the form "x%2 = XXX" after calling task() the second time
+    assert tasks_run == 7
+


Suggested change

# There should have been 2 messages with "error" after calling task() the first time

# and 2 another messages after calling the second time

djarecka · 2021-06-23T16:53:08Z

pydra/engine/tests/test_task.py

+
+    task = pass_odds(name="pass_odds", x=[1, 2, 3, 4, 5], cache_dir=tmpdir).split("x")
+
+    with pytest.raises(Exception, match="even error") as exinfo:


Suggested change

with pytest.raises(Exception, match="even error") as exinfo:

with pytest.raises(Exception, match="even error"):

djarecka · 2021-06-23T16:53:20Z

pydra/engine/tests/test_task.py

+
+    with pytest.raises(Exception, match="even error") as exinfo:
+        task()
+    with pytest.raises(Exception, match="even error") as exinfo:


Suggested change

with pytest.raises(Exception, match="even error") as exinfo:

with pytest.raises(Exception, match="even error"):

djarecka · 2021-06-23T16:53:38Z

pydra/engine/tests/test_task.py

+        if "(error)" in line:
+            errors_found += 1
+
+    # There should have been 5 messages of the form "x%2 = XXX" after calling task() the second time


Suggested change

# There should have been 5 messages of the form "x%2 = XXX" after calling task() the second time

# There should have been 5 messages of the form "x%2 = XXX" after calling task() the first time

djarecka · 2021-06-23T16:54:10Z

pydra/engine/tests/test_task.py

+    # There should have been 5 messages of the form "x%2 = XXX" after calling task() the second time
+    # There should have been 2 messages of the form "x%2 = XXX" after calling task() the second time
+    assert tasks_run == 7
+


Suggested change

# There should have been 2 messages with "error" after calling task() the first time

# and 2 another messages after calling the second time

djarecka · 2021-06-23T16:55:57Z

@chasejohnson3 - thanks for the PR! I've added just a few suggestions

…mments.

chasejohnson3 · 2021-06-24T04:54:13Z

@djarecka Thanks for the feedback! I addressed your recommendations in the most recent commit.

djarecka · 2021-06-24T13:06:56Z

Thanks! I believe it's ready to merge? or are you planning to add anything more?

chasejohnson3 · 2021-06-24T13:23:31Z

I am not planning on making any more changes. Go ahead and merge

[email protected] added 3 commits June 22, 2021 00:12

Rerunning errored tasks and workflows.

15be31e

Rerunning errored tasks and workflows.

8db3414

Fixed merge conflict with commented-out code.

8ed7ce4

satra reviewed Jun 22, 2021

View reviewed changes

pydra/engine/tests/test_task.py Outdated Show resolved Hide resolved

satra and others added 3 commits June 22, 2021 09:00

Update pydra/engine/tests/test_task.py

05ffb61

Made testing print messages more specialized for the rerun test.

bff4b4f

Also cleaned up testing code as recommended by PR comments.

Modified rerun_errored implementation and tests to follow PR comments.

4f63c20

Removed logging from imports.

80c9019

djarecka reviewed Jun 23, 2021

View reviewed changes

Removing unused exception variable and correcting mistakes in test co…

1917c7b

…mments.

djarecka merged commit 996f5a2 into nipype:master Jun 24, 2021

chasejohnson3 mentioned this pull request Jun 28, 2021

Rerun Errored Tasks #482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rerun errored #483

Rerun errored #483

chasejohnson3 commented Jun 22, 2021

codecov bot commented Jun 22, 2021 •

edited

Loading

satra Jun 22, 2021

chasejohnson3 Jun 22, 2021

satra Jun 22, 2021

chasejohnson3 Jun 22, 2021

satra Jun 22, 2021

chasejohnson3 Jun 22, 2021

satra Jun 22, 2021

satra Jun 22, 2021

chasejohnson3 Jun 22, 2021

chasejohnson3 commented Jun 23, 2021

djarecka Jun 23, 2021

djarecka Jun 23, 2021

djarecka Jun 23, 2021

djarecka Jun 23, 2021

djarecka Jun 23, 2021

djarecka Jun 23, 2021

djarecka Jun 23, 2021

djarecka Jun 23, 2021

djarecka commented Jun 23, 2021

chasejohnson3 commented Jun 24, 2021

djarecka commented Jun 24, 2021

chasejohnson3 commented Jun 24, 2021


		task = pass_odds(name="pass_odds", x=[1, 2, 3, 4, 5], cache_dir=tmpdir).split("x")

		task()

	with pytest.raises(Exception) as exinfo:
	with pytest.raises(Exception):

	# There should have been 5 messages of the form "x%2 = XXX" after calling task() the second time
	# There should have been 5 messages of the form "x%2 = XXX" after calling task() the first time


	# There should have been 2 messages with "error" after calling task() the first time
	# and 2 another messages after calling the second time


		task = pass_odds(name="pass_odds", x=[1, 2, 3, 4, 5], cache_dir=tmpdir).split("x")

		with pytest.raises(Exception, match="even error") as exinfo:

Rerun errored #483

Rerun errored #483

Conversation

chasejohnson3 commented Jun 22, 2021

Acknowledgment

Types of changes

Summary

Checklist

codecov bot commented Jun 22, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chasejohnson3 commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djarecka commented Jun 23, 2021

chasejohnson3 commented Jun 24, 2021

djarecka commented Jun 24, 2021

chasejohnson3 commented Jun 24, 2021

codecov bot commented Jun 22, 2021 •

edited

Loading