diff --git a/src/dstack/_internal/cli/services/presets/export.py b/src/dstack/_internal/cli/services/presets/export.py index 213d01fa3..dece78940 100644 --- a/src/dstack/_internal/cli/services/presets/export.py +++ b/src/dstack/_internal/cli/services/presets/export.py @@ -6,6 +6,7 @@ from dstack._internal.cli.models.presets import VerifiedPreset from dstack._internal.cli.services.presets.build import service_configuration_to_yaml_dict from dstack._internal.core.errors import CLIError +from dstack._internal.core.models.files import FilePathMapping def export_preset( @@ -16,20 +17,26 @@ def export_preset( force: bool, ) -> list[Path]: """Writes the preset's service as a `type: service` configuration at - `destination` and copies the files it references next to it, keeping their - relative paths, so the result deploys with plain `dstack apply -f`. - Returns every path written.""" + `destination` and copies the files it references next to it, so the result + deploys with plain `dstack apply -f`. Returns every path written.""" service = preset.service.model_copy(deep=True) - copies: list[tuple[Path, Path]] = [] + stored: list[FilePathMapping] = [] + record_paths: list[Path] = [] for mapping in service.files: source = Path(mapping.local_path) # Loading resolved these against the preset directory; a file stored # outside it keeps its absolute path and needs no copy. if not source.is_relative_to(preset_dir): continue - relative = source.relative_to(preset_dir) - copies.append((source, destination.parent / relative)) - mapping.local_path = relative.as_posix() + stored.append(mapping) + record_paths.append(source.relative_to(preset_dir)) + exported_paths = [_without_record_prefix(path) for path in record_paths] + if _collides(exported_paths, record_paths, destination): + exported_paths = record_paths + copies: list[tuple[Path, Path]] = [] + for mapping, record_path, exported_path in zip(stored, record_paths, exported_paths): + copies.append((preset_dir / record_path, destination.parent / exported_path)) + mapping.local_path = exported_path.as_posix() written = [destination] + [target for _, target in copies] if not force: for target in written: @@ -46,3 +53,35 @@ def export_preset( target.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(source, target) return written + + +def _without_record_prefix(relative: Path) -> Path: + """The store keeps a service's files inside the session records it mirrors, + `service//` (final-service attempt) or `trials//`; the numbering is + internal, so the export keeps only the structure under it: + `service/2/patches/fix.patch` exports as `patches/fix.patch`.""" + if ( + len(relative.parts) > 2 + and relative.parts[0] in ("service", "trials") + and relative.parts[1].isdigit() + ): + return Path(*relative.parts[2:]) + return relative + + +def _collides(exported_paths: list[Path], record_paths: list[Path], destination: Path) -> bool: + """Whether dropping the record prefixes would land two different files on + one exported path (`service/1/patches/fix.patch` and + `service/2/patches/fix.patch` both become `patches/fix.patch`), or a file + on the configuration itself; the caller keeps the full record layout then. + Case-insensitive: on a case-insensitive filesystem `Fix.patch` and + `fix.patch` are one file, and a case-sensitive check would let one copy + silently overwrite the other.""" + record_by_export: dict[str, Path] = {} + for record_path, exported_path in zip(record_paths, exported_paths): + key = exported_path.as_posix().casefold() + if key == destination.name.casefold(): + return True + if record_by_export.setdefault(key, record_path) != record_path: + return True + return False diff --git a/src/tests/_internal/cli/services/presets/test_export.py b/src/tests/_internal/cli/services/presets/test_export.py index 88187a8dd..c42a1ffe8 100644 --- a/src/tests/_internal/cli/services/presets/test_export.py +++ b/src/tests/_internal/cli/services/presets/test_export.py @@ -32,21 +32,130 @@ def test_exports_a_deployable_service_configuration_with_its_files(self, tmp_pat force=False, ) + # The store-internal `service//` record prefix stays out of the + # exported layout. assert written == [ destination, - tmp_path / "deploy" / "service" / "1" / "patches" / "fix.patch", + tmp_path / "deploy" / "patches" / "fix.patch", ] data = yaml.safe_load(destination.read_text()) assert data["type"] == "service" # Relative to the configuration file, which is how `dstack apply` # resolves `files` paths. + assert data["files"] == [{"local_path": "patches/fix.patch", "path": "/patches/fix.patch"}] + assert (tmp_path / "deploy" / "patches" / "fix.patch").read_text() == "--- a\n+++ b\n" + assert ServiceConfiguration.model_validate(data).model is not None + + def test_exports_a_trial_record_file_without_its_record_prefix(self, tmp_path: Path): + store = PresetStore(tmp_path / "presets") + preset = get_preset() + preset.service.files = [ + FilePathMapping(local_path="trials/3/patches/fix.patch", path="/patches/fix.patch") + ] + preset_dir = store.save(preset).parent + (preset_dir / "trials" / "3" / "patches").mkdir(parents=True) + (preset_dir / "trials" / "3" / "patches" / "fix.patch").write_text("--- a\n+++ b\n") + destination = tmp_path / "deploy" / "qwen.dstack.yml" + + written = export_preset( + store.get(preset.id), + preset_dir=preset_dir, + destination=destination, + force=False, + ) + + assert written == [destination, tmp_path / "deploy" / "patches" / "fix.patch"] + data = yaml.safe_load(destination.read_text()) + assert data["files"] == [{"local_path": "patches/fix.patch", "path": "/patches/fix.patch"}] + + def test_keeps_record_paths_when_flattening_would_collide(self, tmp_path: Path): + store = PresetStore(tmp_path / "presets") + preset = get_preset() + preset.service.files = [ + FilePathMapping(local_path="service/1/patches/fix.patch", path="/patches/a.patch"), + FilePathMapping(local_path="service/2/patches/fix.patch", path="/patches/b.patch"), + ] + preset_dir = store.save(preset).parent + for attempt, content in (("1", "one\n"), ("2", "two\n")): + (preset_dir / "service" / attempt / "patches").mkdir(parents=True) + (preset_dir / "service" / attempt / "patches" / "fix.patch").write_text(content) + destination = tmp_path / "deploy" / "qwen.dstack.yml" + + written = export_preset( + store.get(preset.id), + preset_dir=preset_dir, + destination=destination, + force=False, + ) + + assert written == [ + destination, + tmp_path / "deploy" / "service" / "1" / "patches" / "fix.patch", + tmp_path / "deploy" / "service" / "2" / "patches" / "fix.patch", + ] + data = yaml.safe_load(destination.read_text()) assert data["files"] == [ - {"local_path": "service/1/patches/fix.patch", "path": "/patches/fix.patch"} + {"local_path": "service/1/patches/fix.patch", "path": "/patches/a.patch"}, + {"local_path": "service/2/patches/fix.patch", "path": "/patches/b.patch"}, ] assert (tmp_path / "deploy" / "service" / "1" / "patches" / "fix.patch").read_text() == ( - "--- a\n+++ b\n" + "one\n" ) - assert ServiceConfiguration.model_validate(data).model is not None + assert (tmp_path / "deploy" / "service" / "2" / "patches" / "fix.patch").read_text() == ( + "two\n" + ) + + def test_treats_paths_differing_only_in_case_as_a_collision(self, tmp_path: Path): + store = PresetStore(tmp_path / "presets") + preset = get_preset() + preset.service.files = [ + FilePathMapping(local_path="service/1/patches/Fix.patch", path="/patches/a.patch"), + FilePathMapping(local_path="service/2/patches/fix.patch", path="/patches/b.patch"), + ] + preset_dir = store.save(preset).parent + for attempt, name in (("1", "Fix.patch"), ("2", "fix.patch")): + (preset_dir / "service" / attempt / "patches").mkdir(parents=True) + (preset_dir / "service" / attempt / "patches" / name).write_text(name) + destination = tmp_path / "deploy" / "qwen.dstack.yml" + + export_preset( + store.get(preset.id), + preset_dir=preset_dir, + destination=destination, + force=False, + ) + + data = yaml.safe_load(destination.read_text()) + assert data["files"] == [ + {"local_path": "service/1/patches/Fix.patch", "path": "/patches/a.patch"}, + {"local_path": "service/2/patches/fix.patch", "path": "/patches/b.patch"}, + ] + + def test_keeps_record_paths_when_a_file_would_land_on_the_configuration(self, tmp_path: Path): + store = PresetStore(tmp_path / "presets") + preset = get_preset() + preset.service.files = [ + FilePathMapping(local_path="service/1/qwen.dstack.yml", path="/extra/qwen.dstack.yml") + ] + preset_dir = store.save(preset).parent + (preset_dir / "service" / "1").mkdir(parents=True) + (preset_dir / "service" / "1" / "qwen.dstack.yml").write_text("extra\n") + destination = tmp_path / "deploy" / "qwen.dstack.yml" + + written = export_preset( + store.get(preset.id), + preset_dir=preset_dir, + destination=destination, + force=False, + ) + + assert written == [destination, tmp_path / "deploy" / "service" / "1" / "qwen.dstack.yml"] + data = yaml.safe_load(destination.read_text()) + assert data["type"] == "service" + assert data["files"] == [ + {"local_path": "service/1/qwen.dstack.yml", "path": "/extra/qwen.dstack.yml"} + ] + assert (tmp_path / "deploy" / "service" / "1" / "qwen.dstack.yml").read_text() == "extra\n" def test_refuses_to_overwrite_without_force(self, tmp_path: Path): store = PresetStore(tmp_path / "presets")