Skip to content

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

Merged
inducer merged 2 commits into
mainfrom
verify-partition
Nov 14, 2022
Merged

Pytato+distributed: verify partition if in debug mode#291
inducer merged 2 commits into
mainfrom
verify-partition

Conversation

@inducer

@inducer inducer commented Nov 10, 2022

Copy link
Copy Markdown
Owner

@kaushikcfd

Copy link
Copy Markdown
Collaborator

Why don't we check for deadlocks whenever the user calls execute_distribute_parttion for the first time?

@inducer

inducer commented Nov 10, 2022

Copy link
Copy Markdown
Owner Author

Why don't we check for deadlocks whenever the user calls execute_distribute_parttion for the first time?

How do we tell that it's the first time? By setting a flag on the partition?

I'm also a bit wary of doing this implicitly at the pytato level, since it's fairly expensive.

@kaushikcfd

Copy link
Copy Markdown
Collaborator

I was thinking something on the lines of:

@memoize_on_first_arg
def _verify_no_cycles(parts, comm):
    ...

def execute_distribute_partiions(parts, comm):
    if __debug__:
        _verify_no_cyles(parts, comm)

    ...

But the problem here is comm isn't hashable. Not sure if that's avoidable.

@inducer

inducer commented Nov 10, 2022

Copy link
Copy Markdown
Owner Author

That's no different than setting a flag! (just with a more awkward interface)

@kaushikcfd

Copy link
Copy Markdown
Collaborator

That's no different than setting a flag! (just with a more awkward interface)

Ah fair.

@inducer
inducer marked this pull request as ready for review November 14, 2022 20:34
@inducer
inducer merged commit 8df43f4 into main Nov 14, 2022
@inducer
inducer deleted the verify-partition branch November 14, 2022 20:37
Comment thread grudge/array_context.py
# '_BasePytatoArrayContext' has no 'mpi_communicator' member
pt.verify_distributed_partition(
self.actx.mpi_communicator, # pylint: disable=no-member
distributed_partition)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this guaranteed to terminate cleanly or is there a chance we might need a self.actx.mpi_communicator.abort() if a failure was seen?

@inducer inducer Nov 14, 2022

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this might better be addressed by using python -m mpi4py in the MPI command line for these runs. This will automatically abort/kill other ranks if one goes down with an exception.

See, e.g. #295, inducer/pytato#389

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yep it does get resolved if we do python -m mpi4py .... If that's the only way one is supposed to run grudge apps, feel free to close this.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the only way, but it's a good way to avoid hangs. Realistically, it's something every user of mpi4py-dependent code should know. Mirgecom does some hand-holding along these lines:

https://github.com/illinois-ceesd/mirgecom/blob/0d8635eb16717d62d83a0e62ddcdb0fb7866c4d7/mirgecom/mpi.py#L109-L159

But I find that too heavy-handed for pytato or grudge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants