From ddf187f85987a8bde41afa88a13495d1efbe1ea9 Mon Sep 17 00:00:00 2001 From: Rio Yu <52408936+rioyu123@users.noreply.github.com> Date: Thu, 27 Aug 2026 20:56:25 +0800 Subject: [PATCH] fix(cli): handle Windows config section paths Signed-off-by: Rio Yu <52408936+rioyu123@users.noreply.github.com> --- nemo_run/cli/_paths.py | 35 ++++++++ nemo_run/cli/api.py | 10 +-- nemo_run/cli/config.py | 16 ++-- nemo_run/cli/lazy.py | 13 ++- test/cli/test_api.py | 8 +- test/cli/test_config_paths.py | 154 ++++++++++++++++++++++++++++++++++ 6 files changed, 209 insertions(+), 27 deletions(-) create mode 100644 nemo_run/cli/_paths.py create mode 100644 test/cli/test_config_paths.py diff --git a/nemo_run/cli/_paths.py b/nemo_run/cli/_paths.py new file mode 100644 index 00000000..f851e8aa --- /dev/null +++ b/nemo_run/cli/_paths.py @@ -0,0 +1,35 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import ntpath +import os + + +def split_config_path(path: str | os.PathLike[str]) -> tuple[str, str | None]: + """Split ``path[:section]`` without treating a Windows drive as a section. + + The first colon after a drive or UNC prefix starts the optional section. As a + consequence, a one-letter drive-relative value such as ``a:model`` remains a path. + Path-like inputs are converted to strings without normalization. The section is an + empty string, rather than ``None``, when a delimiter is present without a value. + """ + raw_path = os.fspath(path) + drive, tail = ntpath.splitdrive(raw_path) + if not tail and raw_path[:2].replace("\\", "/") == "//": + # ntpath treats a two-component // path as a complete UNC drive, including + # a potential section suffix (for example, //tmp/config.yaml:model). + drive, tail = "", raw_path + file_path, separator, section = tail.partition(":") + return drive + file_path, section if separator else None diff --git a/nemo_run/cli/api.py b/nemo_run/cli/api.py index e05b921f..186620d4 100644 --- a/nemo_run/cli/api.py +++ b/nemo_run/cli/api.py @@ -60,7 +60,10 @@ from nemo_run.cli import devspace as devspace_cli from nemo_run.cli import experiment as experiment_cli +from nemo_run.cli._paths import split_config_path from nemo_run.cli.cli_parser import parse_cli_args, parse_factory +from nemo_run.cli.config import ConfigSerializer +from nemo_run.cli.lazy import LazyEntrypoint from nemo_run.config import ( Config, Partial, @@ -71,8 +74,6 @@ from nemo_run.core.execution import LocalExecutor, SkypilotExecutor, SlurmExecutor from nemo_run.core.execution.base import Executor from nemo_run.core.frontend.console.styles import BOX_STYLE, TABLE_STYLES -from nemo_run.cli.config import ConfigSerializer -from nemo_run.cli.lazy import LazyEntrypoint from nemo_run.run.experiment import Experiment from nemo_run.run.plugin import ExperimentPlugin as Plugin @@ -1732,10 +1733,7 @@ def _export_config(output_path: str, format: Optional[str] = None) -> None: try: # Handle section extraction from path - section = None - file_path = output_path - if ":" in output_path: - file_path, section = output_path.split(":", 1) + file_path, section = split_config_path(output_path) # Create appropriate section message for display section_msg = f" (section: {section})" if section else "" diff --git a/nemo_run/cli/config.py b/nemo_run/cli/config.py index 149ec0b4..1df404ce 100644 --- a/nemo_run/cli/config.py +++ b/nemo_run/cli/config.py @@ -20,6 +20,7 @@ import yaml from fiddle._src import config as config_lib +from nemo_run.cli._paths import split_config_path from nemo_run.core.serialization.yaml import YamlSerializer @@ -321,7 +322,8 @@ def dump_dict( """ from pathlib import Path - path = Path(output_path) + path_str, path_section = split_config_path(output_path) + path = Path(path_str) # Extract section if specified if section: @@ -330,15 +332,11 @@ def dump_dict( data = data[section] # Handle potential section specifier in output_path - if ":" in str(path): - # Split off any section specifier from the path - path_str, section = str(path).split(":", 1) - path = Path(path_str) - + if path_section is not None: # Extract the specified section from data - if section not in data: - raise KeyError(f"Section '{section}' not found in configuration") - data = data[section] + if path_section not in data: + raise KeyError(f"Section '{path_section}' not found in configuration") + data = data[path_section] # Determine format from explicit parameter or file extension if format: diff --git a/nemo_run/cli/lazy.py b/nemo_run/cli/lazy.py index deaf2ce3..5dd37b72 100644 --- a/nemo_run/cli/lazy.py +++ b/nemo_run/cli/lazy.py @@ -33,6 +33,7 @@ from fiddle.experimental import serialization from omegaconf import DictConfig, OmegaConf +from nemo_run.cli._paths import split_config_path from nemo_run.config import Partial if TYPE_CHECKING: @@ -761,9 +762,7 @@ def _is_config_file_path(path_str: str) -> bool: Returns: bool: True if the string appears to be a config file path, False otherwise """ - # Check if there's a section specifier - if ":" in path_str: - path_str = path_str.split(":", 1)[0] + path_str, _ = split_config_path(path_str) # Check for supported extensions SUPPORTED_EXTENSIONS = (".yaml", ".yml", ".json", ".toml") @@ -916,14 +915,14 @@ def load_config_from_path(path_with_syntax: str) -> Any: """ from nemo_run.cli.config import ConfigSerializer from omegaconf import OmegaConf - import os # Extract file path and optional section - section_match = re.match(r"^@([\w\./\\-]+)(?::(\w+))?$", path_with_syntax) - if not section_match: + if not path_with_syntax.startswith("@"): raise ValueError(f"Invalid config file format: {path_with_syntax}") - config_path, section = section_match.groups() + config_path, section = split_config_path(path_with_syntax[1:]) + if not config_path or (section is not None and not re.fullmatch(r"\w+", section)): + raise ValueError(f"Invalid config file format: {path_with_syntax}") # Validate the path exists if not os.path.exists(config_path): diff --git a/test/cli/test_api.py b/test/cli/test_api.py index da359c12..35d112a8 100644 --- a/test/cli/test_api.py +++ b/test/cli/test_api.py @@ -1407,7 +1407,7 @@ def test_export_error_handling(self, temp_dir): mock_console = Mock(spec=Console) - with pytest.raises(Exception): # Expecting FileNotFoundError or similar + with pytest.raises(FileNotFoundError) as exc_info: _serialize_configuration( config, to_yaml=str(non_existent_path), @@ -1416,12 +1416,10 @@ def test_export_error_handling(self, temp_dir): ) # Check that error message was printed - expected_error_msg = str( - FileNotFoundError(f"[Errno 2] No such file or directory: '{str(non_existent_path)}'") - ) mock_console.print.assert_called_with( - f"[bold red]Failed to export configuration to YAML:[/bold red] {expected_error_msg}" + f"[bold red]Failed to export configuration to YAML:[/bold red] {exc_info.value}" ) + assert exc_info.value.filename == str(non_existent_path) def test_export_no_format_error(self): from nemo_run.cli.api import _serialize_configuration diff --git a/test/cli/test_config_paths.py b/test/cli/test_config_paths.py new file mode 100644 index 00000000..34f995ef --- /dev/null +++ b/test/cli/test_config_paths.py @@ -0,0 +1,154 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from pathlib import Path, PurePosixPath +from types import SimpleNamespace +from unittest.mock import mock_open, patch + +import pytest +import yaml + +from nemo_run.cli._paths import split_config_path +from nemo_run.cli.api import _serialize_configuration +from nemo_run.cli.config import ConfigSerializer +from nemo_run.cli.lazy import _is_config_file_path, load_config_from_path + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("config.yaml", ("config.yaml", None)), + ("config.yaml:model", ("config.yaml", "model")), + ("./configs/model.yaml:model", ("./configs/model.yaml", "model")), + ("/configs/model.yaml:model", ("/configs/model.yaml", "model")), + ("//tmp/model.yaml", ("//tmp/model.yaml", None)), + ("//tmp/model.yaml:model", ("//tmp/model.yaml", "model")), + ("//mnt/configs/model.yaml:model", ("//mnt/configs/model.yaml", "model")), + (r"/\server/share/model.yaml:model", (r"/\server/share/model.yaml", "model")), + (r"C:\configs\model.yaml", (r"C:\configs\model.yaml", None)), + (r"C:\configs\model.yaml:model", (r"C:\configs\model.yaml", "model")), + ("C:/configs/model.yaml:model", ("C:/configs/model.yaml", "model")), + ("C:", ("C:", None)), + ("c:model.yaml", ("c:model.yaml", None)), + ( + r"\\server\share\model.yaml:model", + (r"\\server\share\model.yaml", "model"), + ), + (r"\\server\share", (r"\\server\share", None)), + ("config.yaml:", ("config.yaml", "")), + ("config.yaml:model:encoder", ("config.yaml", "model:encoder")), + ("a:model", ("a:model", None)), + ], +) +def test_split_config_path(value, expected): + assert split_config_path(value) == expected + + +@pytest.mark.parametrize("value", [Path("config.yaml"), PurePosixPath("configs/model.yaml")]) +def test_split_config_path_accepts_pathlike(value): + assert split_config_path(value) == (str(value), None) + + +@pytest.mark.parametrize( + "value", + [r"C:\configs\model.yaml", r"C:\configs\model.yaml:model", "C:/model.json:model"], +) +def test_is_config_file_path_accepts_windows_paths(value): + assert _is_config_file_path(value) + + +@pytest.mark.parametrize("value", [r"C:\configs\model.txt", r"C:\configs\model"]) +def test_is_config_file_path_rejects_unsupported_windows_paths(value): + assert not _is_config_file_path(value) + + +def test_load_config_from_windows_path_with_section(monkeypatch): + exist_checks = [] + seen = {} + + def fake_exists(path): + exist_checks.append(path) + return True + + def fake_load_dict(self, path): + seen["loaded"] = path + return {"model": {"hidden_size": 256}} + + # Replace this module's os binding so the Windows-shaped path remains testable on POSIX. + fake_os = SimpleNamespace(path=SimpleNamespace(exists=fake_exists)) + monkeypatch.setattr("nemo_run.cli.lazy.os", fake_os) + monkeypatch.setattr(ConfigSerializer, "load_dict", fake_load_dict) + + loaded = load_config_from_path(r"@C:\configs\model.yaml:model") + + assert exist_checks == [r"C:\configs\model.yaml"] + assert seen["loaded"] == r"C:\configs\model.yaml" + assert loaded.hidden_size == 256 + + +def test_load_config_from_path_accepts_spaces(tmp_path): + config_path = tmp_path / "model config.yaml" + config_path.write_text("hidden_size: 256") + + loaded = load_config_from_path(f"@{config_path}") + + assert loaded.hidden_size == 256 + + +@pytest.mark.parametrize("value", ["config.yaml", "@", "@config.yaml:bad-section"]) +def test_load_config_from_path_keeps_invalid_syntax_contract(value): + with pytest.raises(ValueError, match="Invalid config file format"): + load_config_from_path(value) + + +def test_dump_dict_preserves_windows_output_path(): + output_path = r"C:\configs\model.yaml" + + with patch("builtins.open", mock_open()) as mocked_open: + ConfigSerializer().dump_dict({"hidden_size": 256}, output_path) + + assert str(mocked_open.call_args.args[0]) == output_path + + +def test_dump_dict_extracts_section_after_windows_output_path(): + output_path = r"C:\configs\model.yaml:model" + + with patch("builtins.open", mock_open()) as mocked_open: + ConfigSerializer().dump_dict({"model": {"hidden_size": 256}}, output_path) + + assert str(mocked_open.call_args.args[0]) == r"C:\configs\model.yaml" + written = "".join(call.args[0] for call in mocked_open().write.call_args_list) + assert yaml.safe_load(written) == {"hidden_size": 256} + + +def test_serialize_configuration_preserves_windows_output_path(): + output_path = r"C:\configs\model.yaml" + config = object() + + with patch.object(ConfigSerializer, "dump") as dump: + _serialize_configuration(config, to_yaml=output_path) + + dump.assert_called_once_with(config, output_path) + + +def test_serialize_configuration_extracts_section_after_windows_output_path(): + output_path = r"C:\configs\model.yaml" + section = object() + config = type("ConfigWithModel", (), {"model": section})() + + with patch.object(ConfigSerializer, "dump") as dump: + _serialize_configuration(config, to_yaml=f"{output_path}:model") + + dump.assert_called_once_with(section, output_path)