search

flux-framework / flux-core-v0.11

flux-core v0.11 stable branch
Other
2 stars 6 forks source link

spectrum.lua: fix for current Spectrum MPI #6

Closed grondo closed 5 years ago

grondo commented 5 years ago

As described in #4, update the spectrum.lua plugin to support latest version of Spectrum MPI based on @SteVwonder's debugging effort.

Also had to add support for a wreck.environ:get() method to return a copy of the current environment as a Lua table in the wreck/lua plugins. This was the quickest way to allow the current job environment to be iterated.

grondo commented 5 years ago

Rebased on current master

grondo commented 5 years ago

Sorry forgot to remove an existing test for spectrum.lua setting MPI_ROOT. Fix coming.

codecov-io commented 5 years ago

Codecov Report

Merging #6 into master will decrease coverage by <.01%. The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
- Coverage   80.15%   80.15%   -0.01%     
==========================================
  Files         196      196              
  Lines       35084    35103      +19     
==========================================
+ Hits        28123    28137      +14     
- Misses       6961     6966       +5
Impacted Files Coverage Δ
src/modules/wreck/wrexecd.c 76.92% <90.47%> (+0.18%) :arrow_up:
src/common/libflux/response.c 81.48% <0%> (-1.24%) :arrow_down:
src/modules/connector-local/local.c 73.77% <0%> (-1.19%) :arrow_down:
src/bindings/lua/lua-hostlist/hostlist.c 58.78% <0%> (-0.23%) :arrow_down:
src/common/libflux/message.c 81.39% <0%> (-0.13%) :arrow_down:
src/modules/kvs/kvs.c 66.71% <0%> (+0.14%) :arrow_up:
src/modules/wreck/job.c 69.64% <0%> (+0.23%) :arrow_up:
src/modules/wreck/luastack.c 45.1% <0%> (+3.8%) :arrow_up:
garlick commented 5 years ago

Looks fine to me. Should the OPAL_ environment variables be cleared too?

grondo commented 5 years ago

Should the OPAL_ environment variables be cleared too?

I'm not sure, but @SteVwonder only specified OMPI_* and PMIX_*. Easy enough to add if you think it is necessary.

SteVwonder commented 5 years ago

Thanks @grondo for putting this together.

Should the OPAL_ environment variables be cleared too?

I am not sure. All I really know is that it wasn't necessary in my limited testing. I don't know openmpi/spectrum mpi and their associated env variables, so I will defer to others on the right choice here.

EDIT: @trws might know

grondo commented 5 years ago

Ok, I've pushed the change to only prepend paths that don't already exist in the target env var.

The only thing left here is to decide if we need to remove the OPAL_* env vars?

trws commented 5 years ago

I was removing them in testing I think, but one of them is required for things to work right, something like OPAL_*_PATH I think?

On May 1, 2019, at 2:22 PM, Mark Grondona notifications@github.com<mailto:notifications@github.com> wrote:

Ok, I've pushed the change to only prepend paths that don't already exist in the target env var.

The only thing left here is to decide if we need to remove the OPAL_* env vars?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/flux-framework/flux-core-v0.11/pull/6#issuecomment-488367007, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAFBFNKFKB55QRP7FQ5AZKDPTHNWBANCNFSM4HJOSZ2Q.

SteVwonder commented 5 years ago

LGTM! Thanks @grondo for putting this together.

Merging since we confirmed it works on the current software stack of lassen, as-is. We can update in the future if need be.