Add kaHIP k-way partitioning #231

mgsium · 2022-07-14T22:17:48Z

No description provided.

…into kahip_support

reguly · 2022-07-15T20:37:36Z

It looks like currently if you have both parmetis and kahip, and as for kway, kahip will be used, and there is no way to get parmetis. Please allow the use of both. In the call to op_partition, you can specify both combinations, so it's just a matter of propagating this information into op_partition_kway

reguly

I have some compilation issues when I have Parmetis and kahip. Goes through OK if I only have KaHIP.

reguly · 2022-07-19T16:40:34Z

makefiles/dependencies/kahip.mk

+KAHIP_DEF ?= -DHAVE_KAHIP
+
+ifdef KAHIP_INSTALL_PATH
+  KAHIP_INC_PATH := -I$(KAHIP_INSTALL_PATH)/parallel/parallel_src/interface \


This should just be -I$(KAHIP_INSTALL_PATH)/include
if the kahip library is properly "installed" i.e. during compilation you have -DCMAKE_INSTALL_PREFIX=somedir then during OP2 make config you have KAHIP_INSTALL_PATH=somedir

reguly · 2022-07-19T16:40:44Z

makefiles/dependencies/kahip.mk

+ifdef KAHIP_INSTALL_PATH
+  KAHIP_INC_PATH := -I$(KAHIP_INSTALL_PATH)/parallel/parallel_src/interface \
+                    -I$(KAHIP_INSTALL_PATH)/interface
+  KAHIP_LIB_PATH := -L$(KAHIP_INSTALL_PATH)/build/parallel/parallel_src \


same with LIB_PATH - should be -L$(KAHIP_INSTALL_PATH)/lib

reguly · 2022-07-19T16:44:01Z

op2/src/mpi/op_mpi_part_core.cpp

+#ifdef HAVE_KAHIP
+#include <parhip_interface.h>
+
+typedef idxtype idx_t;


I am getting a compile time error with gcc 9 if I also have ParMetis installed:

In file included from src/mpi/op_mpi_part_core.cpp:73:
/home/shared/software/kahip-ompi401/include/parhip_interface.h:15:28: error: conflicting declaration ‘typedef long long unsigned int idxtype’
15 | typedef unsigned long long idxtype;
| ^~~~~~~
src/mpi/op_mpi_part_core.cpp:65:15: note: previous declaration as ‘typedef idx_t idxtype’
65 | typedef idx_t idxtype;

I ran into the same issue yesterday, looks like ParMETIS defines idx_t while ParHIP defines idxtype, which is distinct. As long as only one of the two are installed, we can use typedef to set the type as necessary but with both installed we're forced to choose between the types at runtime.

The interface for the two k-way partition functions are very similar apart from this type issue so I'm reluctant to define another op_partition_kway just for the ParHIP version. But I've yet to find an elegant solution.

That's a tough one... My first idea, while being wasteful, but at least compact, would be to pass the parmetis-style arrays to a function, and allocate new ones with kahip types, convert them, then call kahip. Which is indeed ugly! But the fundamental issue is that even if both use 64 bits, kahip uses unsigned, whereas parmetis signed...

Could we define a generic op_partition_kway_generic outside the scope of extern "C", then call this from op_partition_kway. Afaik this remains C-compatible if we just expose the signature of op_partition_kway with extern "C", and keep the body outside?

reguly · 2022-07-20T12:04:36Z

Yes, that is a good idea! this is already a cpp file, and we don’t really need extern “C” for op_partition_kway, just op_partition itself. So you can go ahead and template it!

…

On 2022. Jul 20., at 13:50, mgsium ***@***.***> wrote: @mgsium commented on this pull request. In op2/src/mpi/op_mpi_part_core.cpp <#231 (comment)>: > @@ -68,6 +68,14 @@ typedef int idx_t; #endif #endif +// kaHIP header +#ifdef HAVE_KAHIP +#include <parhip_interface.h> + +typedef idxtype idx_t; Could we define a generic op_partition_kway_generic outside the scope of extern "C", then call this from op_partition_kway. Afaik this remains C-compatible if we just expose the signature of op_partition_kway with extern "C", and keep the body outside? — Reply to this email directly, view it on GitHub <#231 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJWVVKFZBKXAYKWLETVFBDVU7RW3ANCNFSM53TWZXLQ>. You are receiving this because you commented.

mgsium · 2022-07-20T17:33:31Z

I think some overloading is still necessary because the two partition functions don't take the same arguments. The best I could think of was:

/*******************************************************************************
 * Generic wrapper for kway-partition functions
 *******************************************************************************/

template <class T>
void do_partition(T *vtxdist, T *xadj, T *adjncy, T *wgtflag, T *numflag, 
            T *ncon, T *nparts,real_t *tpwgts, real_t *ubvec, T *options, 
            T *edgecut, T *part, MPI_Comm *comm) {}

#ifdef HAVE_PARMETIS
template<> void do_partition<idx_t>(idx_t *vtxdist, idx_t *xadj, idx_t *adjncy, 
            idx_t *wgtflag, idx_t *numflag, idx_t *ncon, idx_t *nparts,real_t *tpwgts, 
            real_t *ubvec, idx_t *options, idx_t *edgecut, idx_t *part, MPI_Comm *comm) {
  ParMETIS_V3_PartKway( vtxdist, xadj, adjncy, NULL, NULL, wgtflag, numflag, ncon, 
                        nparts, tpwgts, ubvec, options, edgecut, part, comm);
}
#endif

#ifdef HAVE_KAHIP
template<> void do_partition<idxtype>(idxtype *vtxdist, idxtype *xadj, idxtype *adjncy, 
            idxtype *wgtflag, idxtype *numflag, idxtype *ncon, idxtype *nparts, real_t *tpwgts, 
            real_t *ubvec, idxtype *options, idxtype *edgecut, idxtype *part, MPI_Comm *comm) {
  double imb = 0.03;
  ParHIPPartitionKWay( vtxdist, xadj, adjncy, NULL, NULL, (int*) nparts, &imb, 
                       false, 1, ULTRAFASTMESH, (int*) edgecut, part, comm);
}
#endif

The long argument list is a little ugly (although it can be made a little better by omitting the names of unused arguments).
Not an ideal solution but it feels less cumbersome than overloading op_partition_kway.

reguly · 2022-07-20T18:46:38Z

Could you not template the whole op_partition_kway function? i.e. template <bool use_kahip, typename indextype> void op_partition_kway() { then use indextype throughout }

…

On 2022. Jul 20., at 19:33, mgsium ***@***.***> wrote: I think some overloading is still necessary because the two partition functions don't take the same arguments. The best I could think of was: /******************************************************************************* * Generic wrapper for kway-partition functions *******************************************************************************/ template <class T> void do_partition(T *vtxdist, T *xadj, T *adjncy, T *wgtflag, T *numflag, T *ncon, T *nparts,real_t *tpwgts, real_t *ubvec, T *options, T *edgecut, T *part, MPI_Comm *comm) {} #ifdef HAVE_PARMETIS template<> void do_partition<idx_t>(idx_t *vtxdist, idx_t *xadj, idx_t *adjncy, idx_t *wgtflag, idx_t *numflag, idx_t *ncon, idx_t *nparts,real_t *tpwgts, real_t *ubvec, idx_t *options, idx_t *edgecut, idx_t *part, MPI_Comm *comm) { ParMETIS_V3_PartKway( vtxdist, xadj, adjncy, NULL, NULL, wgtflag, numflag, ncon, nparts, tpwgts, ubvec, options, edgecut, part, comm); } #endif #ifdef HAVE_KAHIP template<> void do_partition<idxtype>(idxtype *vtxdist, idxtype *xadj, idxtype *adjncy, idxtype *wgtflag, idxtype *numflag, idxtype *ncon, idxtype *nparts, real_t *tpwgts, real_t *ubvec, idxtype *options, idxtype *edgecut, idxtype *part, MPI_Comm *comm) { double imb = 0.03; ParHIPPartitionKWay( vtxdist, xadj, adjncy, NULL, NULL, (int*) nparts, &imb, false, 1, ULTRAFASTMESH, (int*) edgecut, part, comm); } #endif The long argument list is a little ugly (although it can be made a little better by omitting the names of unused arguments). Not an ideal solution but it feels less cumbersome than overloading op_partition_kway. — Reply to this email directly, view it on GitHub <#231 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJWVVI2IWZ775QWPUUKAWLVVAZ7NANCNFSM53TWZXLQ>. You are receiving this because you commented.

mgsium · 2022-07-20T19:00:52Z

I'm doing that too, but there's still the problem of choosing whether to call the kahip or parmetis function at runtime.

mgsium · 2022-07-21T18:20:14Z

On second thoughts, do_partition doesn't need to be templated. This should suffice:

#ifdef HAVE_PARMETIS
void do_partition(idx_t *vtxdist, idx_t *xadj, ...) {
  ParMETIS_V3_PartKway( vtxdist, xadj, adjncy, NULL, NULL, wgtflag, numflag, ncon, 
                        nparts, tpwgts, ubvec, options, edgecut, part, comm);
}
#endif

#ifdef HAVE_KAHIP
void do_partition(idxtype *vtxdist, idxtype *xadj, ...) {
  double imb = 0.03;
  ParHIPPartitionKWay( vtxdist, xadj, adjncy, NULL, NULL, (int*) nparts, &imb, 
                       false, 1, ULTRAFASTMESH, (int*) edgecut, part, comm);
}
#endif

reguly · 2022-07-21T18:37:37Z

let's see :)

reguly

Compiles and works!

mgsium · 2022-07-21T19:35:43Z

Great! I'll just change the include and library paths and that should be it

gihanmudalige · 2022-07-21T20:44:53Z

@reguly thanks for following up on this. Seems @mgsium has completed the work in record time !

@mgsium I assume this is now working with the Makefile system in the master branch ?
If we are to merge this I think it would be good to add some documentation on the new capabilities. For example adding details to : https://op2-dsl.readthedocs.io/en/latest/api.html?highlight=partition#c.op_partition
Furthermore if there is a need to have any runtime flags to activate this partiotioner capabilities, that needs to be documented as well. Finally have you checked the coding standards and run it through git clang-format ?

Thanks.

bozbez · 2022-07-21T21:55:48Z

The Makefiles look good to me.
Once this is merged it would also be worth updating the spack recipe.

mgsium · 2022-07-22T15:32:38Z

@gihanmudalige I updated the docs and ran clang-format on the changes. I've been using the Makefile to build and test so all should be good there.

Musab Guma'a and others added 4 commits July 13, 2022 00:25

Add support for k-way partitioning with KaHIP

4c53324

Merge branch 'OP-DSL:master' into kahip_support

e4a48ed

Implement kahip link test

e0eb5c9

Merge branch 'kahip_support' of https://github.com/mgsium/OP2-Common …

c26a188

…into kahip_support

Musab Guma'a added 2 commits July 15, 2022 21:54

propogate kahip/parmetis choice to op_partition_kway

d090a6c

fix comment

ebb931b

reguly requested changes Jul 19, 2022

View reviewed changes

allow parmetis/kahip choice at runtime

23e4fd2

reguly approved these changes Jul 21, 2022

View reviewed changes

fix include and library paths

34667f1

Musab Guma'a added 2 commits July 22, 2022 10:42

run clang-format

a7099d4

update docs

f7433ba

mgsium marked this pull request as ready for review July 22, 2022 12:06

gihanmudalige merged commit fba5c05 into OP-DSL:master Jul 26, 2022

mgsium mentioned this pull request Jul 28, 2022

Add KaHIP support warwick-hpsc/MG-CFD-app-OP2#43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kaHIP k-way partitioning #231

Add kaHIP k-way partitioning #231

mgsium commented Jul 14, 2022

reguly commented Jul 15, 2022

reguly left a comment

reguly Jul 19, 2022

reguly Jul 19, 2022 •

edited

Loading

reguly Jul 19, 2022

mgsium Jul 19, 2022

reguly Jul 19, 2022

mgsium Jul 20, 2022

reguly commented Jul 20, 2022 via email

mgsium commented Jul 20, 2022

reguly commented Jul 20, 2022 via email

mgsium commented Jul 20, 2022

mgsium commented Jul 21, 2022

reguly commented Jul 21, 2022

reguly left a comment

mgsium commented Jul 21, 2022

gihanmudalige commented Jul 21, 2022

bozbez commented Jul 21, 2022

mgsium commented Jul 22, 2022

Add kaHIP k-way partitioning #231

Add kaHIP k-way partitioning #231

Conversation

mgsium commented Jul 14, 2022

reguly commented Jul 15, 2022

reguly left a comment

Choose a reason for hiding this comment

reguly Jul 19, 2022

Choose a reason for hiding this comment

reguly Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

reguly Jul 19, 2022

Choose a reason for hiding this comment

mgsium Jul 19, 2022

Choose a reason for hiding this comment

reguly Jul 19, 2022

Choose a reason for hiding this comment

mgsium Jul 20, 2022

Choose a reason for hiding this comment

reguly commented Jul 20, 2022 via email

mgsium commented Jul 20, 2022

reguly commented Jul 20, 2022 via email

mgsium commented Jul 20, 2022

mgsium commented Jul 21, 2022

reguly commented Jul 21, 2022

reguly left a comment

Choose a reason for hiding this comment

mgsium commented Jul 21, 2022

gihanmudalige commented Jul 21, 2022

bozbez commented Jul 21, 2022

mgsium commented Jul 22, 2022

reguly Jul 19, 2022 •

edited

Loading