From 412541c4c48516444fffbb87c2335669c18d2d92 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Sat, 22 Feb 2025 12:33:25 -0800 Subject: [PATCH 1/3] conditionally quote env vars Signed-off-by: Saurabh --- deepspeed/launcher/multinode_runner.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/deepspeed/launcher/multinode_runner.py b/deepspeed/launcher/multinode_runner.py index 3e0c9d8a1652..218fb02be766 100644 --- a/deepspeed/launcher/multinode_runner.py +++ b/deepspeed/launcher/multinode_runner.py @@ -8,6 +8,7 @@ import shutil import subprocess import warnings +import re from shlex import split from abc import ABC, abstractmethod from deepspeed.accelerator import get_accelerator @@ -34,7 +35,10 @@ def get_cmd(self, environment, active_resources): """Return the command to execute on node""" def add_export(self, key, var): - self.exports[key.strip()] = f"\"{var.strip()}\"" + var = var.strip() + if re.search(r'[^\w@%+=:,./-]', var): + var = f"\"{var}\"" + self.exports[key.strip()] = var def parse_user_args(self): return self.args.user_args From 19b75eb8de3af691921cc3c804feb339cc305b3f Mon Sep 17 00:00:00 2001 From: Saurabh Date: Sat, 8 Mar 2025 14:59:36 -0800 Subject: [PATCH 2/3] Add tests Signed-off-by: Saurabh --- tests/unit/launcher/test_user_args.py | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/unit/launcher/test_user_args.py b/tests/unit/launcher/test_user_args.py index b86be4dfe74c..b61a522eb4b4 100644 --- a/tests/unit/launcher/test_user_args.py +++ b/tests/unit/launcher/test_user_args.py @@ -6,8 +6,18 @@ import pytest import subprocess +from types import SimpleNamespace + from deepspeed.accelerator import get_accelerator +from deepspeed.launcher.multinode_runner import MultiNodeRunner + +class DummyRunner(MultiNodeRunner): + def backend_exists(self): + return True + def get_cmd(self, environment, active_resources): + return [] + if not get_accelerator().is_available(): pytest.skip("only supported in accelerator environments.", allow_module_level=True) @@ -37,6 +47,11 @@ def cmd(user_script_fp, prompt, multi_node): cmd = ("deepspeed", "--num_nodes", "1", "--num_gpus", "1", user_script_fp, "--prompt", prompt) return cmd +@pytest.fixture +def dummy_runner(): + args = SimpleNamespace(user_args=[], user_script="dummy_script.py") + return DummyRunner(args, "dummy_world_info") + @pytest.mark.parametrize("prompt", [ '''"I am 6' tall"''', """'I am 72" tall'""", """'"translate English to Romanian: "'""", @@ -64,3 +79,20 @@ def test_bash_string_args(tmpdir, user_script_fp): p = subprocess.Popen(["bash", bash_fp], stdout=subprocess.PIPE, stderr=subprocess.PIPE) out, err = p.communicate() assert "ARG PARSE SUCCESS" in out.decode("utf-8"), f"User args not parsed correctly: {err.decode('utf-8')}" + + +def test_add_export_with_special_characters(dummy_runner): + """ + Values with special characters (e.g., 64(x2)) must be quoted to avoid bash syntax errors. + """ + dummy_runner.add_export("SLURM_JOB_CPUS_PER_NODE", "64(x2)") + assert dummy_runner.exports["SLURM_JOB_CPUS_PER_NODE"] == "\"64(x2)\"" + + +def test_add_export_no_special_characters(dummy_runner): + """ + Values without special characters should remain unquoted (e.g., PYTHONPATH). + This avoids issues where unnecessary quotes break module imports. + """ + dummy_runner.add_export("PYTHONPATH", "/usr/local/lib/python3.9/site-packages") + assert dummy_runner.exports["PYTHONPATH"] == "/usr/local/lib/python3.9/site-packages" \ No newline at end of file From 7802cd4f9758041d2fb180462b310420b10a0a29 Mon Sep 17 00:00:00 2001 From: Saurabh Koshatwar Date: Tue, 11 Mar 2025 16:39:22 -0700 Subject: [PATCH 3/3] formatting fix Signed-off-by: Saurabh Koshatwar --- tests/unit/launcher/test_user_args.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/unit/launcher/test_user_args.py b/tests/unit/launcher/test_user_args.py index b61a522eb4b4..fd1489803812 100644 --- a/tests/unit/launcher/test_user_args.py +++ b/tests/unit/launcher/test_user_args.py @@ -11,13 +11,16 @@ from deepspeed.accelerator import get_accelerator from deepspeed.launcher.multinode_runner import MultiNodeRunner + class DummyRunner(MultiNodeRunner): + def backend_exists(self): return True def get_cmd(self, environment, active_resources): return [] - + + if not get_accelerator().is_available(): pytest.skip("only supported in accelerator environments.", allow_module_level=True) @@ -47,6 +50,7 @@ def cmd(user_script_fp, prompt, multi_node): cmd = ("deepspeed", "--num_nodes", "1", "--num_gpus", "1", user_script_fp, "--prompt", prompt) return cmd + @pytest.fixture def dummy_runner(): args = SimpleNamespace(user_args=[], user_script="dummy_script.py") @@ -95,4 +99,4 @@ def test_add_export_no_special_characters(dummy_runner): This avoids issues where unnecessary quotes break module imports. """ dummy_runner.add_export("PYTHONPATH", "/usr/local/lib/python3.9/site-packages") - assert dummy_runner.exports["PYTHONPATH"] == "/usr/local/lib/python3.9/site-packages" \ No newline at end of file + assert dummy_runner.exports["PYTHONPATH"] == "/usr/local/lib/python3.9/site-packages"