Fixed bug #237 skylark cp does not warn user if object does not exist #248

ethanmehta · 2022-04-04T21:12:45Z

Fixes #237

In run_replication_plan method of ReplicatorClient, added assertion warning user that no matching objects were found with the specified source bucket and prefix. Tested with skylark cp method.

parasj · 2022-04-04T22:00:38Z

skylark/replicate/replicator_client.py

@@ -211,6 +211,7 @@ def run_replication_plan(self, job: ReplicationJob) -> ReplicationJob:
            "azure",
            "gcp",
        ], f"Only AWS, Azure, and GCP are supported, but got {job.dest_region}"
+        assert len(job.src_objs) > 0, f"Found {len(job.src_objs)} matching objects. Objects do not exist in src bucket or bucket is empty"


Some feedback: return errors to users by using an if statement then printing a readable error (e.g. logger.error), then raise typer.Abort()

parasj · 2022-04-04T22:01:36Z

@milesturin I added you as a reviewer since you raised the initial issue. Would this fix the original issue?

… method of s3_interface.py, if source bucket does not exist. bug #237

… of s3_interface.py, if source bucket does not exist. bug #237

milesturin · 2022-04-06T18:18:59Z

@ethanmehta unfortunately these do not fix the issue. Try running skylark cp on a nonexistent object, if the fix is successful it will report that the object does not exist to the user, if not it will print Downloading: 0.00B [00:00, ?B/s]

…loud to cloud with s3 source bucket

…if the source bucket or object do not exist, bug #237

ethanmehta · 2022-04-06T20:29:13Z

Thanks @milesturin. It should work now.

If a user uses skylark cp s3 local and the s3 bucket or object do not exist, skylark will warn and abort.

If a user uses skylark cp s3 s3 and the s3 source bucket or object do not exist, skylark will warn and abort.

Is this the desired behavior?

milesturin

@ethanmehta Seems to work well, I left a few small suggestions but other than that it looks good to me. @parasj I can only speak to the s3 interface stuff so take a quick glance and make sure it looks good for the replication stuff as well.

milesturin · 2022-04-12T20:39:52Z

skylark/cli/cli.py

@@ -126,6 +126,12 @@ def cp(
        # Set up replication topology
        if solve:
            objs = list(src_client.list_objects(path_src))
+            if not objs:
+                logger.warning(


I would change this to a logger.error since it causes program termination. Also, no need to make the argument an f string since we aren't inserting any data into it. Finally I'd say make it a one liner since the parameter is small.

Maybe make the error slightly more specific too, something like "Specified object does not exist."

milesturin · 2022-04-12T20:42:26Z

skylark/cli/cli_helper.py

        total_bytes = 0.0
        for obj in object_interface.list_objects(prefix=src_key):
            sub_key = obj.key[len(src_key) :]
            sub_key = sub_key.lstrip("/")
            dest_path = dst / sub_key
            total_bytes += _copy(obj, dest_path)
+            obj_count += 1
+
+        if not obj_count:


Same suggestions as before

milesturin · 2022-04-12T20:42:37Z

skylark/cli/cli_helper.py

@@ -230,6 +238,13 @@ def replicate_helper(
    else:
        # make replication job
        objs = list(ObjectStoreInterface.create(topo.source_region(), source_bucket).list_objects(src_key_prefix))
+        if not objs:


Same suggestions as before

milesturin · 2022-04-12T20:43:03Z

skylark/obj_store/s3_interface.py

@@ -23,6 +25,9 @@ def __init__(self, aws_region, bucket_name, use_tls=True, part_size=None, throug
        self.auth = AWSAuthentication()
        self.aws_region = self.infer_s3_region(bucket_name) if aws_region is None or aws_region == "infer" else aws_region
        self.bucket_name = bucket_name
+        if not self.bucket_exists():


Looks good, just change it to a logger.error

milesturin · 2022-04-12T20:43:13Z

skylark/obj_store/s3_interface.py

+            region = s3_client.get_bucket_location(Bucket=bucket_name).get("LocationConstraint", "us-east-1")
+            return region if region is not None else "us-east-1"
+        except:
+            logger.warn("Specified bucket does not exist.")


Same suggestion as before

milesturin · 2022-04-12T20:45:15Z

skylark/cli/cli_helper.py

+                f"Objects do not exist."
+            )
+            raise typer.Abort()
+            return 1


Remove this return, it'll never get run

milesturin · 2022-04-12T20:45:31Z

skylark/replicate/replicator_client.py

@@ -11,6 +11,7 @@
 from skylark.utils import logger
 from tqdm import tqdm
 import pandas as pd
+import typer


I don't think this gets used

parasj · 2022-04-15T00:24:34Z

skylark/obj_store/s3_interface.py

@@ -22,6 +24,9 @@ def __init__(self, aws_region, bucket_name, use_tls=True, part_size=None, throug
        self.auth = AWSAuthentication()
        self.aws_region = self.infer_s3_region(bucket_name) if aws_region is None or aws_region == "infer" else aws_region
        self.bucket_name = bucket_name
+        if not self.bucket_exists():
+            logger.warn("Specified bucket does not exist.")
+            raise typer.Abort()


@ethanmehta Can we raise an exception other than the typer ones? S3Interface is in the API so it shouldn't import this.

@ethanmehta check this page out, there are a number of exceptions regarding missing objects in botocore, might be able to find an applicable one in there.

There's a new exceptions.py file with cloud provider independent exceptions, so we should make a new exception class (e.g. MissingBucketException or MissingObjectException)

parasj · 2022-04-15T00:28:29Z

@ethanmehta I resolved a merge conflict that I introduced in main.

…b.com/parasj/skylark into dev/ethanmehta/bug-warn-objects-exist

….py. Skylark raises these instead of typer.Abort.

ethanmehta · 2022-04-16T21:25:41Z

Should be good to merge.

parasj

LGTM!

parasj · 2022-04-25T06:34:02Z

@ethanmehta Feel free to merge this PR after resolving merge conflicts!

Fixed bug #237 skylark cp does not warn user if object does not exist

2fbc4a7

parasj requested a review from milesturin April 4, 2022 22:01

parasj requested changes Apr 4, 2022

View reviewed changes

ethanmehta added 2 commits April 6, 2022 16:43

For aws, aborts with error message and typer.abort in infer_s3_region…

a12ba6e

… method of s3_interface.py, if source bucket does not exist. bug #237

For aws, aborts with error message and typer.abort in __init__ method…

65ea7a9

… of s3_interface.py, if source bucket does not exist. bug #237

ethanmehta added 2 commits April 6, 2022 19:49

Bug #237, skylark now warns and aborts if object does not exist for c…

c066b3d

…loud to cloud with s3 source bucket

For a cp between a s3 bucket and local, skylark now warns and aborts …

0d1efa8

…if the source bucket or object do not exist, bug #237

parasj assigned ethanmehta Apr 12, 2022

milesturin requested changes Apr 12, 2022

View reviewed changes

Merge branch 'main' into dev/ethanmehta/bug-warn-objects-exist

efeab18

parasj reviewed Apr 15, 2022

View reviewed changes

ethanmehta and others added 4 commits April 16, 2022 20:56

Incorporating changes from review

8d98701

Merge branch 'dev/ethanmehta/bug-warn-objects-exist' of https://githu…

0a35a76

…b.com/parasj/skylark into dev/ethanmehta/bug-warn-objects-exist

Added MissingBucketException and MissingObjectException in exceptions…

96d2763

….py. Skylark raises these instead of typer.Abort.

Merge branch 'main' into dev/ethanmehta/bug-warn-objects-exist

5fb359a

parasj approved these changes Apr 25, 2022

View reviewed changes

parasj added the bug Something isn't working label Apr 25, 2022

Merge branch 'main' into dev/ethanmehta/bug-warn-objects-exist

f1628c9

parasj added this to the Bugs milestone Apr 25, 2022

Merge branch 'main' into dev/ethanmehta/bug-warn-objects-exist

3a99ced

ethanmehta merged commit bdea2ea into main Apr 26, 2022

ethanmehta deleted the dev/ethanmehta/bug-warn-objects-exist branch April 26, 2022 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed bug #237 skylark cp does not warn user if object does not exist #248

Fixed bug #237 skylark cp does not warn user if object does not exist #248

ethanmehta commented Apr 4, 2022 •

edited by parasj

Loading

parasj Apr 4, 2022

parasj commented Apr 4, 2022

milesturin commented Apr 6, 2022

ethanmehta commented Apr 6, 2022

milesturin left a comment

milesturin Apr 12, 2022

milesturin Apr 12, 2022

milesturin Apr 12, 2022

milesturin Apr 12, 2022

milesturin Apr 12, 2022

milesturin Apr 12, 2022

milesturin Apr 12, 2022

milesturin Apr 12, 2022

parasj Apr 15, 2022

milesturin Apr 15, 2022

parasj Apr 15, 2022

parasj commented Apr 15, 2022

ethanmehta commented Apr 16, 2022

parasj left a comment

parasj commented Apr 25, 2022

Fixed bug #237 skylark cp does not warn user if object does not exist #248

Fixed bug #237 skylark cp does not warn user if object does not exist #248

Conversation

ethanmehta commented Apr 4, 2022 • edited by parasj Loading

Choose a reason for hiding this comment

parasj commented Apr 4, 2022

milesturin commented Apr 6, 2022

ethanmehta commented Apr 6, 2022

milesturin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasj commented Apr 15, 2022

ethanmehta commented Apr 16, 2022

parasj left a comment

Choose a reason for hiding this comment

parasj commented Apr 25, 2022

ethanmehta commented Apr 4, 2022 •

edited by parasj

Loading