Skip to content

Commit

Permalink
Remove preset environment variables from slurmSettings and mpirunSett…
Browse files Browse the repository at this point in the history
…ings (#181)

Remove preset environment variables from slurmSettings and mpirunSettings since they are no longer needed.

Some other clean-ups:

- Check env var value types earlier in the process to ensure types are correct before launching any jobs
- Added a note to the `update_env` docs to inform users on how to have smartsim inherit the user environment
- Only throw `--export` flag if env_vars is not empty, otherwise we get an argument parsing error

Verified full testing passes on an HPC system with `SMARTSIM_TEST_LAUNCHER=slurm`.

Resolves #128

[ Commited by @ben-albrecht ]
[ Reviewed by @Spartee && @al-rigazzi ]
  • Loading branch information
ben-albrecht authored Apr 5, 2022
1 parent 6299c30 commit 8b5ba19
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 31 deletions.
7 changes: 5 additions & 2 deletions smartsim/_core/launcher/step/slurmStep.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,9 @@ def get_launch_cmd(self):
env_var_str,
comma_separated_env_vars,
) = self.run_settings.format_comma_sep_env_vars()
srun_cmd += ["--export", env_var_str]

if len(env_var_str) > 0:
srun_cmd += ["--export", env_var_str]

if comma_separated_env_vars:
srun_cmd = ["env"] + comma_separated_env_vars + srun_cmd
Expand Down Expand Up @@ -199,7 +201,8 @@ def _make_mpmd(self):
cmd += mpmd.format_run_args()
cmd += ["--job-name", self.name]
(env_var_str, _) = mpmd.format_comma_sep_env_vars()
cmd += ["--export", env_var_str]
if len(env_var_str) > 0:
cmd += ["--export", env_var_str]
cmd += mpmd.exe
cmd += mpmd.exe_args

Expand Down
17 changes: 15 additions & 2 deletions smartsim/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -357,10 +357,23 @@ def run_command(self):
def update_env(self, env_vars):
"""Update the job environment variables
To fully inherit the current user environment, add the
workload-manager-specific flag to the launch command through the
:meth:`add_exe_args` method. For example, ``--export=ALL`` for
slurm, or ``-V`` for PBS/aprun.
:param env_vars: environment variables to update or add
:type env_vars: dict[str, str]
:type env_vars: dict[str, Union[str, int, float, bool]]
:raises TypeError: if env_vars values cannot be coerced to strings
"""
self.env_vars.update(env_vars)
val_types = (str, int, float, bool)
# Coerce env_vars values to str as a convenience to user
for (env, val) in env_vars.items():
if type(val) not in val_types:
raise TypeError(f"env_vars[{env}] was of type {type(val)}, not {val_types_union}")
else:
self.env_vars[env] = val

def add_exe_args(self, args):
"""Add executable arguments to executable
Expand Down
6 changes: 0 additions & 6 deletions smartsim/settings/mpirunSettings.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,16 +224,10 @@ def format_run_args(self):
def format_env_vars(self):
"""Format the environment variables for mpirun
Automatically exports ``PYTHONPATH``, ``LD_LIBRARY_PATH``
and ``PATH``
:return: list of env vars
:rtype: list[str]
"""
formatted = []
presets = ["PATH", "LD_LIBRARY_PATH", "PYTHONPATH"]
for preset in presets:
formatted.extend(["-x", preset])

if self.env_vars:
for name, value in self.env_vars.items():
Expand Down
15 changes: 0 additions & 15 deletions smartsim/settings/slurmSettings.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,25 +291,10 @@ def format_comma_sep_env_vars(self):
:returns: the formatted string of environment variables
:rtype: tuple[str, list[str]]
"""
# TODO make these overridable by user
presets = ["PATH", "LD_LIBRARY_PATH", "PYTHONPATH"]

comma_separated_format_str = []

def add_env_var(var, format_str):
try:
value = os.environ[var]
format_str += "=".join((var, value)) + ","
return format_str
except KeyError:
return format_str

format_str = ""

# add env var presets due to slurm weirdness
for preset in presets:
format_str = add_env_var(preset, format_str)

# add user supplied variables
for k, v in self.env_vars.items():
if "," in str(v):
Expand Down
6 changes: 0 additions & 6 deletions tests/test_openmpi_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,6 @@ def test_format_env():
settings.update_env({"OMP_NUM_THREADS": 10})
formatted = settings.format_env_vars()
result = [
"-x",
"PATH",
"-x",
"LD_LIBRARY_PATH",
"-x",
"PYTHONPATH",
"-x",
"OMP_NUM_THREADS=10",
"-x",
Expand Down

0 comments on commit 8b5ba19

Please sign in to comment.