Merge pull request #316 from latchbio/ayush/cp-error-messaging

CHANGELOG.md

@@ -22,6 +22,10 @@ Types of changes
 
 * Added ability to get a pandas Dataframe from a registry table.
 
+### Changed
+
+* Better error messaging for both `latch cp` and `latch mv`.
+
 ## 2.36.3 - 2023-10-25
 
 ### Fixed

latch_cli/main.py

@@ -55,7 +55,7 @@ def main():
             """).strip("\n"),
             fg="red",
         )
-        raise click.exceptions.Exit() from e
+        raise click.exceptions.Exit(1) from e
 
     local_ver = parse_version(get_local_package_version())
     latest_ver = parse_version(get_latest_package_version())

latch_cli/services/cp/download.py

@@ -39,6 +39,14 @@ def download(
     dest: Path,
     config: CPConfig,
 ):
+    if not dest.parent.exists():
+        click.secho(
+            f"Invalid copy destination {dest}. Parent directory {dest.parent} does not"
+            " exist.",
+            fg="red",
+        )
+        raise click.exceptions.Exit(1)
+
     normalized = normalize_path(src)
     data = get_node_data(src)
 
@@ -63,10 +71,12 @@ def download(
     )
 
     if res.status_code != 200:
-        raise ValueError(
+        click.secho(
             f"failed to fetch presigned url(s) for path {src} with code"
-            f" {res.status_code}: {res.json()['error']}"
+            f" {res.status_code}: {res.json()['error']}",
+            fg="red",
         )
+        raise click.exceptions.Exit(1)
 
     json_data = res.json()
     if can_have_children:
@@ -78,9 +88,11 @@ def download(
         try:
             dest.mkdir(exist_ok=True)
         except FileNotFoundError as e:
-            raise ValueError(f"No such download destination {dest}") from e
-        except FileExistsError as e:
-            raise ValueError(f"Download destination {dest} is not a directory") from e
+            click.secho(f"No such download destination {dest}", fg="red")
+            raise click.exceptions.Exit(1) from e
+        except (FileExistsError, NotADirectoryError) as e:
+            click.secho(f"Download destination {dest} is not a directory", fg="red")
+            raise click.exceptions.Exit(1) from e
 
         unconfirmed_jobs: List[DownloadJob] = []
         confirmed_jobs: List[DownloadJob] = []

latch_cli/services/cp/ldata_utils.py

@@ -2,6 +2,8 @@
 from enum import Enum
 from typing import Dict, List, TypedDict
 
+import click
+
 try:
     from functools import cache
 except ImportError:
@@ -142,7 +144,8 @@ def get_node_data(
                 is_parent=is_parent,
             )
         except (TypeError, ValueError) as e:
-            raise get_path_error(remote_path, "not found", acc_id) from e
+            click.echo(get_path_error(remote_path, "not found", acc_id))
+            raise click.exceptions.Exit(1) from e
 
     return GetNodeDataResult(acc_id, ret)
 

latch_cli/services/cp/main.py

@@ -1,6 +1,9 @@
 from pathlib import Path
+from textwrap import dedent
 from typing import List
 
+import click
+
 from latch_cli.services.cp.config import CPConfig, Progress
 from latch_cli.services.cp.download import download
 from latch_cli.services.cp.glob import expand_pattern
@@ -41,8 +44,12 @@ def cp(
             else:
                 remote_copy(src, dest)
         else:
-            raise ValueError(
-                f"`latch cp` cannot be used for purely local file copying. Please make"
-                f" sure one or both of your paths is a remote path (beginning with"
-                f" `latch://`)"
+            click.secho(
+                dedent(f"""
+                `latch cp` cannot be used for purely local file copying.
+
+                Please ensure at least one of your arguments is a remote path (beginning with `latch://`)
+                """).strip("\n"),
+                fg="red",
             )
+            raise click.exceptions.Exit(1)

latch_cli/services/cp/remote_copy.py

@@ -23,13 +23,15 @@ def remote_copy(
     path_by_id = {v.id: k for k, v in node_data.data.items()}
 
     if src_data.is_parent:
-        raise get_path_error(src, "not found", acc_id)
+        click.echo(get_path_error(src, "not found", acc_id))
+        raise click.exceptions.Exit(1)
 
     new_name = None
     if dest_data.is_parent:
         new_name = get_name_from_path(dest)
     elif dest_data.type in {LDataNodeType.obj, LDataNodeType.link}:
-        raise get_path_error(dest, "object already exists at path.", acc_id)
+        click.echo(get_path_error(dest, "object already exists at path.", acc_id))
+        raise click.exceptions.Exit(1)
 
     try:
         execute(
@@ -57,36 +59,46 @@ def remote_copy(
         )
     except TransportQueryError as e:
         if e.errors is None or len(e.errors) == 0:
-            raise e
+            click.echo(get_path_error(src, str(e), acc_id))
+            raise click.exceptions.Exit(1) from e
 
         msg: str = e.errors[0]["message"]
 
         if msg.startswith("Permission denied on node"):
             node_id = msg.rsplit(" ", 1)[1]
             path = path_by_id[node_id]
 
-            raise get_path_error(path, "permission denied.", acc_id) from e
+            click.echo(get_path_error(path, "permission denied.", acc_id))
+            raise click.exceptions.Exit(1) from e
         elif msg == "Refusing to make node its own parent":
-            raise get_path_error(dest, f"is a parent of {src}.", acc_id) from e
+            click.echo(get_path_error(dest, f"is a parent of {src}.", acc_id))
+            raise click.exceptions.Exit(1) from e
         elif msg == "Refusing to parent node to an object node":
-            raise get_path_error(dest, f"object exists at path.", acc_id) from e
+            click.echo(get_path_error(dest, f"object exists at path.", acc_id))
+            raise click.exceptions.Exit(1) from e
         elif msg == "Refusing to move a share link (or into a share link)":
             if src_data.type is LDataNodeType.link:
                 path = src
             else:
                 path = dest
 
-            raise get_path_error(path, f"is a share link.", acc_id) from e
+            click.echo(get_path_error(path, f"is a share link.", acc_id))
+            raise click.exceptions.Exit(1) from e
         elif msg.startswith("Refusing to copy account root"):
-            raise get_path_error(src, "is an account root.", acc_id) from e
+            click.echo(get_path_error(src, "is an account root.", acc_id))
+            raise click.exceptions.Exit(1) from e
         elif msg.startswith("Refusing to copy removed node"):
-            raise get_path_error(src, "not found.", acc_id) from e
+            click.echo(get_path_error(src, "not found.", acc_id))
+            raise click.exceptions.Exit(1) from e
         elif msg.startswith("Refusing to copy already in-transit node"):
-            raise get_path_error(src, "copy already in progress.", acc_id) from e
+            click.echo(get_path_error(src, "copy already in progress.", acc_id))
+            raise click.exceptions.Exit(1) from e
         elif msg == "Conflicting object in destination":
-            raise get_path_error(dest, "object exists at path.", acc_id) from e
+            click.echo(get_path_error(dest, "object exists at path.", acc_id))
+            raise click.exceptions.Exit(1) from e
 
-        raise e
+        click.echo(get_path_error(src, str(e), acc_id))
+        raise click.exceptions.Exit(1) from e
 
     click.echo(f"""
 {click.style("Copy Requested.", fg="green")}

latch_cli/services/cp/upload.py

@@ -56,7 +56,8 @@ def upload(
 ):
     src_path = Path(src)
     if not src_path.exists():
-        raise ValueError(f"Could not find {src_path}.")
+        click.secho(f"Could not find {src_path}: no such file or directory.", fg="red")
+        raise click.exceptions.Exit(1)
 
     if config.progress != Progress.none:
         click.secho(f"Uploading {src_path.name}", fg="blue")
@@ -75,9 +76,11 @@ def upload(
 
     if not dest_is_dir:
         if not dest_exists:  # path is latch:///a/b/file_1/file_2
-            raise ValueError(f"No such file or directory: {dest}")
+            click.secho(f"No such file or directory: {dest}", fg="red")
+            raise click.exceptions.Exit(1)
         if src_path.is_dir():
-            raise ValueError(f"{normalized} is not a directory.")
+            click.secho(f"{normalized} is not a directory.", fg="red")
+            raise click.exceptions.Exit(1)
 
     if config.progress == Progress.none:
         num_bars = 0
@@ -287,85 +290,92 @@ def start_upload(
     throttle: Optional[Throttle] = None,
     latency_q: Optional["LatencyQueueType"] = None,
 ) -> Optional[StartUploadReturnType]:
-    try:
-        if not src.exists():
-            raise ValueError(f"Could not find {src}: no such file or link")
-
-        if src.is_symlink():
-            src = src.resolve()
-
-        content_type, _ = mimetypes.guess_type(src)
-        if content_type is None:
-            with open(src, "rb") as f:
-                sample = f.read(units.KiB)
-
-            try:
-                sample.decode()
-                content_type = "text/plain"
-            except UnicodeDecodeError:
-                content_type = "application/octet-stream"
-
-        file_size = src.stat().st_size
-        if file_size > latch_constants.maximum_upload_size:
-            raise ValueError(
-                f"File is {with_si_suffix(file_size)} which exceeds the maximum upload"
-                " size (5TiB)"
-            )
-
-        part_count = min(
-            latch_constants.maximum_upload_parts,
-            math.ceil(file_size / latch_constants.file_chunk_size),
-        )
-        part_size = max(
-            latch_constants.file_chunk_size,
-            math.ceil(file_size / latch_constants.maximum_upload_parts),
+    if not src.exists():
+        raise click.exceptions.Exit(
+            click.style(f"Could not find {src}: no such file or link", fg="red")
         )
 
-        retries = 0
-        while retries < MAX_RETRIES:
-            if throttle is not None:
-                time.sleep(throttle.get_delay() * math.exp(retries * 1 / 2))
-
-            start = time.monotonic()
-            res = tinyrequests.post(
-                latch_config.api.data.start_upload,
-                headers={"Authorization": get_auth_header()},
-                json={
-                    "path": dest,
-                    "content_type": content_type,
-                    "part_count": part_count,
-                },
+    if src.is_symlink():
+        src = src.resolve()
+
+    content_type, _ = mimetypes.guess_type(src)
+    if content_type is None:
+        with open(src, "rb") as f:
+            sample = f.read(units.KiB)
+
+        try:
+            sample.decode()
+            content_type = "text/plain"
+        except UnicodeDecodeError:
+            content_type = "application/octet-stream"
+
+    file_size = src.stat().st_size
+    if file_size > latch_constants.maximum_upload_size:
+        raise click.exceptions.Exit(
+            click.style(
+                f"File is {with_si_suffix(file_size)} which exceeds the maximum"
+                " upload size (5TiB)",
+                fg="red",
             )
-            end = time.monotonic()
+        )
 
-            if latency_q is not None:
-                latency_q.put(end - start)
+    part_count = min(
+        latch_constants.maximum_upload_parts,
+        math.ceil(file_size / latch_constants.file_chunk_size),
+    )
+    part_size = max(
+        latch_constants.file_chunk_size,
+        math.ceil(file_size / latch_constants.maximum_upload_parts),
+    )
 
-            try:
-                json_data = res.json()
-            except JSONDecodeError:
-                retries += 1
-                continue
+    retries = 0
+    while retries < MAX_RETRIES:
+        if throttle is not None:
+            time.sleep(throttle.get_delay() * math.exp(retries * 1 / 2))
+
+        start = time.monotonic()
+        res = tinyrequests.post(
+            latch_config.api.data.start_upload,
+            headers={"Authorization": get_auth_header()},
+            json={
+                "path": dest,
+                "content_type": content_type,
+                "part_count": part_count,
+            },
+        )
+        end = time.monotonic()
 
-            if res.status_code != 200:
-                retries += 1
-                continue
+        if latency_q is not None:
+            latency_q.put(end - start)
 
-            if progress_bars is not None:
-                progress_bars.update_total_progress(1)
+        try:
+            json_data = res.json()
+        except JSONDecodeError:
+            retries += 1
+            continue
 
-            if "version_id" in json_data["data"]:
-                return  # file is empty, so no need to upload any content
+        if res.status_code != 200:
+            retries += 1
+            continue
 
-            data: StartUploadData = json_data["data"]
+        if progress_bars is not None:
+            progress_bars.update_total_progress(1)
 
-            return StartUploadReturnType(
-                **data, part_count=part_count, part_size=part_size, src=src, dest=dest
-            )
+        if "version_id" in json_data["data"]:
+            return  # file is empty, so no need to upload any content
 
-        raise ValueError(f"Unable to generate upload URL for {src}")
-    except Exception as e:
-        raise e.__class__(f"{src}", *e.args)
+        data: StartUploadData = json_data["data"]
+
+        return StartUploadReturnType(
+            **data, part_count=part_count, part_size=part_size, src=src, dest=dest
+        )
+
+    raise click.exceptions.Exit(
+        click.style(
+            f"Unable to generate upload URL for {src}",
+            fg="red",
+        )
+    )
 
 
 @dataclass(frozen=True)
@@ -394,7 +404,9 @@ def upload_file_chunk(
 
     res = tinyrequests.put(url, data=data)
     if res.status_code != 200:
-        raise RuntimeError(f"Failed to upload part {part_index} of {src}")
+        raise click.exceptions.Exit(
+            click.style(f"Failed to upload part {part_index} of {src}", fg="red")
+        )
 
     ret = CompletedPart(
         src=src,
@@ -451,7 +463,11 @@ def end_upload(
     )
 
     if res.status_code != 200:
-        raise RuntimeError(f"Unable to complete file upload: {res.json()['error']}")
+        raise click.exceptions.Exit(
+            click.style(
+                f"Unable to complete file upload: {res.json()['error']}", fg="red"
+            )
+        )
 
     if progress_bars is not None:
         progress_bars.update_total_progress(1)