Check that distributed DAG is deadlock-free by matthiasdiener · Pull Request #366 · inducer/pytato

matthiasdiener · 2022-10-18T19:05:34Z

After merge:

Pytato+distributed: verify partition if in debug mode grudge#291

matthiasdiener · 2022-10-19T18:04:55Z

Is this going in the direction you had in mind @inducer?

inducer · 2022-10-19T18:22:35Z

Is this going in the direction you had in mind @inducer?

Yes, but unless I'm mistaken, you're currently still missing the edges relating to inter-rank communication. (And those are the interesting ones.)

kaushikcfd · 2022-10-20T03:17:20Z

I'm guessing the DAG being constructed here has nodes of the form (part_id, rank_nr) with edges of 3 types:

Inter-rank dependencies from the comm nodes matched by their tags
Intra-rank dependencies of the form P1->P2 iff P2 uses P1's output as an input.

matthiasdiener · 2022-11-01T00:35:37Z

+
+        print(all_summarized_parts)
+        output_to_defining_pid: Dict[_DistributedName, _DistributedPartId] = {}
+        for sumpart in all_summarized_parts.values():


For the keys in part.input_name_to_recv_node, there are no corresponding output_names, which is why this fails later on.

Ah, good catch. I believe that that's what we were missing. Do you want to push a fix?

I'm not sure what the right fix is - should we just ignore those input names in the later loop (around line 1023)?

Edit:
Done in 89dc0ef

kaushikcfd · 2022-11-01T21:01:16Z

Is the global parts DAG verifier here noticing cycles in any of the distributed tests? I ask because it seems some of the CI runs are getting timed out by running into a deadlock -- https://github.com/inducer/pytato/actions/runs/3365558563/jobs/5583742094. The CI run is from #374.

kaushikcfd · 2022-11-01T23:05:07Z

Looks like the CI in this PR is also experiencing the deadlock? I don't see any other reason why the tests would take this long.

matthiasdiener · 2022-11-01T23:11:05Z

Looks like the CI in this PR is also experiencing the deadlock? I don't see any other reason why the tests would take this long.

The tests (added in 05b4668) run fine on my local machine. My guess is that the MPI runtime inside the CI trips over the fact that one rank exited with an exception and just hangs.

kaushikcfd · 2022-11-01T23:12:11Z

The tests run fine on my local machine.

It seems to me moving to Py3.11 is triggering the error.

My guess is that the MPI runtime inside the CI trips over the fact that one rank exited with an exception and just hangs.

Could be the case. I didn't account for that.

kaushikcfd · 2022-11-03T23:31:20Z

Thanks! I ran deadlock_checker with this branch and the user experience was almost great. The driver reported the error -- ValueError: duplicate send for comm id: '_CommIdentifier(source_rank=1, dest_rank=0, comm_tag=43)' and deadlocked. The error was very helpful, but there were some non-ideal bits:

I don't think there is a duplicate send from rank 1.
The deadlocking bit is not great, it should gracefully terminate.

kaushikcfd · 2022-11-03T23:35:18Z

(I'll remove myself as a reviewer since you guys are more knowledgeable about this than me.)

matthiasdiener · 2022-11-04T00:08:49Z

Thanks! I ran deadlock_checker

Thanks, this is really helpful! There was a bug in _SummarizedDistributedSend which is hopefully fixed now.

with this branch and the user experience was almost great. The driver reported the error -- ValueError: duplicate send for comm id: '_CommIdentifier(source_rank=1, dest_rank=0, comm_tag=43)' and deadlocked. The error was very helpful, but there were some non-ideal bits:

I don't think there is a duplicate send from rank 1.

Right, with the fix there is a duplicate send from rank 0-> 1 now, which seems correct (the visualized DAG shows the same duplicate).

The deadlocking bit is not great, it should gracefully terminate.

Not sure what the best way to handle this is. It seems like it's MPI runtime specific.

inducer · 2022-11-09T18:37:00Z

I'm working on cleaning this up. In

https://github.com/inducer/pytato/compare/794e1c79d1995cdee842368188cc4afd4143cb25..a8a72dcbfa68be5edf58515945e8ba11cb066703

I've dropped all the unrelated stuff.

inducer · 2022-11-10T00:20:58Z

+        with pytest.raises(ValueError):
+            verify_distributed_partition(comm, distributed_parts)
+    else:
+        verify_distributed_partition(comm, distributed_parts)


These tests don't cover many of the things the verifier guards against. While I'm tempted to merge this so that we can start using it, I feel like it might be wise to wait for coverage of at least the following scenarios:

Duplicate send

Duplicate recv

Cycle in the graph

Thanks - I added these additional tests in 8d112fe

inducer · 2022-11-10T00:33:48Z

Sorry, had a few more force-pushes just now for a few final cleanups. Done now.

Co-authored-by: Matthias Diener <mdiener@illinois.edu>

inducer · 2022-11-14T02:01:32Z

Thanks!

And thanks for making these extra commits, that made the changes easy to review. I've folded them into the verifier and test commits, fixed some typos, and added a base class and eliminated what I believe was a redundant test. (The two spots that raised DuplicateSend made that easy to spot.)

matthiasdiener force-pushed the check-dist-deadlock branch 2 times, most recently from 92b53d1 to 0789c7d Compare October 18, 2022 21:02

matthiasdiener self-assigned this Oct 18, 2022

matthiasdiener force-pushed the check-dist-deadlock branch from 7780422 to 78e3c46 Compare October 19, 2022 17:59

matthiasdiener force-pushed the check-dist-deadlock branch from af88a87 to faa623e Compare October 19, 2022 21:35

MTCam mentioned this pull request Oct 20, 2022

Add missing comm tag forwarding illinois-ceesd/mirgecom#784

Merged

8 tasks

inducer reviewed Oct 20, 2022

View reviewed changes

Comment thread pytato/distributed.py Outdated

matthiasdiener force-pushed the check-dist-deadlock branch 4 times, most recently from dd6d79d to 3af81dd Compare October 31, 2022 21:59

kaushikcfd reviewed Nov 1, 2022

View reviewed changes

Comment thread pytato/distributed.py Outdated

inducer force-pushed the check-dist-deadlock branch from 9d47108 to 446daf5 Compare November 1, 2022 00:13

matthiasdiener commented Nov 1, 2022

View reviewed changes

matthiasdiener force-pushed the check-dist-deadlock branch from 3c5339e to 5900a0c Compare November 3, 2022 22:31

matthiasdiener marked this pull request as ready for review November 3, 2022 22:44

matthiasdiener requested review from inducer and kaushikcfd November 3, 2022 22:44

kaushikcfd removed their request for review November 3, 2022 23:35

matthiasdiener force-pushed the check-dist-deadlock branch from 66b9dc2 to f02249c Compare November 4, 2022 00:28

matthiasdiener force-pushed the check-dist-deadlock branch from afc6d24 to 7210e6f Compare November 4, 2022 22:30

matthiasdiener mentioned this pull request Nov 7, 2022

Minor distributed visualization improvements #385

Merged

inducer force-pushed the check-dist-deadlock branch 3 times, most recently from 794e1c7 to a8a72dc Compare November 9, 2022 18:36

inducer force-pushed the check-dist-deadlock branch 4 times, most recently from e114fe1 to 222611f Compare November 9, 2022 19:17

inducer reviewed Nov 10, 2022

View reviewed changes

inducer force-pushed the check-dist-deadlock branch from 222611f to 46992cd Compare November 10, 2022 00:28

matthiasdiener and others added 7 commits November 9, 2022 18:29

add mpi4py intersphinx

ff34b85

Remove stray distributed docs from pytato.partition

a4c2532

Use MPI4py types in type annotation in pt.distributed

6361bfd

Add some assertions in number_distributed_tags and _post_receive

9e424bc

Fix typo: _map_distributed_graph_partition_nodes

94f7a31

Fix sectioning, improve naming in pt.distributed

3a49b5b

test_distributed: avoid separate distributed import

d8fd70a

inducer force-pushed the check-dist-deadlock branch from 46992cd to 994d1b2 Compare November 10, 2022 00:32

inducer mentioned this pull request Nov 10, 2022

Pytato+distributed: verify partition if in debug mode inducer/grudge#291

Merged

1 task

inducer and others added 3 commits November 13, 2022 19:59

Add verify_distributed_partition

87b4d20

Co-authored-by: Matthias Diener <mdiener@illinois.edu>

Test verify_distributed_partition

5524762

Verify DAG partition in random-DAG distributed test

1a7e391

inducer force-pushed the check-dist-deadlock branch from 4fda54a to 1a7e391 Compare November 14, 2022 02:00

inducer enabled auto-merge (rebase) November 14, 2022 02:01

inducer merged commit 946b99e into main Nov 14, 2022

inducer deleted the check-dist-deadlock branch November 14, 2022 02:46

Conversation

matthiasdiener commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthiasdiener commented Oct 19, 2022

Uh oh!

inducer commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaushikcfd commented Oct 20, 2022

Uh oh!

Uh oh!

Uh oh!

matthiasdiener Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

inducer Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

matthiasdiener Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaushikcfd commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaushikcfd commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthiasdiener commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaushikcfd commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaushikcfd commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaushikcfd commented Nov 3, 2022

Uh oh!

matthiasdiener commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inducer commented Nov 9, 2022

Uh oh!

inducer Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

matthiasdiener Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

inducer commented Nov 10, 2022

Uh oh!

inducer commented Nov 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthiasdiener commented Oct 18, 2022 •

edited

Loading

inducer commented Oct 19, 2022 •

edited

Loading

matthiasdiener Nov 1, 2022 •

edited

Loading

kaushikcfd commented Nov 1, 2022 •

edited

Loading

kaushikcfd commented Nov 1, 2022 •

edited

Loading

matthiasdiener commented Nov 1, 2022 •

edited

Loading

kaushikcfd commented Nov 1, 2022 •

edited

Loading

kaushikcfd commented Nov 3, 2022 •

edited

Loading

matthiasdiener commented Nov 4, 2022 •

edited

Loading