From e4c381c7b1b8526501b2c6a719dc2b7c7792a364 Mon Sep 17 00:00:00 2001 From: Ziyang Guo <121015044+RerankerGuo@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:49:05 +0800 Subject: [PATCH] fix(python-execute): rename the timeout parameter to survive CLI client kwargs (#587) benchmark.yaml's python_execute job advertised a `timeout` parameter, but `timeout` is one of the CLI's client-selection kwargs (reme/reme.py pops it before the payload is sent), so `reme python_execute code=... timeout=300` never delivered the timeout to the job. The value silently became the HTTP client's request timeout while the subprocess kept the 60s default, so benchmark code that needs longer was killed at 60s with no hint why. HTTP and MCP callers were unaffected. Rename the parameter to `python_timeout`, mirroring how the shell job already avoids the same collision with `shell_timeout`. The step prefers the new name and still accepts the legacy `timeout` context key so existing benchmark configs keep working. A new guard test fails if any job in reme/config/*.yaml or a plugin plugin.yaml declares a client-reserved parameter name again. --- reme/config/benchmark.yaml | 2 +- reme/steps/common/python_execute.py | 12 +++++---- tests/unit/test_common_steps.py | 31 ++++++++++++++++++++-- tests/unit/test_reme_cli.py | 41 +++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/reme/config/benchmark.yaml b/reme/config/benchmark.yaml index 99cb05ef..9212092c 100644 --- a/reme/config/benchmark.yaml +++ b/reme/config/benchmark.yaml @@ -139,7 +139,7 @@ jobs: code: type: string description: "Python code to execute. Print the final result to stdout." - timeout: + python_timeout: type: number description: "Execution timeout in seconds; defaults to 60." required: diff --git a/reme/steps/common/python_execute.py b/reme/steps/common/python_execute.py index b98b48db..6da5d441 100644 --- a/reme/steps/common/python_execute.py +++ b/reme/steps/common/python_execute.py @@ -27,7 +27,9 @@ class PythonExecuteStep(BaseStep): assert self.context is not None code = self.context.get("code", "") - timeout, timeout_error = self._parse_timeout(self.context.get("timeout", DEFAULT_TIMEOUT)) + timeout, timeout_error = self._parse_timeout( + self.context.get("python_timeout", self.context.get("timeout", DEFAULT_TIMEOUT)), + ) if not isinstance(code, str) or not code.strip(): self.context.response.success = False self.context.response.answer = "code is required" @@ -45,7 +47,7 @@ class PythonExecuteStep(BaseStep): { "returncode": result.returncode, "stderr": result.stderr, - "timeout": timeout, + "python_timeout": timeout, }, ) return self.context.response @@ -58,7 +60,7 @@ class PythonExecuteStep(BaseStep): { "returncode": result.returncode, "stderr": stderr, - "timeout": timeout, + "python_timeout": timeout, }, ) return self.context.response @@ -94,7 +96,7 @@ class PythonExecuteStep(BaseStep): try: timeout = float(raw) except (TypeError, ValueError): - return DEFAULT_TIMEOUT, "timeout must be a positive number" + return DEFAULT_TIMEOUT, "python_timeout must be a positive number" if timeout <= 0: - return DEFAULT_TIMEOUT, "timeout must be a positive number" + return DEFAULT_TIMEOUT, "python_timeout must be a positive number" return timeout, "" diff --git a/tests/unit/test_common_steps.py b/tests/unit/test_common_steps.py index 1c6aa63d..b9ec3aa3 100644 --- a/tests/unit/test_common_steps.py +++ b/tests/unit/test_common_steps.py @@ -134,13 +134,40 @@ def test_python_execute_step_reports_stderr_on_failure(): def test_python_execute_step_times_out(): """python_execute converts subprocess timeout into a failed response.""" + async def run(): + step = PythonExecuteStep() + resp = await step(code="import time\ntime.sleep(1)", python_timeout=0.01) + assert resp.success is False + assert resp.answer == "Python execution timed out after 0.01s" + assert resp.metadata["python_timeout"] == 0.01 + print("✓ test_python_execute_step_times_out passed") + + _run(run()) + + +def test_python_execute_step_accepts_legacy_timeout_name(): + """Configs written before the rename keep their timeout through the legacy key.""" + async def run(): step = PythonExecuteStep() resp = await step(code="import time\ntime.sleep(1)", timeout=0.01) assert resp.success is False assert resp.answer == "Python execution timed out after 0.01s" - assert resp.metadata["timeout"] == 0.01 - print("✓ test_python_execute_step_times_out passed") + assert resp.metadata["python_timeout"] == 0.01 + print("✓ test_python_execute_step_accepts_legacy_timeout_name passed") + + _run(run()) + + +def test_python_execute_step_rejects_invalid_timeout(): + """A non-positive python_timeout fails the call instead of running unbounded.""" + + async def run(): + step = PythonExecuteStep() + resp = await step(code="print(1)", python_timeout=0) + assert resp.success is False + assert resp.answer == "python_timeout must be a positive number" + print("✓ test_python_execute_step_rejects_invalid_timeout passed") _run(run()) diff --git a/tests/unit/test_reme_cli.py b/tests/unit/test_reme_cli.py index 13d74cbd..2fbc73fd 100644 --- a/tests/unit/test_reme_cli.py +++ b/tests/unit/test_reme_cli.py @@ -7,6 +7,7 @@ import sys from types import SimpleNamespace import pytest +import yaml from reme.components.service import cli_service from reme.components.service.cli_service import CliService @@ -331,6 +332,46 @@ def test_call_server_passes_shell_parameters_as_payload(monkeypatch, capsys): assert capsys.readouterr().out == "ok\n" +def test_call_server_passes_python_execute_parameters_as_payload(monkeypatch, capsys): + """python_execute's timeout parameter avoids the client-side timeout option.""" + seen = {} + _set_client_backend(monkeypatch, _recording_client(seen)) + monkeypatch.setattr(reme_module, "running_app_config", lambda: None) + + async def run(): + await reme_module.call_server( + "python_execute", + backend="http", + code="print(1)", + python_timeout=5, + timeout=1.5, + ) + + asyncio.run(run()) + + assert seen["action"] == "python_execute" + assert seen["payload"] == {"code": "print(1)", "python_timeout": 5} + assert seen["client_kwargs"] == {"timeout": 1.5} + assert capsys.readouterr().out == "ok\n" + + +def test_no_job_parameter_collides_with_client_kwargs(): + """A job parameter named like a client option could never be set through the CLI.""" + reserved = set(reme_module._CLIENT_KWARGS) | {"backend"} # pylint: disable=protected-access + repo_root = Path(reme_module.__file__).resolve().parents[1] + configs = sorted((repo_root / "reme" / "config").glob("*.yaml")) + configs += sorted(repo_root.glob("plugins/*/src/*/plugin.yaml")) + for config_path in configs: + jobs = yaml.safe_load(config_path.read_text(encoding="utf-8")).get("jobs") or {} + for job_name, job in jobs.items(): + properties = ((job or {}).get("parameters") or {}).get("properties") or {} + clash = sorted(set(properties) & reserved) + assert not clash, ( + f"{config_path.relative_to(repo_root)} job '{job_name}' declares client-reserved " + f"parameters: {clash}" + ) + + def test_call_server_uses_running_plugins_and_their_service_defaults(monkeypatch, capsys): """A bare client call can load the Client backend enabled by the running app.""" seen = {}