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..cfc63d3dd 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,14 @@ 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 or _local_dataset_in(resolved) 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,11 +295,52 @@ 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 +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. @@ -370,11 +416,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..2db18f2c2 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, **_): @@ -105,6 +109,50 @@ 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_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 @@ -117,13 +165,67 @@ 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) + 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) @@ -157,7 +259,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" @@ -174,13 +277,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. - monkeypatch.setattr(train_cmd, "import_code_module", - lambda *_a, **_k: (_ for _ in ()).throw(ImportError("no")), - raising=False) - try: + import typer + + 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. + 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}, {}) == {}