Skip to content

Commit c25b36c

Browse files
committed
fix(cli): only match prefixed overwrites on word boundaries in _parse_prefixed_args
The prefix detection in _parse_prefixed_args used startswith(prefix), so any task argument whose parameter name merely starts with a reserved prefix was rejected with a misleading error, e.g.: ValueError: Run overwrites must start with 'run.'. Got runtime=3600 Task parameters named runtime, run_id, executors, plugins_dir, etc. crashed every CLI invocation. Prefixed args are now only matched when directly followed by a delimiter ('=', '.', '['); keyword arguments for task parameters are passed through untouched. Bare tokens starting with the prefix still raise the original error. Additionally, only the leading prefix occurrence is stripped (previously run.a.run.b=1 was mangled to ab=1, silently targeting the wrong parameter) and values containing '=' are no longer truncated (executor=k=v previously yielded value 'k'). Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
1 parent b85eb67 commit c25b36c

2 files changed

Lines changed: 76 additions & 12 deletions

File tree

‎nemo_run/cli/api.py‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1659,19 +1659,22 @@ def _parse_prefixed_args(
16591659
"""
16601660
prefixed_arg_value, prefixed_args, other_args = None, [], []
16611661
for arg in args:
1662-
if arg.startswith(prefix):
1663-
if arg.startswith(f"{prefix}="):
1664-
prefixed_arg_value = arg.split("=")[1]
1665-
else:
1666-
if not arg.startswith(f"{prefix}.") and not arg.startswith(f"{prefix}["):
1667-
raise ValueError(
1668-
f"{prefix.capitalize()} overwrites must start with '{prefix}.'. Got {arg}"
1669-
)
1670-
if arg.startswith(f"{prefix}."):
1671-
prefixed_args.append(arg.replace(f"{prefix}.", ""))
1672-
elif arg.startswith(f"{prefix}["):
1673-
prefixed_args.append(arg.replace(prefix, ""))
1662+
if arg.startswith(f"{prefix}="):
1663+
prefixed_arg_value = arg.split("=", 1)[1]
1664+
elif arg.startswith(f"{prefix}."):
1665+
prefixed_args.append(arg.replace(f"{prefix}.", "", 1))
1666+
elif arg.startswith(f"{prefix}["):
1667+
prefixed_args.append(arg.replace(prefix, "", 1))
1668+
elif arg.startswith(prefix) and "=" not in arg:
1669+
# A bare token starting with the prefix (e.g. a positional value)
1670+
# cannot address a task parameter, so treat it as a malformed overwrite.
1671+
raise ValueError(
1672+
f"{prefix.capitalize()} overwrites must start with '{prefix}.'. Got {arg}"
1673+
)
16741674
else:
1675+
# Keyword arguments for parameters whose names merely start with the
1676+
# prefix (e.g. runtime=3600 for prefix "run") belong to the task,
1677+
# not to the prefixed namespace.
16751678
other_args.append(arg)
16761679
return prefixed_arg_value, prefixed_args, other_args
16771680

‎test/cli/test_api.py‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,22 @@ def test_run_context_execute_task(self, mock_run, mock_dryrun_fn, sample_functio
167167
mock_dryrun_fn.assert_called_once()
168168
mock_run.assert_called_once()
169169

170+
@patch("nemo_run.dryrun_fn")
171+
@patch("nemo_run.run")
172+
def test_run_context_execute_task_with_run_prefixed_parameter(
173+
self, mock_run, mock_dryrun_fn
174+
):
175+
"""Task parameters whose names start with a reserved prefix (run/executor/plugins)
176+
must be treated as task args, not as malformed prefixed overwrites."""
177+
ctx = RunContext(name="test_run", skip_confirmation=True)
178+
179+
def sample_function(runtime: int = 60, lr: float = 0.1):
180+
return None
181+
182+
ctx.cli_execute(sample_function, ["runtime=120", "lr=0.2"])
183+
mock_dryrun_fn.assert_called_once()
184+
mock_run.assert_called_once()
185+
170186
def test_run_context_to_config(self):
171187
ctx = RunContext(name="test_run")
172188
config = ctx.to_config()
@@ -1163,6 +1179,51 @@ def test_parse_prefixed_args_no_prefix(self):
11631179
assert prefix_args == []
11641180
assert other_args == ["arg1=value1", "arg2=value2"]
11651181

1182+
def test_parse_prefixed_args_word_boundary(self):
1183+
"""Keyword args whose parameter names merely start with the prefix are task args."""
1184+
from nemo_run.cli.api import _parse_prefixed_args
1185+
1186+
args = ["runtime=3600", "run_id=42", "lr=0.1"]
1187+
prefix_value, prefix_args, other_args = _parse_prefixed_args(args, "run")
1188+
1189+
assert prefix_value is None
1190+
assert prefix_args == []
1191+
assert other_args == args
1192+
1193+
prefix_value, prefix_args, other_args = _parse_prefixed_args(["executors=2"], "executor")
1194+
assert prefix_value is None
1195+
assert prefix_args == []
1196+
assert other_args == ["executors=2"]
1197+
1198+
prefix_value, prefix_args, other_args = _parse_prefixed_args(
1199+
["plugins_dir=/tmp"], "plugins"
1200+
)
1201+
assert prefix_value is None
1202+
assert prefix_args == []
1203+
assert other_args == ["plugins_dir=/tmp"]
1204+
1205+
# Nested keys starting with the prefix stay task args as well.
1206+
_, prefix_args, other_args = _parse_prefixed_args(["runtime.limit=60"], "run")
1207+
assert prefix_args == []
1208+
assert other_args == ["runtime.limit=60"]
1209+
1210+
def test_parse_prefixed_args_nested_key_not_corrupted(self):
1211+
"""Only the leading prefix is stripped from prefixed args."""
1212+
from nemo_run.cli.api import _parse_prefixed_args
1213+
1214+
_, prefix_args, _ = _parse_prefixed_args(["run.a.run.b=1"], "run")
1215+
assert prefix_args == ["a.run.b=1"]
1216+
1217+
_, prefix_args, _ = _parse_prefixed_args(["plugins[0].plugins_dir=x"], "plugins")
1218+
assert prefix_args == ["[0].plugins_dir=x"]
1219+
1220+
def test_parse_prefixed_args_value_with_equals(self):
1221+
"""Values containing '=' are not truncated at the first '='."""
1222+
from nemo_run.cli.api import _parse_prefixed_args
1223+
1224+
prefix_value, _, _ = _parse_prefixed_args(["executor=k=v"], "executor")
1225+
assert prefix_value == "k=v"
1226+
11661227

11671228
class TestConfigExport:
11681229
@pytest.fixture

0 commit comments

Comments
 (0)