diff --git a/docs/push.md b/docs/push.md index 3b5a460..8a974cd 100644 --- a/docs/push.md +++ b/docs/push.md @@ -137,6 +137,22 @@ For large datasets with thousands of files, a single `dvc push` can be slow due Parallel push distributes the work across multiple nodes, with each worker handling a disjoint, byte-balanced partition of the files. This avoids conflicts, and because no single worker is left holding all the large files, wall-clock time scales with the number of workers rather than with the biggest partition. +## Targets vs remotes + +A target names *data* to push -- a tracked path, a `.dvc` file, or a stage +name. The remote is chosen with `-r`, so a bare remote name is read as a +target and nothing matches it: + +```bash +$ dt push myremote +Error: No such target: 'myremote' (not a tracked path, a .dvc file, or a stage name) + +'myremote' is a configured remote, not a target. To push to it: + dt push -r myremote +``` + +With no targets at all, `dt push` pushes everything. + ## Example Suppose a project has remotes for cloud storage and HPC as follows: diff --git a/dt/__init__.py b/dt/__init__.py index ba35cfb..1dfeb6c 100644 --- a/dt/__init__.py +++ b/dt/__init__.py @@ -1,3 +1,3 @@ """DVC Tools - Convenient tools for working with DVC in HPC environments.""" -__version__ = "0.25.0" +__version__ = "0.25.1" diff --git a/dt/cache.py b/dt/cache.py index 758713f..0103141 100644 --- a/dt/cache.py +++ b/dt/cache.py @@ -367,6 +367,8 @@ def collect_hashes_for_targets( try: result = utils.collect_tracked_entries(targets=targets, push=False) + except utils.TargetNotFoundError as e: + raise CacheError(str(e)) except utils.DependencyError as e: raise CacheError(str(e)) diff --git a/dt/cli.py b/dt/cli.py index f4887f5..7b33f32 100644 --- a/dt/cli.py +++ b/dt/cli.py @@ -3708,7 +3708,7 @@ def push(ctx, workers, worker, manifest, remote, no_wait, dry, verbose): try: # Dry run mode: show what would be pushed if dry: - targets = [arg for arg in ctx.args if not arg.startswith('-')] + targets = push_mod.extract_targets(list(ctx.args)) manifest = push_mod.build_manifest( targets=targets if targets else None, remote=remote, @@ -3765,7 +3765,7 @@ def push(ctx, workers, worker, manifest, remote, no_wait, dry, verbose): # Parallel mode: submit qxub jobs if workers is not None: # Extract targets from extra args (non-option args) - targets = [arg for arg in ctx.args if not arg.startswith('-')] + targets = push_mod.extract_targets(list(ctx.args)) # Extract qxub-relevant options if any (could extend later) qxub_args = None @@ -3792,6 +3792,8 @@ def push(ctx, workers, worker, manifest, remote, no_wait, dry, verbose): # Simple mode: push to all project remotes # If remote specified, push to just that one + push_mod.check_targets(list(ctx.args)) + if remote: success, output = push_mod.push_to_remote(remote, ctx.args) status = "✓" if success else "✗" diff --git a/dt/errors.py b/dt/errors.py index 230b418..f1de641 100644 --- a/dt/errors.py +++ b/dt/errors.py @@ -140,6 +140,20 @@ class RemoteError(DTError): pass +class TargetNotFoundError(DTError): + """A target does not name anything DVC tracks. + + Raised instead of letting DVC's own collection exception escape, so a + mistyped (or misplaced) target reads as an error message rather than a + traceback. Carries the offending targets so callers can add their own + hint -- ``dt push`` suggests ``-r`` when the target is a remote name. + """ + + def __init__(self, message: str, targets=None): + super().__init__(message) + self.targets = list(targets or []) + + class StatusError(DTError): """Error during status operations.""" pass diff --git a/dt/push.py b/dt/push.py index 5306ef7..067c97e 100644 --- a/dt/push.py +++ b/dt/push.py @@ -209,6 +209,88 @@ def push_all(args: List[str]) -> List[Tuple[str, bool, str]]: return results +#: `dvc push` options that take a separate value. Their value is not a target, +#: so it has to be skipped when picking targets out of the passthrough args. +VALUE_OPTIONS = {'-j', '--jobs', '-r', '--remote', '--rev', '--remote-refresh'} + + +def extract_targets(args: List[str]) -> List[str]: + """Pick the targets out of args forwarded to `dvc push`. + + Anything option-shaped is dropped, as is the value of an option that takes + one -- otherwise `dt push --dry --jobs 8` reads 8 as a target. + """ + targets = [] + skip_next = False + for arg in args: + if skip_next: + skip_next = False + continue + if arg.startswith('-'): + skip_next = arg in VALUE_OPTIONS + continue + targets.append(arg) + return targets + + +def remote_names() -> Set[str]: + """Every remote name visible to this repo, at any config scope.""" + try: + from . import remote as remote_mod + return {name for name, _url, _default in remote_mod.list_remotes(project_only=False)} + except Exception: + return {name for name, _url in get_project_remotes()} + + +def explain_bad_target(message: str, targets: Optional[List[str]] = None) -> str: + """Add a hint to a "no such target" message. + + The common way to hit this is forgetting ``-r``: ``dt push myremote`` + reads ``myremote`` as data to push, not as the remote to push it to. Say + so when the target really is a remote name; otherwise just say what a + target is. + """ + named = [t for t in (targets or []) if t in remote_names()] + + if named: + hint = ( + f"\n\n'{named[0]}' is a configured remote, not a target. " + f"To push to it:\n" + f" dt push -r {named[0]}" + ) + else: + hint = ( + "\n\nA target names data to push -- a tracked path, a .dvc file, " + "or a stage name.\nRun `dt push` with no targets to push everything, " + "or `dt push -r NAME` to choose a remote." + ) + + return message + hint + + +def check_targets(args: List[str]) -> None: + """Reject a passthrough arg that names a remote rather than data. + + The simple push path shells out to `dvc push`, which would report the + same argument as a missing stage. Only a target that cannot resolve *and* + names a configured remote is rejected here, so nothing DVC would have + accepted is turned away. + + Raises: + PushError: If a target is a remote name. + """ + missing = utils.unresolvable_targets(extract_targets(args)) + named = [target for target in missing if target in remote_names()] + if not named: + return + + raise PushError(explain_bad_target( + f"No such target: {named[0]!r} " + f"(not a tracked path, a .dvc file, or a stage name)", + named, + )) + + # ============================================================================= # Parallel push infrastructure # ============================================================================= @@ -244,6 +326,8 @@ def build_manifest( # Use shared helper to collect tracked entries try: result = utils.collect_tracked_entries(targets=targets, remote=remote, push=True) + except utils.TargetNotFoundError as e: + raise PushError(explain_bad_target(str(e), e.targets)) except utils.DependencyError as e: raise PushError(str(e)) diff --git a/dt/utils.py b/dt/utils.py index 40eac4a..671052d 100644 --- a/dt/utils.py +++ b/dt/utils.py @@ -6,11 +6,12 @@ import csv import hashlib import os +import re import shutil import socket import subprocess from pathlib import Path -from typing import Any, Dict, Iterable, List, Optional, Tuple, Union +from typing import Any, Dict, Iterable, List, Optional, Set, Tuple, Union from dvc.repo import Repo from dvc_data.hashfile.hash import file_md5 as _dvc_file_md5 @@ -18,7 +19,28 @@ from dvc_data.hashfile.meta import Meta from dvc_data.hashfile.tree import Tree -from .errors import DependencyError, DVCFileError +from .errors import DependencyError, DVCFileError, TargetNotFoundError + +# DVC's exceptions for "that target does not name anything I track". Caught +# where we call DVC's collection directly, so a bad target is reported rather +# than raised as a traceback. +try: + from dvc.exceptions import ( + NoOutputOrStageError, + OutputNotFoundError, + PathMissingError, + ) + from dvc.stage.exceptions import StageFileDoesNotExistError, StageNotFound + + _TARGET_COLLECTION_ERRORS = ( + NoOutputOrStageError, + OutputNotFoundError, + PathMissingError, + StageFileDoesNotExistError, + StageNotFound, + ) +except ImportError: # pragma: no cover - DVC always provides these + _TARGET_COLLECTION_ERRORS = () # ============================================================================= @@ -226,6 +248,67 @@ def _get_dvc_file_metadata(path: str) -> Dict[str, Any]: return {'size': None, 'nfiles': None} +def _stage_names(dvc_yaml: Path = Path('dvc.yaml')) -> Set[str]: + """Stage names defined in a dvc.yaml, empty if there isn't one.""" + import yaml + + if not dvc_yaml.exists(): + return set() + try: + with open(dvc_yaml) as f: + data = yaml.safe_load(f) or {} + except (OSError, yaml.YAMLError): + return set() + stages = data.get('stages') + return set(stages) if isinstance(stages, dict) else set() + + +def unresolvable_targets(targets: Optional[List[str]]) -> List[str]: + """Targets that cannot name anything DVC tracks. + + A target is a path, a .dvc file, or a stage name; anything that is none of + those cannot resolve, whatever the repo state. Kept deliberately + permissive -- an out that is merely not checked out still has its .dvc + file, so it is not reported here. + """ + missing = [] + for target in targets or []: + path = Path(target) + if path.exists() or Path(f"{target}.dvc").exists(): + continue + # `dvc.yaml:stage` and `subdir/dvc.yaml:stage` name a stage by file. + head, sep, _tail = target.partition(':') + if sep and Path(head).exists(): + continue + if target in _stage_names(): + continue + # A path inside a tracked directory resolves through the directory's + # .dvc file, even when the directory is not checked out. + if any(Path(f"{parent}.dvc").exists() for parent in path.parents): + continue + missing.append(target) + return missing + + +def _target_not_found(exc: Exception, targets: Optional[List[str]]) -> TargetNotFoundError: + """Turn DVC's collection failure into a message that names the target.""" + # DVC's messages lead with the offending target in quotes; prefer that, + # since it reflects what DVC actually choked on. + match = re.match(r"'([^']+)'", str(exc)) + if match and match.group(1) in (targets or []): + named = [match.group(1)] + else: + named = unresolvable_targets(targets) or list(targets or []) + + quoted = ', '.join(repr(t) for t in named) + plural = 's' if len(named) > 1 else '' + return TargetNotFoundError( + f"No such target{plural}: {quoted} " + f"(not a tracked path, a .dvc file, or a stage name)", + targets=named, + ) + + def collect_tracked_entries( targets: Optional[List[str]] = None, remote: Optional[str] = None, @@ -251,6 +334,7 @@ def collect_tracked_entries( Raises: DependencyError: If DVC internals are not available + TargetNotFoundError: If a target does not name anything DVC tracks """ try: from dvc.repo.fetch import _collect_indexes @@ -259,19 +343,22 @@ def collect_tracked_entries( repo = Repo() - indexes = _collect_indexes( - repo, - targets=targets, - remote=remote, - all_branches=False, - with_deps=False, - all_tags=False, - recursive=False, - all_commits=False, - revs=None, - workspace=True, - push=push, - ) + try: + indexes = _collect_indexes( + repo, + targets=targets, + remote=remote, + all_branches=False, + with_deps=False, + all_tags=False, + recursive=False, + all_commits=False, + revs=None, + workspace=True, + push=push, + ) + except _TARGET_COLLECTION_ERRORS as e: + raise _target_not_found(e, targets) from e if not indexes: return { diff --git a/pyproject.toml b/pyproject.toml index 84b3126..727ea57 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "dvc-tools" -version = "0.25.0" +version = "0.25.1" description = "Convenient tools for working with DVC in HPC environments" readme = "README.md" requires-python = ">=3.8" diff --git a/tests/unit/test_errors.py b/tests/unit/test_errors.py index 9960d42..4ae619a 100644 --- a/tests/unit/test_errors.py +++ b/tests/unit/test_errors.py @@ -50,6 +50,7 @@ class TestExceptionHierarchy: errors.PushError, errors.RemoteError, errors.SummaryError, + errors.TargetNotFoundError, errors.TmpError, errors.WorktreeError, errors.AddError, @@ -80,6 +81,7 @@ def test_inherits_from_dterror(self, exception_class): errors.PushError, errors.RemoteError, errors.SummaryError, + errors.TargetNotFoundError, errors.TmpError, errors.WorktreeError, errors.AddError, @@ -111,6 +113,7 @@ def test_can_be_raised_and_caught(self, exception_class): errors.PushError, errors.RemoteError, errors.SummaryError, + errors.TargetNotFoundError, errors.TmpError, errors.WorktreeError, errors.AddError, @@ -220,3 +223,16 @@ def test_exception_chain(self): # Run with: pytest tests/test_errors.py -v + + +class TestTargetNotFoundError: + """TargetNotFoundError carries the offending targets.""" + + def test_stores_targets(self): + err = errors.TargetNotFoundError("No such target: 'x'", targets=["x"]) + assert err.targets == ["x"] + assert str(err) == "No such target: 'x'" + + def test_targets_default_to_empty(self): + err = errors.TargetNotFoundError("No such target") + assert err.targets == [] diff --git a/tests/unit/test_push.py b/tests/unit/test_push.py index ccc3506..6b0f534 100644 --- a/tests/unit/test_push.py +++ b/tests/unit/test_push.py @@ -18,8 +18,11 @@ push_partition, parallel_push, _missing_from_remote, + explain_bad_target, + extract_targets, + check_targets, ) -from dt.errors import PushError +from dt.errors import PushError, TargetNotFoundError # ============================================================================= @@ -497,3 +500,101 @@ def test_remote_incomplete_raises(self): stack.enter_context(p) with pytest.raises(PushError, match="1 file"): parallel_push(num_workers=2, wait=True) + + +# ============================================================================= +# Bad-target reporting tests +# ============================================================================= + +class TestExtractTargets: + """Tests for the extract_targets helper.""" + + def test_drops_options(self): + """Option-shaped args are not targets.""" + assert extract_targets(["--force", "data.txt"]) == ["data.txt"] + + def test_drops_option_values(self): + """The value of a value-taking option is not a target.""" + assert extract_targets(["--jobs", "8"]) == [] + assert extract_targets(["-r", "myremote", "data.txt"]) == ["data.txt"] + + def test_keeps_value_after_flag(self): + """A flag takes no value, so the next arg is still a target.""" + assert extract_targets(["--force", "8"]) == ["8"] + + def test_inline_option_value(self): + """--jobs=8 consumes its own value.""" + assert extract_targets(["--jobs=8", "data.txt"]) == ["data.txt"] + + +class TestExplainBadTarget: + """Tests for the explain_bad_target helper.""" + + def test_suggests_remote_flag_for_remote_name(self): + """A target that names a remote gets the -r suggestion.""" + with patch("dt.push.remote_names", return_value={"myremote"}): + msg = explain_bad_target("No such target: 'myremote'", ["myremote"]) + + assert "dt push -r myremote" in msg + assert "configured remote" in msg + + def test_explains_targets_otherwise(self): + """A target that is not a remote gets the generic explanation.""" + with patch("dt.push.remote_names", return_value={"myremote"}): + msg = explain_bad_target("No such target: 'typo.txt'", ["typo.txt"]) + + assert "-r NAME" in msg + assert "configured remote" not in msg + + def test_keeps_original_message(self): + """The original message is preserved.""" + with patch("dt.push.remote_names", return_value=set()): + msg = explain_bad_target("No such target: 'typo.txt'", ["typo.txt"]) + + assert msg.startswith("No such target: 'typo.txt'") + + +class TestCheckTargets: + """Tests for the check_targets pre-flight check.""" + + def test_rejects_remote_name_used_as_target(self, tmp_path, monkeypatch): + """`dt push myremote` is refused before shelling out to dvc.""" + monkeypatch.chdir(tmp_path) + with patch("dt.push.remote_names", return_value={"myremote"}): + with pytest.raises(PushError) as exc_info: + check_targets(["myremote"]) + + assert "dt push -r myremote" in str(exc_info.value) + + def test_allows_target_that_is_not_a_remote(self, tmp_path, monkeypatch): + """A bad target that is not a remote is left for dvc to report.""" + monkeypatch.chdir(tmp_path) + with patch("dt.push.remote_names", return_value={"myremote"}): + check_targets(["typo.txt"]) + + def test_allows_real_path_sharing_a_remote_name(self, tmp_path, monkeypatch): + """A real path is a target even if a remote shares its name.""" + monkeypatch.chdir(tmp_path) + (tmp_path / "myremote").write_text("data") + with patch("dt.push.remote_names", return_value={"myremote"}): + check_targets(["myremote"]) + + def test_ignores_option_values(self, tmp_path, monkeypatch): + """An option value that happens to name a remote is not a target.""" + monkeypatch.chdir(tmp_path) + with patch("dt.push.remote_names", return_value={"myremote"}): + check_targets(["-r", "myremote"]) + + +class TestBuildManifestBadTarget: + """build_manifest reports bad targets instead of raising DVC's error.""" + + def test_target_not_found_becomes_push_error_with_hint(self): + """A TargetNotFoundError is re-raised as a PushError with a hint.""" + err = TargetNotFoundError("No such target: 'myremote'", targets=["myremote"]) + with patch("dt.push.utils.collect_tracked_entries", side_effect=err): + with patch("dt.push.remote_names", return_value={"myremote"}): + with pytest.raises(PushError) as exc_info: + build_manifest(targets=["myremote"]) + + assert "dt push -r myremote" in str(exc_info.value) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 648d1cc..183a2f2 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -13,7 +13,7 @@ import pytest from dt import utils -from dt.errors import DependencyError, DVCFileError +from dt.errors import DependencyError, DVCFileError, TargetNotFoundError # ============================================================================= @@ -859,3 +859,88 @@ def test_summarises_a_long_list(self): [f'f{i}.txt' for i in range(25)], 'd', 'repo', action='import', ) assert '... and 15 more' in msg + + +class TestCollectTrackedEntriesBadTarget: + """A target DVC cannot resolve is an error message, not a traceback.""" + + def test_dvc_collection_error_becomes_target_not_found(self, tmp_path, monkeypatch): + """DVC's NoOutputOrStageError is converted, naming the target.""" + from dvc.exceptions import NoOutputOrStageError + + monkeypatch.chdir(tmp_path) + with patch.object(utils, 'Repo'): + with patch( + 'dvc.repo.fetch._collect_indexes', + side_effect=NoOutputOrStageError('myremote', 'dvc.yaml'), + create=True, + ): + with pytest.raises(TargetNotFoundError) as exc_info: + utils.collect_tracked_entries(targets=['myremote']) + + assert exc_info.value.targets == ['myremote'] + assert "'myremote'" in str(exc_info.value) + + def test_names_only_the_unresolvable_target(self, tmp_path, monkeypatch): + """A target that does exist is not blamed alongside the bad one.""" + from dvc.exceptions import OutputNotFoundError + + monkeypatch.chdir(tmp_path) + (tmp_path / 'data.txt').write_text('x') + with patch.object(utils, 'Repo'): + with patch( + 'dvc.repo.fetch._collect_indexes', + side_effect=OutputNotFoundError('typo.txt'), + create=True, + ): + with pytest.raises(TargetNotFoundError) as exc_info: + utils.collect_tracked_entries(targets=['data.txt', 'typo.txt']) + + assert exc_info.value.targets == ['typo.txt'] + + +class TestUnresolvableTargets: + """Which targets cannot possibly name tracked data.""" + + def test_existing_path_is_resolvable(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / 'data.txt').write_text('x') + assert utils.unresolvable_targets(['data.txt']) == [] + + def test_dvc_file_makes_a_missing_path_resolvable(self, tmp_path, monkeypatch): + """A not-checked-out out is still a target: its .dvc file is there.""" + monkeypatch.chdir(tmp_path) + (tmp_path / 'data.txt.dvc').write_text('outs: []\n') + assert utils.unresolvable_targets(['data.txt']) == [] + + def test_unknown_name_is_unresolvable(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + assert utils.unresolvable_targets(['myremote']) == ['myremote'] + + def test_stage_name_is_resolvable(self, tmp_path, monkeypatch): + """A stage in dvc.yaml is a target even though it is not a path.""" + monkeypatch.chdir(tmp_path) + (tmp_path / 'dvc.yaml').write_text('stages:\n train:\n cmd: echo hi\n') + assert utils.unresolvable_targets(['train']) == [] + + def test_stage_named_by_file_is_resolvable(self, tmp_path, monkeypatch): + """`dvc.yaml:stage` resolves via the file it names.""" + monkeypatch.chdir(tmp_path) + (tmp_path / 'dvc.yaml').write_text('stages:\n train:\n cmd: echo hi\n') + assert utils.unresolvable_targets(['dvc.yaml:train']) == [] + + def test_no_targets(self): + assert utils.unresolvable_targets(None) == [] + + +class TestUnresolvableTargetsInsideTrackedDir: + """A path under a tracked directory resolves via the directory's .dvc.""" + + def test_file_in_tracked_dir_not_checked_out(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + (tmp_path / 'dir.dvc').write_text('outs: []\n') + assert utils.unresolvable_targets(['dir/b.txt']) == [] + + def test_file_under_untracked_dir_is_unresolvable(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + assert utils.unresolvable_targets(['dir/b.txt']) == ['dir/b.txt']