radical-cybertools / radical.pilot

RADICAL-Pilot
http://radical-cybertools.github.io/radical-pilot/index.html
Other
54 stars 23 forks source link

ensure that conda shell functions are exported #2961

Closed mtitov closed 1 year ago

codecov[bot] commented 1 year ago

Codecov Report

Merging #2961 (0f736cb) into devel (8f83ed8) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##            devel    #2961   +/-   ##
=======================================
  Coverage   41.45%   41.45%           
=======================================
  Files          95       95           
  Lines       10482    10482           
=======================================
  Hits         4345     4345           
  Misses       6137     6137           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mtitov commented 1 year ago

Tested on both Polaris and Summit (fixed the initial issue and doesn't break normal execution respectively)

@andre-merzky on Summit I see another error from env.log, which doesn't break the execution (attached corresponding env.log and bs0_pre_0.sh)

...
env/bs0_pre_0.sh: line 266: __add_sys_prefix_to_path: command not found
...

summit-env.log.txt.txt summit-env-bs0_pre_0.sh.txt

but we can fix it the same way - adding test "$(set | grep '__add_sys_prefix_to_path ()')" && export -f __add_sys_prefix_to_path - tested with this fix, and error message was eliminated - should I add this?

andre-merzky commented 1 year ago

but we can fix it the same way - adding test "$(set | grep '__add_sys_prefix_to_path ()')" && export -f __add_sys_prefix_to_path - tested with this fix, and error message was eliminated - should I add this?

If it is a non-fatal error I would rather opt not to fix it. It is a hack after all and fragile by nature, I think it's better to keep it as minimal as possible...

mtitov commented 1 year ago

but we can fix it the same way - adding test "$(set | grep '__add_sys_prefix_to_path ()')" && export -f __add_sys_prefix_to_path - tested with this fix, and error message was eliminated - should I add this?

If it is a non-fatal error I would rather opt not to fix it. It is a hack after all and fragile by nature, I think it's better to keep it as minimal as possible...

just for more context, this is what that function is on Summit

__add_sys_prefix_to_path () 
{ 
    if [ -n "${_CE_CONDA}" ] && [ -n "${WINDIR+x}" ]; then
        SYSP=$(\dirname "${CONDA_EXE}");
    else
        SYSP=$(\dirname "${CONDA_EXE}");
        SYSP=$(\dirname "${SYSP}");
    fi;
    if [ -n "${WINDIR+x}" ]; then
        PATH="${SYSP}/bin:${PATH}";
        PATH="${SYSP}/Scripts:${PATH}";
        PATH="${SYSP}/Library/bin:${PATH}";
        PATH="${SYSP}/Library/usr/bin:${PATH}";
        PATH="${SYSP}/Library/mingw-w64/bin:${PATH}";
        PATH="${SYSP}:${PATH}";
    else
        PATH="${SYSP}/bin:${PATH}";
    fi;
    \export PATH
}

Summit does add CONDA_EXE to the PATH, that's why it is not an issue there, and seems like Polaris export this function on its own (that's why also not an issue there). I worry that it might be a future issue if some system will not set conda "properly" and this __add_sys_prefix_to_path is conda-related function.