ompi/dpm: set arch to spawn procs in heterogeneous mode

[ggouaillardet]

Thanks Siegmar Gross for reporting this issue.

[rhc54]

@ggouaillardet Looks good to me - thanks for tracking it down!

👍

[ggouaillardet]

i am still wondering whether this is the correct fix ...

in a non dynamic program, proc_arch is set in ompi_proc_complete_init_single which is invoked by ompi_proc_complete_init in ompi_mpi_init
among other things, ompi_proc_complete_init_single sets the proc_arch

i made the very mimimal changes in ompi_dpm_connect_accept in order to fix the issue, but i am wondering if that is really enough. for example, do we still miss something and should we invoke ompi_proc_complete_init_single instead ?

[hppritcha]

@rhc54 could you double check whether this is the right way to fix this.

[rhc54]

@ggouaillardet I don't think we can call ompi_proc_complete_init_single as it's trying to receive the locality instead of just setting it. However, that is the only point of departure between the two places. So I would suggest considering adding a flag to the function to indicate whether the locality should be recovered or set, and then reuse the code so we avoid issues in the future.

Make sense?

[ggouaillardet]

@rhc54 i made the changes to ompi_proc_complete_init_single

can you especially double check ompi/dpm/dpm.c ?
i #ifdef'ed out some code since i have no idea whether i should do something with the locality or not

[rhc54]

@ggouaillardet Sorry for delay - this just fell off my radar. Your changes to ompi/dpm involving the locality are incorrect - we must flag the dynamically connected procs as "non-local" so the shared memory components don't attempt to talk to them as we don't support that mode. Other than that, this looks fine.

[ggouaillardet]

Is "we" btl/sm and btl/vader ?
If yes, should dpm (ompi) let the btl (opal) figure out what to do if they do not support that ?

[rhc54]

Yes - neither of the shared memory components support communication across jobs. You are welcome to propose a change to their logic so they don't use "non-local" as their flag, though I'm not sure they have any plans to support cross-job shared memory.

[ggouaillardet]

@rhc54

from mca_btl_sm_add_procs

    for (proc = 0; proc < (int32_t)nprocs; proc++) {
        /* check to see if this proc can be reached via shmem (i.e.,
           if they're on my local host and in my job) */
        if (procs[proc]->proc_name.jobid != my_proc->proc_name.jobid ||
            !OPAL_PROC_ON_LOCAL_NODE(procs[proc]->proc_flags)) {
            peers[proc] = NULL;
            continue;
        }

and from vader_add_procs

    for (int32_t proc = 0, local_rank = 0 ; proc < (int32_t) nprocs ; ++proc) {
        /* check to see if this proc can be reached via shmem (i.e.,
           if they're on my local host and in my job) */
        if (procs[proc]->proc_name.jobid != my_proc->proc_name.jobid ||
            !OPAL_PROC_ON_LOCAL_NODE(procs[proc]->proc_flags)) {
            peers[proc] = NULL;
            continue;
        }

neither sm nor vader can be used between two different jobs, and regardless their proc_flags

did i miss something ?

if not, i will simplify the commit, and optionally retrieve locality since that cannot hurt.

[rhc54]

lol - ok, they already addressed this issue. Sure, go ahead!

[ggouaillardet]

this PR is now replaced by #1385