Recast partition_mesh in terms of generic part identifiers

[majosm]

Should remove the need for a custom partition boundary tag in grudge.

Targets #307.

[majosm]

@inducer Is there a way I can get the CI to run on this? (Not sure why it isn't; maybe because the target branch isn't main?)

[inducer]

Looks like it was just taking a while...?

[matthiasdiener]

@inducer Is there a way I can get the CI to run on this? (Not sure why it isn't; maybe because the target branch isn't main?)

Github is having some issues today: https://www.githubstatus.com/

[majosm]

I think this is ready for a look, apart from the doc failures (which I'm not sure how to fix).

[alexfikl]

I think this is ready for a look, apart from the doc failures (which I'm not sure how to fix).

You need to add mpi4py to intersphinx in docs/conf.py for that to work. Probably something like

"https://mpi4py.readthedocs.io/en/stable": None

[alexfikl]

I'm not familiar with the MPI parts of meshmode, but was looking a bit through this and left some nitpicks!

[inducer]

Sorry about the conflicts! Turns out I had some uncommitted work lying around that was needed to get inducer/grudge#236 to go.

[inducer]

One question I have about this is to what extent this is at odds with the current approach in inducer/grudge#236, specifically when you say

Should remove the need for a custom partition boundary tag in grudge.

I feel like this is headed for BTAG_PARTITION(BTAG_MULTIVOL_PARTITION(...)), which seems not that convenient, even if the inner thing were renamed. Can you explain a bit what your thought would be on how this would look in grudge?

[majosm]

I feel like this is headed for BTAG_PARTITION(BTAG_MULTIVOL_PARTITION(...)), which seems not that convenient, even if the inner thing were renamed. Can you explain a bit what your thought would be on how this would look in grudge?

I was picturing that part_id in the multivolume case would just be a tuple (vol, rank). Then BTAG_MULTIVOL_PARTITION(...) would be replaced by BTAG_PARTITION((vol, rank)). Is that not going to work? I haven't looked through the grudge PR closely yet.

[inducer]

I was picturing that part_id in the multivolume case would just be a tuple (vol, rank).

The issue I see is that, on many occasions, grudge (at least as currently done in inducer/grudge#236, though I don't see how to avoid it) needs to pick apart the boundary tag to understand its components (to see which ranks/volumes are being connected), so making the parition IDs entirely opaque won't do.

[majosm]

The issue I see is that, on many occasions, grudge (at least as currently done in inducer/grudge#236, though I don't see how to avoid it) needs to pick apart the boundary tag to understand its components (to see which ranks/volumes are being connected), so making the parition IDs entirely opaque won't do.

Right, that sounds fine to me. The idea here was to make it opaque to meshmode (and thus avoid the need to postprocess the partitioned mesh in order to relabel the tags). The grudge code can still use the partition identifier data however it wants.

[inducer]

stamp out remaining traces of fun

That's no fun! :P

[inducer]

and thus avoid the need to postprocess the partitioned mesh in order to relabel the tags

That's sort of my question though. Unless we make grudge live with BTAG_PARTITION(something), there's still a relabeling needed.

[majosm]

Unless we make grudge live with BTAG_PARTITION(something),

That's kind of what I was thinking. I assume we will need a function in grudge that does the mesh partitioning anyway (using partition_mesh and mpi_distribute under the hood), so I figure at that time it can insert whatever data it needs into the partition ID and pull it out as needed later. All the uses of <instance of BTAG_MULTIVOL_PARTITION>.<whatever> would become <instance of BTAG_PARTITION>.part_id.<whatever>.

[inducer]

Hmm. That's what I figured. The one fly in that ointment is that, currently, BTAG_MULTIVOL_PARTITION doesn't just identify the remote partition (as would be the case with your proposed approach), but also the local one (it has self_volume_tag). Representation-wise, that is sort of redundant, so I sort of see your point. Let me see how hard it is to get rid of self_volume_tag in inducer/grudge#236.

[majosm]

Hmm. That's what I figured. The one fly in that ointment is that, currently, BTAG_MULTIVOL_PARTITION doesn't just identify the remote partition (as would be the case with your proposed approach), but also the local one (it has self_volume_tag). Representation-wise, that is sort of redundant, so I sort of see your point. Let me see how hard it is to get rid of self_volume_tag in inducer/grudge#236.

Would it possibly help if the facial adjacency groups had volumes associated with them? We'll need to read in the volume tags in read_gmsh, and at least the original plan was to store them inside the mesh (something like #292). We could potentially split the facial adjacency by volume tag the same way we do with boundary tags.

[inducer]

Would it possibly help if the facial adjacency groups had volumes associated with them? We'll need to read in the volume tags in read_gmsh, and at least the original plan was to store them inside the mesh (something like #292). We could potentially split the facial adjacency by volume tag the same way we do with boundary tags.

That might be convenient for easier partitioning, but for the above problem, that's neither here nor there IMO. That's because by the time we get to grudge, each different volume is a separate Discretization, and therefore a separate Mesh.

[inducer]

Let me see how hard it is to get rid of self_volume_tag in inducer/grudge#236.

The initial sense I'm getting is that this is a good direction. I should have that pushed in a bit.

[inducer]

Urgh. So, in order to support the changes in inducer/grudge#236 that we discussed (which I still like), I ended up making some changes to #307 that I think duplicate some of what you did here. IOW, I made another mess of conflicts. Sorry!

[inducer]

Ready to look at? Or would you like me to wait? (I ask because it's not draft, but there also isn't an explicit review request.)

[inducer]

Grrr, again. Sorry. :)

[majosm]

If the CI passes, this should be ready for review now.

Couple of notes:

• There are some FIXMEs relating to relabeling variables and moving code around; I didn't want to clutter this PR up with that stuff, so I'll probably do a follow-up later (maybe after Allow distributed partition boundaries to be not just rank boundaries #307 goes in?) to deal with that.
• I think this is squashable now that the mypy stuff is out.

[inducer]

Thanks! Just a few minor things, then this is good to go. (Feel free to squash+merge when done.)