From 18b145549304d0a70d0a10171be1cc3d1e893652 Mon Sep 17 00:00:00 2001 From: Amanda Richardson Date: Mon, 19 Aug 2024 21:44:41 -0500 Subject: [PATCH] all tests pass --- .../_core/launcher/dragon/dragonLauncher.py | 7 ++++-- smartsim/settings/arguments/launch/lsf.py | 23 ++++++++++++++++-- smartsim/settings/dispatch.py | 6 ++--- .../test_settings/test_dragonLauncher.py | 6 +++-- .../test_settings/test_lsfLauncher.py | 24 +++++++++---------- tests/test_shell_launcher.py | 3 ++- 6 files changed, 47 insertions(+), 22 deletions(-) diff --git a/smartsim/_core/launcher/dragon/dragonLauncher.py b/smartsim/_core/launcher/dragon/dragonLauncher.py index 992707959..56bb10bff 100644 --- a/smartsim/_core/launcher/dragon/dragonLauncher.py +++ b/smartsim/_core/launcher/dragon/dragonLauncher.py @@ -28,6 +28,7 @@ import os import typing as t +import pathlib from smartsim._core.schemas.dragonRequests import DragonRunPolicy from smartsim.error import errors @@ -368,6 +369,8 @@ def _as_run_request_args_and_policy( exe: ExecutableProtocol, path: str | os.PathLike[str], env: t.Mapping[str, str | None], + stdout_path: pathlib.Path, + stderr_path: pathlib.Path, ) -> tuple[DragonRunRequestView, DragonRunPolicy]: # ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ # FIXME: This type is 100% unacceptable, but I don't want to spend too much @@ -389,8 +392,8 @@ def _as_run_request_args_and_policy( env=env, # TODO: Not sure how this info is injected name=None, - output_file=None, - error_file=None, + output_file=stdout_path, + error_file=stderr_path, **run_args, ), policy, diff --git a/smartsim/settings/arguments/launch/lsf.py b/smartsim/settings/arguments/launch/lsf.py index 80cd748f1..b8b6f5d91 100644 --- a/smartsim/settings/arguments/launch/lsf.py +++ b/smartsim/settings/arguments/launch/lsf.py @@ -27,16 +27,35 @@ from __future__ import annotations import typing as t +import pathlib +import subprocess from smartsim.log import get_logger -from smartsim.settings.dispatch import ShellLauncher, dispatch, make_shell_format_fn +from smartsim.settings.dispatch import ShellLauncher, dispatch, ExecutableProtocol, _EnvironMappingType, ShellLauncherCommand from ...common import set_check_input from ...launchCommand import LauncherType from ..launchArguments import LaunchArguments logger = get_logger(__name__) -_as_jsrun_command = make_shell_format_fn(run_command="jsrun") + +def _as_jsrun_command(args: LaunchArguments, + exe: ExecutableProtocol, + path: pathlib.Path, + env: _EnvironMappingType, + stdout_path: pathlib.Path, + stderr_path: pathlib.Path) -> ShellLauncherCommand: + command_tuple = ( + "jsrun", + *(args.format_launch_args() or ()), + "--", + *exe.as_program_arguments(), + f"--stdio_stdout={stdout_path}", + f"--stdio_stderr={stderr_path}", + + ) + # add output and err to CMD tuple -> add dev Null for stdout and stderr + return ShellLauncherCommand(env, path, subprocess.DEVNULL, subprocess.DEVNULL, command_tuple) @dispatch(with_format=_as_jsrun_command, to_launcher=ShellLauncher) diff --git a/smartsim/settings/dispatch.py b/smartsim/settings/dispatch.py index 57ea2098b..a2af2bd4f 100644 --- a/smartsim/settings/dispatch.py +++ b/smartsim/settings/dispatch.py @@ -54,8 +54,8 @@ class ShellLauncherCommand(t.NamedTuple): env: _EnvironMappingType path: pathlib.Path - stdout: pathlib.Path - stderr: pathlib.Path + stdout: TextIOWrapper + stderr: TextIOWrapper command_tuple: tuple[str, tuple[str, ...]] | t.Sequence[str] @@ -516,7 +516,7 @@ def start( exe, *rest = shell_command.command_tuple expanded_exe = helpers.expand_exe_path(exe) # pylint: disable-next=consider-using-with - self._launched[id_] = sp.Popen((expanded_exe, *rest), cwd=shell_command.path, env={k:v for k,v in shell_command.env.items() if v is not None}, stdout=open(shell_command.stdout), stderr=open(shell_command.stderr)) + self._launched[id_] = sp.Popen((expanded_exe, *rest), cwd=shell_command.path, env={k:v for k,v in shell_command.env.items() if v is not None}, stdout=shell_command.stdout, stderr=shell_command.stderr) # Popen starts a new process and gives you back a handle to process, getting back the pid - process id return id_ diff --git a/tests/temp_tests/test_settings/test_dragonLauncher.py b/tests/temp_tests/test_settings/test_dragonLauncher.py index 38ee11486..3b2837a60 100644 --- a/tests/temp_tests/test_settings/test_dragonLauncher.py +++ b/tests/temp_tests/test_settings/test_dragonLauncher.py @@ -78,7 +78,7 @@ def test_formatting_launch_args_into_request( if gpu_affinity is not NOT_SET: launch_args.set_gpu_affinity(gpu_affinity) req, policy = _as_run_request_args_and_policy( - launch_args, mock_echo_executable, test_dir, {} + launch_args, mock_echo_executable, test_dir, {}, "output.txt", "error.txt" ) expected_args = { @@ -90,7 +90,7 @@ def test_formatting_launch_args_into_request( if v is not NOT_SET } expected_run_req = DragonRunRequestView( - exe="echo", exe_args=["hello", "world"], path=test_dir, env={}, **expected_args + exe="echo", exe_args=["hello", "world"], path=test_dir, env={}, output_file="output.txt", error_file="error.txt", **expected_args ) assert req.exe == expected_run_req.exe assert req.exe_args == expected_run_req.exe_args @@ -99,6 +99,8 @@ def test_formatting_launch_args_into_request( assert req.hostlist == expected_run_req.hostlist assert req.pmi_enabled == expected_run_req.pmi_enabled assert req.path == expected_run_req.path + assert req.output_file == expected_run_req.output_file + assert req.error_file == expected_run_req.error_file expected_run_policy_args = { k: v diff --git a/tests/temp_tests/test_settings/test_lsfLauncher.py b/tests/temp_tests/test_settings/test_lsfLauncher.py index 6d7df7c7c..e3879a3bc 100644 --- a/tests/temp_tests/test_settings/test_lsfLauncher.py +++ b/tests/temp_tests/test_settings/test_lsfLauncher.py @@ -24,7 +24,7 @@ # OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. import pytest -import pathlib +import subprocess from smartsim.settings import LaunchSettings from smartsim.settings.arguments.launch.lsf import ( @@ -92,42 +92,42 @@ def test_launch_args(): @pytest.mark.parametrize( "args, expected", ( - pytest.param({}, ("jsrun", "--", "echo", "hello", "world"), id="Empty Args"), + pytest.param({}, ("jsrun", "--", "echo", "hello", "world", '--stdio_stdout=output.txt', '--stdio_stderr=error.txt'), id="Empty Args"), pytest.param( {"n": "1"}, - ("jsrun", "-n", "1", "--", "echo", "hello", "world"), + ("jsrun", "-n", "1", "--", "echo", "hello", "world", '--stdio_stdout=output.txt', '--stdio_stderr=error.txt'), id="Short Arg", ), pytest.param( {"nrs": "1"}, - ("jsrun", "--nrs=1", "--", "echo", "hello", "world"), + ("jsrun", "--nrs=1", "--", "echo", "hello", "world", '--stdio_stdout=output.txt', '--stdio_stderr=error.txt'), id="Long Arg", ), pytest.param( {"v": None}, - ("jsrun", "-v", "--", "echo", "hello", "world"), + ("jsrun", "-v", "--", "echo", "hello", "world", '--stdio_stdout=output.txt', '--stdio_stderr=error.txt'), id="Short Arg (No Value)", ), pytest.param( {"verbose": None}, - ("jsrun", "--verbose", "--", "echo", "hello", "world"), + ("jsrun", "--verbose", "--", "echo", "hello", "world", '--stdio_stdout=output.txt', '--stdio_stderr=error.txt'), id="Long Arg (No Value)", ), pytest.param( {"tasks_per_rs": "1", "n": "123"}, - ("jsrun", "--tasks_per_rs=1", "-n", "123", "--", "echo", "hello", "world"), + ("jsrun", "--tasks_per_rs=1", "-n", "123", "--", "echo", "hello", "world", '--stdio_stdout=output.txt', '--stdio_stderr=error.txt'), id="Short and Long Args", ), ), ) def test_formatting_launch_args(mock_echo_executable, args, expected, test_dir): - outfile = "out.txt" - errfile = "err.txt" + outfile = "output.txt" + errfile = "error.txt" env, path, stdin, stdout, args = _as_jsrun_command( JsrunLaunchArguments(args), mock_echo_executable, test_dir, {}, outfile, errfile ) assert tuple(args) == expected - assert path == pathlib.Path(test_dir) + assert path == test_dir assert env == {} - assert stdin == f"--stdio_stdout={outfile}" - assert stdout == f"--stdio_stderr={errfile}" + assert stdin == subprocess.DEVNULL + assert stdout == subprocess.DEVNULL diff --git a/tests/test_shell_launcher.py b/tests/test_shell_launcher.py index b59093c27..7d5641bcf 100644 --- a/tests/test_shell_launcher.py +++ b/tests/test_shell_launcher.py @@ -27,6 +27,7 @@ import tempfile import unittest.mock import pytest +import subprocess import pathlib import psutil import difflib @@ -135,7 +136,7 @@ def test_popen_returns_popen_object(test_dir: str): shell_launcher = ShellLauncher() run_dir, out_file, err_file = generate_directory(test_dir) with open(out_file, "w", encoding="utf-8") as out, open(err_file, "w", encoding="utf-8") as err: - cmd = ShellLauncherCommand({}, run_dir, out, err, EchoHelloWorldEntity().as_program_arguments()) + cmd = ShellLauncherCommand({}, run_dir, subprocess.DEVNULL, subprocess.DEVNULL, EchoHelloWorldEntity().as_program_arguments()) id = shell_launcher.start(cmd) proc = shell_launcher._launched[id] assert isinstance(proc, sp.Popen)