Skip to content

Boundary lazy comm v2 - #183

Merged
inducer merged 72 commits into
mainfrom
boundary_lazy_comm_v2
Mar 2, 2022
Merged

Boundary lazy comm v2#183
inducer merged 72 commits into
mainfrom
boundary_lazy_comm_v2

Conversation

@matthiasdiener

@matthiasdiener matthiasdiener commented Nov 2, 2021

Copy link
Copy Markdown
Collaborator

@matthiasdiener matthiasdiener self-assigned this Nov 2, 2021
@matthiasdiener matthiasdiener mentioned this pull request Nov 2, 2021
2 tasks
@matthiasdiener

Copy link
Copy Markdown
Collaborator Author

As far as I can see, this does successfully do the code generation for the partitioned code now, but fails in the "normal" pytato code generation now since DistributedSendRefHolder etc. is still part of the DAG, which the CodegenMapper can not handle. I'm not sure what the right approach here is @inducer - should the partitioning remain part of transform_dag(), or should it go somewhere else?

@inducer

inducer commented Nov 17, 2021

Copy link
Copy Markdown
Owner

The RefHolder should have been removed by partitioning. I'm not sure why it's still around.

@matthiasdiener

Copy link
Copy Markdown
Collaborator Author

The RefHolder should have been removed by partitioning. I'm not sure why it's still around.

Sorry, I wasn't very clear.
The RefHolder is removed correctly from what I can tell. However, the partitioned code isn't saved anywhere at the moment, so the subsequent execution still uses the unpartitioned code and fails. I was wondering whether transform_dag() is the right place to do the partitioning, and where the new execution behavior should be implemented (in LazilyCompilingFunctionCaller::__call__ ?)

@inducer

inducer commented Nov 17, 2021

Copy link
Copy Markdown
Owner

Sure, the execution logic needs to change, and I agree that transform_dag is not the correct place for the partitioning. We need an array context subclass that knows about distributed execution, and that needs to worry about distributed execution in the appropriate places. (freeze and precompiled function execution as you mentioned)

@matthiasdiener

Copy link
Copy Markdown
Collaborator Author

I think this is ready for another review @inducer.

@inducer
inducer force-pushed the boundary_lazy_comm_v2 branch from 38a82f3 to ed3a4fb Compare March 1, 2022 00:09
@inducer

inducer commented Mar 2, 2022

Copy link
Copy Markdown
Owner

FYI I introduced an MPIPyOpenCLArrayContext. This was needed to avoid just ignoring symbolic tags in the eager case, which didn't sit well with me. This in turn shifted the "source of truth" for the MPI communicator from the discretization to the array context, which is just as well IMO.

@inducer
inducer force-pushed the boundary_lazy_comm_v2 branch from 34e73e8 to 7750e11 Compare March 2, 2022 00:44
@inducer
inducer enabled auto-merge (squash) March 2, 2022 00:54
@inducer
inducer merged commit 44760a5 into main Mar 2, 2022
@inducer
inducer deleted the boundary_lazy_comm_v2 branch March 2, 2022 02:05
@inducer
inducer restored the boundary_lazy_comm_v2 branch March 2, 2022 03:05
@matthiasdiener
matthiasdiener deleted the boundary_lazy_comm_v2 branch March 2, 2022 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mirgecom production Developments critical to MIRGECom

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants