From f68fdd2a55baa4610013f7b7253eb07d271b0016 Mon Sep 17 00:00:00 2001 From: "praisonai-triage-agent[bot]" <272766704+praisonai-triage-agent[bot]@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:17:22 +0000 Subject: [PATCH 1/3] fix(train): correct remote status/dataset/dry-run and Windows CI skip - Detect failed status by prefix: runner.status() returns 'failed (exit N)', so the equality check reported failed remote runs as success. - Ship a dataset named only in --config when it is a local file, so the remote process is not handed a path that exists only locally. - Resolve and validate the remote block in --dry-run so the preview matches the real dispatch and bad settings are caught before renting a GPU. - Reject falsey non-mapping remote values ([], '', false) instead of quietly treating them as train-locally. - Assert typer.Exit in the no-remote dispatch test rather than swallowing every exception; skip the POSIX-only keyring tests on Windows. Co-authored-by: Mervin Praison --- .../engine/test_portability.py | 3 ++ .../praisonai_train/cli/commands/train.py | 40 ++++++++++++++++--- .../praisonai_train/remote/settings.py | 13 ++++-- .../tests/unit/test_remote_dispatch.py | 39 ++++++++++++++++-- .../tests/unit/test_remote_parity.py | 12 ++++++ 5 files changed, 96 insertions(+), 11 deletions(-) diff --git a/src/praisonai-desktop/engine/test_portability.py b/src/praisonai-desktop/engine/test_portability.py index 211b4a0b9..d2895a7e9 100644 --- a/src/praisonai-desktop/engine/test_portability.py +++ b/src/praisonai-desktop/engine/test_portability.py @@ -282,6 +282,9 @@ def setUp(self): os.environ["PATH"] = self.bin + os.pathsep + self.old_path def tearDown(self): + # setUp skips before these are set on Windows. + if getattr(self, "bin", None) is None: + return os.environ["PATH"] = self.old_path shutil.rmtree(self.bin, ignore_errors=True) diff --git a/src/praisonai-train/praisonai_train/cli/commands/train.py b/src/praisonai-train/praisonai_train/cli/commands/train.py index 151dc4f1a..3a139ee54 100644 --- a/src/praisonai-train/praisonai_train/cli/commands/train.py +++ b/src/praisonai-train/praisonai_train/cli/commands/train.py @@ -153,7 +153,7 @@ def _v(value): # the difference between a caught typo and a wasted run. Resolved and # printed WITHOUT loading the (heavy, optional) runner, so a preview # never depends on the training deps being installed. - _print_resolved_config(config, overrides) + _print_resolved_config(config, overrides, remote_overrides) return if not dataset and not config: @@ -268,9 +268,18 @@ def _dispatch_remote(resolved, remote_overrides, config_path, dataset): workdir=block["workdir"]) shipped = _write_shipped_config(resolved) + # A dataset named only in --config still has to be copied. Fall back to the + # resolved dataset when the positional argument is absent, but only when it + # points at a local file -- a HuggingFace id or a path already on the remote + # host is not something to ship. + ship_dataset = dataset + if not ship_dataset: + resolved_dataset = resolved.get("dataset") + if isinstance(resolved_dataset, str) and Path(resolved_dataset).is_file(): + ship_dataset = resolved_dataset try: run = runner.start(config_path=shipped, - dataset_path=Path(dataset) if dataset else None, + dataset_path=Path(ship_dataset) if ship_dataset else None, expect_gpus=block["gpus"]) except RemoteError as exc: output.print_error(f"Could not start the run on {block['host']}", @@ -290,7 +299,10 @@ def _dispatch_remote(resolved, remote_overrides, config_path, dataset): runner.tail(run, on_line=typer.echo) state = runner.status(run) typer.echo(f"status: {state}") - if state == "failed": + # status() returns "failed (exit N)", not a bare "failed", so an equality + # check would print the failure and then exit 0 -- reporting success for a + # run that did not complete. + if state.startswith("failed"): raise typer.Exit(1) return True @@ -370,11 +382,29 @@ def _resolve_config(config_path, overrides): return resolved -def _print_resolved_config(config_path, overrides): - """Show the config the run would use: the file, then the flags on top.""" +def _print_resolved_config(config_path, overrides, remote_overrides=None): + """Show the config the run would use: the file, then the flags on top. + + The remote block is resolved with the same precedence and validation as the + real dispatch, so the preview shows the host, interpreter, workdir and GPU + count the run would actually use -- and a bad remote setting is caught here + rather than after an hour of rented GPU. + """ import yaml + from ..output.console import get_output_controller + from praisonai_train.remote import settings as remote_settings + resolved = _resolve_config(config_path, overrides) + + try: + block = remote_settings.resolve(resolved, remote_overrides or {}) + except remote_settings.RemoteSettingsError as exc: + get_output_controller().print_error("Bad remote settings", remediation=str(exc)) + raise typer.Exit(1) from exc + if block: + resolved["remote"] = remote_settings.redact(block) + typer.echo(yaml.safe_dump(resolved, sort_keys=True, default_flow_style=False).rstrip()) if overrides: typer.echo(f"\n# {len(overrides)} value(s) came from flags: " diff --git a/src/praisonai-train/praisonai_train/remote/settings.py b/src/praisonai-train/praisonai_train/remote/settings.py index 305d045fa..d7ab96bfe 100644 --- a/src/praisonai-train/praisonai_train/remote/settings.py +++ b/src/praisonai-train/praisonai_train/remote/settings.py @@ -55,11 +55,18 @@ def resolve(config: dict, overrides: dict) -> dict: wins. Returns {} when no host is settled, which is how "train locally" is expressed -- there is no separate mode switch to get out of step with it. """ - block = config.get("remote") or {} - if not isinstance(block, dict): + # Only an omitted or null `remote` means "train locally". A falsey but + # present value -- [], "", false -- is a malformed block, not an absence, + # and `... or {}` would have quietly swallowed it into local training. + raw = config.get("remote") + if raw is None: + block = {} + elif not isinstance(raw, dict): raise RemoteSettingsError( "remote: must be a mapping of key: value, not " - f"{type(block).__name__}") + f"{type(raw).__name__}") + else: + block = raw merged = defaults() merged.update({k: v for k, v in block.items() if v is not None}) diff --git a/src/praisonai-train/tests/unit/test_remote_dispatch.py b/src/praisonai-train/tests/unit/test_remote_dispatch.py index cc3f5d456..cbef20659 100644 --- a/src/praisonai-train/tests/unit/test_remote_dispatch.py +++ b/src/praisonai-train/tests/unit/test_remote_dispatch.py @@ -105,6 +105,22 @@ def test_it_does_not_write_config_yaml_into_the_working_directory( assert not (tmp_path / "config.yaml").exists(), ( "a remote run clobbered ./config.yaml") + def test_a_dataset_named_only_in_the_config_is_shipped(self, tmp_path): + # No positional dataset argument, but the resolved config names a local + # file. It must still be copied, or the remote process gets a path that + # exists only on this machine. + data = tmp_path / "d.json" + data.write_text("[]", encoding="utf-8") + _dispatch({"remote": {"host": "gpubox"}, "dataset": str(data)}) + sent = FakeRunner.instances[0].started_with["dataset"] + assert sent is not None and pathlib.Path(sent) == data + + def test_a_non_local_dataset_is_not_shipped(self, tmp_path): + # A HuggingFace id (or a path already on the far side) is not a file to + # copy. + _dispatch({"remote": {"host": "gpubox"}, "dataset": "org/dataset"}) + assert FakeRunner.instances[0].started_with["dataset"] is None + def test_the_gpu_expectation_is_passed_through(self): _dispatch({"remote": {"host": "gpubox", "gpus": 4}}) assert FakeRunner.instances[0].started_with["gpus"] == 4 @@ -130,6 +146,20 @@ def status(self, run): with pytest.raises(typer.Exit): _dispatch({"remote": {"host": "gpubox"}}) + def test_a_failed_status_with_an_exit_code_is_reported_as_failure(self, monkeypatch): + # status() returns "failed (exit N)", not a bare "failed". An equality + # check matched neither and reported a failed run as success. + import typer + + class Failing(FakeRunner): + def status(self, run): + return "failed (exit 1)" + + import praisonai_train.remote.runner as runner_mod + monkeypatch.setattr(runner_mod, "RemoteRunner", Failing) + with pytest.raises(typer.Exit): + _dispatch({"remote": {"host": "gpubox"}}) + def test_bad_remote_settings_exit_rather_than_training_locally(self): import typer with pytest.raises(typer.Exit): @@ -174,13 +204,16 @@ def test_llm_without_remote_settings_does_not_dispatch(self, tmp_path, monkeypat monkeypatch.chdir(tmp_path) config = _config(tmp_path, {"model_name": "unsloth/tiny", "dataset": "d.json"}) # Stop before the heavy import; the point is only that nothing was sent. + import typer + monkeypatch.setattr(train_cmd, "import_code_module", lambda *_a, **_k: (_ for _ in ()).throw(ImportError("no")), raising=False) - try: + # The ImportError is surfaced as a typer.Exit; asserting it keeps this + # test honest -- a blind `except Exception` would pass even if the + # command failed for an unrelated reason before the local trainer. + with pytest.raises(typer.Exit): train_cmd.train_llm(config=config) - except Exception: - pass assert FakeRunner.instances == [], "a local run was sent to a remote host" def test_dry_run_shows_the_remote_block_and_sends_nothing(self, tmp_path, monkeypatch): diff --git a/src/praisonai-train/tests/unit/test_remote_parity.py b/src/praisonai-train/tests/unit/test_remote_parity.py index 977844f0e..a777cd450 100644 --- a/src/praisonai-train/tests/unit/test_remote_parity.py +++ b/src/praisonai-train/tests/unit/test_remote_parity.py @@ -116,3 +116,15 @@ def test_gpus_must_be_a_positive_integer(self): def test_a_non_mapping_remote_block_is_refused(self): with pytest.raises(settings.RemoteSettingsError): settings.resolve({"remote": "gpubox"}, {}) + + def test_a_falsey_non_mapping_remote_block_is_refused(self): + # [], "" and false are present but malformed. `remote: or {}` would + # have swallowed them into local training instead of flagging the + # mistake -- only an omitted or null value means "train here". + for bad in ([], "", False): + with pytest.raises(settings.RemoteSettingsError): + settings.resolve({"remote": bad}, {}) + + def test_an_omitted_or_null_remote_block_trains_here(self): + assert settings.resolve({}, {}) == {} + assert settings.resolve({"remote": None}, {}) == {} From 1e877e5febc981aa87fc3aa412db994a3dff242c Mon Sep 17 00:00:00 2001 From: MervinPraison Date: Thu, 27 Aug 2026 13:05:51 +0100 Subject: [PATCH 2/3] test(train): the fake spoke a status vocabulary the runner does not The triage bot fixed a real bug of mine: the dispatch compared `state == "failed"` while status() returns `"failed (exit N)"`, so a remote run that failed was reported as a success. My test did not catch it because my fake returned exactly `"failed"` -- and `"done"` for success, which the runner never returns either. The fake agreed with the bug, so the test passed and the code was wrong. That is the same defect as the code it was meant to check, one level up. The fake now speaks the runner's actual vocabulary -- completed, running, unknown, `failed (exit N)` -- and a test reads runner.py from disk and asserts those spellings still exist, so the fake cannot quietly drift from the thing it stands in for. Deleting `failed (exit` from the runner now fails the dispatch tests. Two test-harness repairs alongside it. `monkeypatch.setattr(train_cmd, "import_code_module", ...)` never intercepted anything: train_llm imports that name inside the function body, so the patch has to be on praisonai_train._code_bridge, where the lookup happens. And the vocabulary test read RemoteRunner through the module -- which the autouse fixture has already replaced with the fake, so it would have asserted that the fake agrees with itself. Three mutations, three caught, including restoring the equality comparison. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/unit/test_remote_dispatch.py | 57 +++++++++++++++++-- 1 file changed, 51 insertions(+), 6 deletions(-) diff --git a/src/praisonai-train/tests/unit/test_remote_dispatch.py b/src/praisonai-train/tests/unit/test_remote_dispatch.py index cbef20659..3c17f2312 100644 --- a/src/praisonai-train/tests/unit/test_remote_dispatch.py +++ b/src/praisonai-train/tests/unit/test_remote_dispatch.py @@ -28,7 +28,11 @@ def __init__(self, host, python, workdir): self.host, self.python, self.workdir = host, python, workdir self.started_with = None self.tailed = False - self.state = "done" + # The vocabulary the real runner speaks, not one invented here. It + # returned "done" before, and the dispatch compared against "failed" + # with ==, so a run that ended "failed (exit 1)" was reported as a + # success and the fake agreed with the bug. + self.state = "completed" FakeRunner.instances.append(self) def start(self, config_path=None, dataset_path=None, expect_gpus=1, **_): @@ -133,13 +137,53 @@ def test_the_interpreter_and_workdir_are_passed_through(self): assert made.workdir == "~/runs" +class TestTheStatusVocabulary: + """The fake must speak the language the real runner speaks.""" + + def test_the_fake_only_returns_states_the_runner_can_return(self): + # Read from disk, not through the module: the autouse fixture has + # already swapped RemoteRunner for the fake, so inspecting the + # attribute would have this test confirm the fake agrees with itself. + source = (pathlib.Path(__file__).resolve().parents[2] + / "praisonai_train" / "remote" / "runner.py").read_text() + for word in ("completed", "running", "unknown"): + assert word in source, ( + f"the runner no longer reports {word!r}; the fakes here are " + "built on that vocabulary") + assert "failed (exit" in source, ( + "the runner no longer reports 'failed (exit N)' -- the dispatch " + "matches that shape by prefix") + + @pytest.mark.parametrize("state", ["failed (exit 1)", "failed (exit 137)"]) + def test_a_failed_run_is_a_failure_however_it_is_spelled(self, state, monkeypatch): + import typer + + class Failing(FakeRunner): + def status(self, run): + return state + + import praisonai_train.remote.runner as runner_mod + monkeypatch.setattr(runner_mod, "RemoteRunner", Failing) + with pytest.raises(typer.Exit): + _dispatch({"remote": {"host": "gpubox"}}) + + def test_a_completed_run_is_not_a_failure(self, monkeypatch): + class Completed(FakeRunner): + def status(self, run): + return "completed" + + import praisonai_train.remote.runner as runner_mod + monkeypatch.setattr(runner_mod, "RemoteRunner", Completed) + assert _dispatch({"remote": {"host": "gpubox"}}) is True + + class TestFailure: def test_a_run_that_ends_failed_is_reported_as_failure(self, monkeypatch): import typer class Failing(FakeRunner): def status(self, run): - return "failed" + return "failed (exit 1)" import praisonai_train.remote.runner as runner_mod monkeypatch.setattr(runner_mod, "RemoteRunner", Failing) @@ -187,7 +231,8 @@ def test_llm_with_a_remote_config_never_reaches_the_local_trainer( def _boom(*_a, **_k): raise AssertionError("the local trainer was reached for a remote run") - monkeypatch.setattr(train_cmd, "import_code_module", _boom, raising=False) + import praisonai_train._code_bridge as bridge + monkeypatch.setattr(bridge, "import_code_module", _boom) train_cmd.train_llm(config=config) assert len(FakeRunner.instances) == 1, "the run was not sent anywhere" @@ -206,9 +251,9 @@ def test_llm_without_remote_settings_does_not_dispatch(self, tmp_path, monkeypat # Stop before the heavy import; the point is only that nothing was sent. import typer - monkeypatch.setattr(train_cmd, "import_code_module", - lambda *_a, **_k: (_ for _ in ()).throw(ImportError("no")), - raising=False) + import praisonai_train._code_bridge as bridge + monkeypatch.setattr(bridge, "import_code_module", + lambda *_a, **_k: (_ for _ in ()).throw(ImportError("no"))) # The ImportError is surfaced as a typer.Exit; asserting it keeps this # test honest -- a blind `except Exception` would pass even if the # command failed for an unrelated reason before the local trainer. From 6b1a792ad75bde8834a0d7695294d5ca65786da3 Mon Sep 17 00:00:00 2001 From: "praisonai-triage-agent[bot]" <272766704+praisonai-triage-agent[bot]@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:47:56 +0000 Subject: [PATCH 3/3] fix(train): ship list-form and data_files local datasets to remote hosts The remote dispatch fallback only recognised a dataset given as a bare string, but the trainer's canonical shape is a list of mappings and it loads any name/data_files that exists on disk. A config using `dataset: [{name: ./local.json}]` therefore had its file left behind, and the remote run failed on a path that existed only on the launching machine. _local_dataset_in now inspects both forms, mirroring how the trainer resolves a local file, and returns the first local dataset to ship. Co-authored-by: Mervin Praison --- .../praisonai_train/cli/commands/train.py | 44 ++++++++++++++++--- .../tests/unit/test_remote_dispatch.py | 28 ++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/src/praisonai-train/praisonai_train/cli/commands/train.py b/src/praisonai-train/praisonai_train/cli/commands/train.py index 3a139ee54..cfc63d3dd 100644 --- a/src/praisonai-train/praisonai_train/cli/commands/train.py +++ b/src/praisonai-train/praisonai_train/cli/commands/train.py @@ -272,11 +272,7 @@ def _dispatch_remote(resolved, remote_overrides, config_path, dataset): # resolved dataset when the positional argument is absent, but only when it # points at a local file -- a HuggingFace id or a path already on the remote # host is not something to ship. - ship_dataset = dataset - if not ship_dataset: - resolved_dataset = resolved.get("dataset") - if isinstance(resolved_dataset, str) and Path(resolved_dataset).is_file(): - ship_dataset = resolved_dataset + ship_dataset = dataset or _local_dataset_in(resolved) try: run = runner.start(config_path=shipped, dataset_path=Path(ship_dataset) if ship_dataset else None, @@ -307,6 +303,44 @@ def _dispatch_remote(resolved, remote_overrides, config_path, dataset): return True +def _local_dataset_in(resolved): + """The one local dataset file the resolved config names, or None. + + The trainer accepts a dataset as a bare string or as the list-of-mappings + it normalises to -- `[{name: ...}]`, optionally with `data_files` -- and + loads any `name`/`data_files` that `os.path.exists` as a local file + (praisonai_train/train/llm/trainer.py:882). A remote run has to copy that + file, or the far side is handed a path that exists only on this machine. + Only the string form was covered before, so the canonical list form went + unshipped. + + A HuggingFace id or a path already on the remote host is not a file here, + so it is left alone. Only the first local file is returned: the runner + ships a single positional dataset, which matches how a `--config` run is + launched. + """ + entries = resolved.get("dataset") + if isinstance(entries, str): + entries = [entries] + elif not isinstance(entries, list): + return None + + for entry in entries: + if isinstance(entry, str): + candidate = entry + elif isinstance(entry, dict): + # `data_files` is the explicit local file; `name` doubles as a path + # when it is one, exactly as the trainer treats it. + candidate = entry.get("data_files") or entry.get("name") + if isinstance(candidate, (list, tuple)): + candidate = candidate[0] if candidate else None + else: + continue + if isinstance(candidate, str) and Path(candidate).is_file(): + return candidate + return None + + def _write_shipped_config(resolved): """The config to send, in a temp file. Returns its path. diff --git a/src/praisonai-train/tests/unit/test_remote_dispatch.py b/src/praisonai-train/tests/unit/test_remote_dispatch.py index 3c17f2312..2db18f2c2 100644 --- a/src/praisonai-train/tests/unit/test_remote_dispatch.py +++ b/src/praisonai-train/tests/unit/test_remote_dispatch.py @@ -125,6 +125,34 @@ def test_a_non_local_dataset_is_not_shipped(self, tmp_path): _dispatch({"remote": {"host": "gpubox"}, "dataset": "org/dataset"}) assert FakeRunner.instances[0].started_with["dataset"] is None + def test_a_list_form_local_dataset_is_shipped(self, tmp_path): + # The trainer's canonical shape is a list of mappings, and a `name` + # that is a local file is loaded from disk. The string-only fallback + # missed it, so the file never reached the host and the run failed on a + # path that exists only here. + data = tmp_path / "d.json" + data.write_text("[]", encoding="utf-8") + _dispatch({"remote": {"host": "gpubox"}, + "dataset": [{"name": str(data)}]}) + sent = FakeRunner.instances[0].started_with["dataset"] + assert sent is not None and pathlib.Path(sent) == data + + def test_a_list_form_data_files_local_dataset_is_shipped(self, tmp_path): + # `data_files` names the local file explicitly, with `name` free to be + # a label. The trainer loads `data_files`, so it is what must ship. + data = tmp_path / "d.jsonl" + data.write_text("", encoding="utf-8") + _dispatch({"remote": {"host": "gpubox"}, + "dataset": [{"name": "my-set", "data_files": str(data)}]}) + sent = FakeRunner.instances[0].started_with["dataset"] + assert sent is not None and pathlib.Path(sent) == data + + def test_a_list_form_hub_dataset_is_not_shipped(self, tmp_path): + # A hub id in list form is not a local file; nothing to copy. + _dispatch({"remote": {"host": "gpubox"}, + "dataset": [{"name": "org/dataset"}]}) + assert FakeRunner.instances[0].started_with["dataset"] is None + def test_the_gpu_expectation_is_passed_through(self): _dispatch({"remote": {"host": "gpubox", "gpus": 4}}) assert FakeRunner.instances[0].started_with["gpus"] == 4