Hash change guards #698

tclose · 2023-09-11T03:36:49Z

Types of changes

New feature (non-breaking change which adds functionality)

Summary

Addresses #681 issue

Detects changes in the hashes of task/workflow inputs and raises informative error messages identifying which fields have changed
Reinstates behaviour of "blocked tasks" unittest, so it once again triggers "graph is not empty" exception
On "graph not empty" exception, check Workflow.inputs._graph_checksums to determine which task's checksums have changed and raise informative message

Checklist

I have added tests to cover my changes (if necessary)

codecov · 2023-09-11T04:48:49Z

Codecov Report

Attention: Patch coverage is 97.05882% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 83.93%. Comparing base (0e66136) to head (ff281aa).

Files	Patch %	Lines
pydra/engine/submitter.py	88.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #698      +/-   ##
==========================================
+ Coverage   83.65%   83.93%   +0.27%     
==========================================
  Files          24       24              
  Lines        4986     5029      +43     
  Branches     1416     1429      +13     
==========================================
+ Hits         4171     4221      +50     
+ Misses        809      802       -7     
  Partials        6        6

Flag	Coverage Δ
unittests	`83.93% <97.05%> (+0.27%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tclose · 2023-09-11T22:04:41Z

I believe this is good for review now. The new SLURM test keeps on getting stuck somewhere but I assume that it isn't related

tclose · 2023-09-12T04:22:31Z

NB: I'm not sure why the coverage is down. It seems to be an issue with the dask submitter code

ghisvail

LGTM with minor comments / questions which might be due to my own ignorance or overlook.

pydra/engine/submitter.py

pydra/engine/tests/test_specs.py

djarecka · 2023-09-24T23:51:12Z

indeed looks like the coverage has dropped, what is weird, since for some time we didn't test Dask at all...

djarecka

Thank you! I think it is really a great improvement to the debugging options for Pydra

tclose · 2023-09-25T04:04:29Z

indeed looks like the coverage has dropped, what is weird, since for some time we didn't test Dask at all...

I don't trust codecov in this instance (and others). The dask test is passing so it couldn't be missing the Dask submitter, which is where the bulk of the coverage drop comes from (otherwise it would be a coverage increase I believe)

effigies

Thanks for your patience. Overall it seems to do the job. I think cleaner approaches could be worked out, but nothing here will prevent coming up with them later.

A couple small suggestions.

pydra/utils/hash.py

pydra/engine/tests/test_submitter.py

pydra/engine/core.py

effigies

I guess I could have made suggestions...

pydra/engine/tests/test_submitter.py

…asks are blocked by referencing Workflow.inputs._graph_checksums. Reworked blocked tasks unittest so that it now raises an error again

…) in order to retain hidden _hashes member used to provided detailed information on any inadvertant hash changes pre/post run

…altered spec.hash() function

… that hash changes are being detected

…roblematic nodes

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

… a deepcopy as for some poorly behaved objects the hash of the deepcopy can be different

… ok)

pydra/engine/core.py

tclose · 2024-02-29T22:06:01Z

@djarecka I believe this is ready to merge now

for more information, see https://pre-commit.ci

djarecka · 2024-03-07T17:28:48Z

I'm good with merging, thank you @tclose !

tclose marked this pull request as ready for review September 11, 2023 12:22

tclose requested a review from effigies September 11, 2023 22:03

tclose force-pushed the hash-change-guards branch from 076fe0a to 240a9d0 Compare September 11, 2023 22:17

tclose requested review from djarecka and ghisvail September 13, 2023 12:04

ghisvail approved these changes Sep 14, 2023

View reviewed changes

pydra/engine/submitter.py Outdated Show resolved Hide resolved

pydra/engine/tests/test_specs.py Show resolved Hide resolved

pydra/engine/tests/test_specs.py Show resolved Hide resolved

djarecka approved these changes Sep 25, 2023

View reviewed changes

tclose force-pushed the hash-change-guards branch from 1849c84 to e776f6a Compare September 25, 2023 04:20

effigies approved these changes Oct 13, 2023

View reviewed changes

effigies reviewed Oct 13, 2023

View reviewed changes

pydra/engine/tests/test_submitter.py Show resolved Hide resolved

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

tclose mentioned this pull request Feb 23, 2024

hashing functions/objects defined elsewhere #717

Closed

tclose added 14 commits February 24, 2024 22:03

Implemented hash-change guards in TaskBase._run()

3a72666

added hash guards to workflow execution (i.e. on workflow inputs)

b378f83

reworked error message for blocked upstream tasks to specifiy which t…

5aaf62f

…asks are blocked by referencing Workflow.inputs._graph_checksums. Reworked blocked tasks unittest so that it now raises an error again

touched up messages for hash change errors

0397a18

replaced attrs.evolve with manual setattr of inputs within Task._run(…

1d77f4b

…) in order to retain hidden _hashes member used to provided detailed information on any inadvertant hash changes pre/post run

updated explicit hashes in unittests to match new values produced by …

e2d009b

…altered spec.hash() function

added a sleep to alter_x to make sure it runs after identity

48d68f5

fix up

1a9eed0

[skip ci] updates comment

d76bb3d

upped sleep

6e1f030

added random in to ensure that alter_x finishes after identity

6717d85

added more reruns to test_hash_changes_in_workflow_graph

1cd9491

fixed bugs in graph hash error test

7d6849b

added logging to assist in tracking down hashing issues

df9ecc6

tclose and others added 13 commits February 24, 2024 22:03

[skip ci] touched up error message

930dfc1

added docstring to bytes_repr to retrigger checks

6128951

updated bytes_repr doc string

4ba8da0

removed check for result None in TaskBase._run(). Shouldn't occur now…

15a775d

… that hash changes are being detected

added test to hit unstable hash check branch

1263f5a

expanded hash graph error message to include final values/hashes of p…

b54166e

…roblematic nodes

More touch ups to error messages

048c0a1

Update pydra/engine/tests/test_submitter.py

c282d3e

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Update pydra/engine/tests/test_submitter.py

509afc7

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

reworked hash change detection error messages

6e91065

modified modified inputs so it returns the actual original inputs not…

9896735

… a deepcopy as for some poorly behaved objects the hash of the deepcopy can be different

changed deepcopy to copy in checksum_states (not sure whether this is…

e660351

… ok)

changed test_hash_changes unittest so it works with python 3.8

8a5541c

tclose force-pushed the hash-change-guards branch from fe3373f to 8a5541c Compare February 24, 2024 11:03

changed _graph_checksums to a dict instead of a list

4e1d4a8

djarecka reviewed Feb 27, 2024

View reviewed changes

pydra/engine/core.py Show resolved Hide resolved

satra reviewed Feb 28, 2024

View reviewed changes

pydra/engine/core.py Show resolved Hide resolved

satra reviewed Feb 29, 2024

View reviewed changes

pydra/engine/core.py Show resolved Hide resolved

tclose force-pushed the hash-change-guards branch from 7f69636 to 4e1d4a8 Compare February 29, 2024 21:18

tclose and others added 2 commits March 2, 2024 14:01

Merge branch 'master' into hash-change-guards

b94f185

[pre-commit.ci] auto fixes from pre-commit.com hooks

ff281aa

for more information, see https://pre-commit.ci

tclose merged commit ff01e4c into nipype:master Mar 8, 2024
43 checks passed

tclose deleted the hash-change-guards branch March 8, 2024 05:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hash change guards #698

Hash change guards #698

tclose commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

tclose commented Sep 11, 2023

tclose commented Sep 12, 2023

ghisvail left a comment

djarecka commented Sep 24, 2023

djarecka left a comment

tclose commented Sep 25, 2023

effigies left a comment

effigies left a comment

tclose commented Feb 29, 2024

djarecka commented Mar 7, 2024

Hash change guards #698

Hash change guards #698

Conversation

tclose commented Sep 11, 2023 • edited Loading

Types of changes

Summary

Checklist

codecov bot commented Sep 11, 2023 • edited Loading

Codecov Report

tclose commented Sep 11, 2023

tclose commented Sep 12, 2023

ghisvail left a comment

Choose a reason for hiding this comment

djarecka commented Sep 24, 2023

djarecka left a comment

Choose a reason for hiding this comment

tclose commented Sep 25, 2023

effigies left a comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

tclose commented Feb 29, 2024

djarecka commented Mar 7, 2024

tclose commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading