[Exception] Add initial DeltaCatError support #290

Zyiqin-Miranda · 2024-04-08T23:18:28Z

This PR add initial DeltaCat exception support.
With this support:
DeltaCat exception can be easily identified by error code going forward.
Retryable DeltaCat error can be identified at task level and job level.

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite	Current: `785962d`	Previous: `d77a208`	Ratio
`deltacat/tests/compute/test_compact_partition_incremental.py::test_compact_partition_incremental[9-incremental-decimal-pk-multi-dup_V1]`	`1.13810172851053` iter/sec (`stddev: 0`)	`2.294583661883228` iter/sec (`stddev: 0`)	`2.02`
`deltacat/tests/compute/test_compact_partition_incremental.py::test_compact_partition_incremental[10-incremental-decimal-pk-partitionless_V2]`	`1.074566958595815` iter/sec (`stddev: 0`)	`2.253822151056081` iter/sec (`stddev: 0`)	`2.10`
`deltacat/tests/compute/test_compact_partition_incremental.py::test_compact_partition_incremental[12-incremental-decimal-single-hash-bucket_V2]`	`1.311349877966825` iter/sec (`stddev: 0`)	`2.6295975732771018` iter/sec (`stddev: 0`)	`2.01`

This comment was automatically generated by workflow using github-action-benchmark.

raghumdani

Looks good so far. I didn't see code resource increase in the subsequent retries if Ray worker dies or out of memory occurs.

deltacat/aws/constants.py

raghumdani · 2024-04-10T04:37:35Z

deltacat/compute/compactor_v2/utils/exception_handler.py

+
+
+@parametrized
+def handle_exception(func, task_id):


can we get task_id inside this function?

Any reason for not resolving this?

Did't get your original comment. Are you asking if we are able to pass task_id?

Instead of passing task_id to this function, can we fetch it inside this?

This handle_exception is used as a function annotation to all Ray remote tasks. Not sure how to fetch it inside?

Just call get_current_ray_task_id within this function :)

Oh I see what you mean, will update in next revision.

Any update on this PR?

deltacat/compute/compactor_v2/utils/exception_handler.py

deltacat/exceptions.py

deltacat/utils/pyarrow.py

raghumdani · 2024-04-10T18:41:16Z

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark. Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
Benchmark suite Current: 8529248 Previous: d77a208 Ratio
deltacat/tests/compute/test_compact_partition_incremental.py::test_compact_partition_incremental[1-incremental-pkstr-sknone-norcf_V1] 0.5710365395077868 iter/sec (stddev: 0) 2.2521535582165826 iter/sec (stddev: 0) 3.94

This comment was automatically generated by workflow using github-action-benchmark.

Can you look at this performance drop as well?

Zyiqin-Miranda · 2024-04-12T19:08:59Z

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark. Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
Benchmark suite Current: 8529248 Previous: d77a208 Ratio
deltacat/tests/compute/test_compact_partition_incremental.py::test_compact_partition_incremental[1-incremental-pkstr-sknone-norcf_V1] 0.5710365395077868 iter/sec (stddev: 0) 2.2521535582165826 iter/sec (stddev: 0) 3.94
This comment was automatically generated by workflow using github-action-benchmark.

Can you look at this performance drop as well?

The performance alert is gone after the address comment commit. Not consistent.

raghumdani

Can you reply on the comments you don't plan to address?

raghumdani · 2024-04-13T04:27:40Z

deltacat/aws/s3u.py

+            ) from e
+        raise DownloadTableError(
+            msg=f"Failed table download from: {s3_url}", s3_url=s3_url
+        ) from e
    except BaseException as e:


This is not resolved. Are you planning on resolving it?

deltacat/compute/compactor_v2/utils/exception_handler.py

raghumdani · 2024-04-13T04:33:39Z

deltacat/compute/compactor_v2/utils/exception_handler.py

+            task_id=task_id,
+        ) from e
+    else:
+        raise DependencyRayError()


Just re-raise Ray e? You are losing all the stack trace and exception messages by raising a custom error at this point. Moreover, you are bypassing Ray's default retries on system errors.

deltacat/compute/compactor_v2/utils/exception_handler.py

Co-authored-by: Raghavendra M Dani <draghave@amazon.com> Signed-off-by: pf <19919899+pfaraone@users.noreply.github.com>

raghumdani

Took a fresh look again. Please address few final comments.

deltacat/aws/constants.py

deltacat/compute/compactor_v2/steps/hash_bucket.py

raghumdani · 2024-06-06T05:10:56Z

deltacat/compute/compactor_v2/utils/exception_handler.py

+    ConnectionError,
+    TimeoutError,
+    DaftTransientError,
+    tenacity.RetryError,


I believe this should have DeltaCAT RetryableError only and probably tenacity.RetryError (unless you re-raise the wrapped error) as all other errors will be catched by exception handler and converted to RetryableError before Ray catches them.

deltacat/aws/constants.py

raghumdani · 2024-06-06T05:18:40Z

deltacat/compute/compactor_v2/utils/exception_handler.py

+    return task_id, node_ip
+
+
+def _handle_ray_error(e: Exception):


Suggested change

def _handle_ray_error(e: Exception):

def _handle_ray_error(e: RayError):

raghumdani · 2024-06-06T21:22:54Z

deltacat/tests/compute/compactor_v2/utils/test_exception_handler.py

+    def mock_raise_exception(self, exception_to_raise):
+        raise exception_to_raise
+
+    def test_exception_handler(self):


We need one test all top level branches atleast preferably the ones where we extract the wrapped exception and re-raise the error.

raghumdani · 2024-06-06T21:25:21Z

deltacat/compute/compactor_v2/utils/exception_handler.py

+            msg=f"Daft error occurred while executing task:{task_id} on node ip:{node_ip}.",
+            task_id=task_id,
+            node_ip=node_ip,
+        ) from e


are we ignoring if error doesn't fall into any of the above?

raghumdani · 2024-06-06T21:25:29Z

deltacat/compute/compactor_v2/utils/exception_handler.py

+        ) from e
+
+
+def _handle_daft_error(e: Exception):


Same here regarding typing.

raghumdani · 2024-06-06T21:28:21Z

deltacat/exceptions.py

+class DeltaCatErrorNames(str, Enum):
+
+    # Dependency Error
+    GENERAL_DEPENDENCY_ERROR = "GeneralDependencyError"


A description would be helpful regarding what each error means.

deltacat/compute/compactor_v2/utils/exception_handler.py

github-actions bot reviewed Apr 8, 2024

View reviewed changes

raghumdani reviewed Apr 10, 2024

View reviewed changes

Zyiqin-Miranda force-pushed the dev/exception branch from 9c7462b to 5478ad9 Compare April 12, 2024 00:14

raghumdani reviewed Apr 13, 2024

View reviewed changes

Zyiqin-Miranda force-pushed the dev/exception branch 5 times, most recently from 6bbaf98 to 3abf95a Compare June 5, 2024 05:17

Zyiqin-Miranda and others added 7 commits June 4, 2024 22:22

[Exception] Resolve conflicts

ad93847

address comments

8072545

using more type hints

67041f5

Update deltacat/compute/compactor_v2/utils/exception_handler.py

871e7ca

Co-authored-by: Raghavendra M Dani <draghave@amazon.com> Signed-off-by: pf <19919899+pfaraone@users.noreply.github.com>

Update deltacat/compute/compactor_v2/utils/exception_handler.py

b52d54e

Co-authored-by: Raghavendra M Dani <draghave@amazon.com> Signed-off-by: pf <19919899+pfaraone@users.noreply.github.com>

Update deltacat/compute/compactor_v2/utils/exception_handler.py

1759a1d

Co-authored-by: Raghavendra M Dani <draghave@amazon.com> Signed-off-by: pf <19919899+pfaraone@users.noreply.github.com>

created S3ErrorCodes str enum

a0b49c5

Zyiqin-Miranda force-pushed the dev/exception branch 2 times, most recently from ee5aa40 to 785962d Compare June 5, 2024 06:22

Resolving conflicts

b51b9ea

Zyiqin-Miranda force-pushed the dev/exception branch from 785962d to b51b9ea Compare June 5, 2024 06:59

raghumdani reviewed Jun 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Exception] Add initial DeltaCatError support #290

[Exception] Add initial DeltaCatError support #290

Zyiqin-Miranda commented Apr 8, 2024

github-actions bot left a comment •

edited

raghumdani left a comment

raghumdani Apr 10, 2024

raghumdani Apr 12, 2024

Zyiqin-Miranda Apr 12, 2024

raghumdani Apr 12, 2024

Zyiqin-Miranda Apr 12, 2024

raghumdani Apr 13, 2024

Zyiqin-Miranda Apr 15, 2024

raghumdani Apr 23, 2024

raghumdani commented Apr 10, 2024

⚠️ Performance Alert ⚠️

Zyiqin-Miranda commented Apr 12, 2024

⚠️ Performance Alert ⚠️

raghumdani left a comment

raghumdani Apr 13, 2024

raghumdani Apr 13, 2024

raghumdani left a comment

raghumdani Jun 6, 2024

raghumdani Jun 6, 2024

raghumdani Jun 6, 2024

raghumdani Jun 6, 2024

raghumdani Jun 6, 2024

raghumdani Jun 6, 2024

	def _handle_ray_error(e: Exception):
	def _handle_ray_error(e: RayError):

[Exception] Add initial DeltaCatError support #290

Are you sure you want to change the base?

[Exception] Add initial DeltaCatError support #290

Conversation

Zyiqin-Miranda commented Apr 8, 2024

github-actions bot left a comment • edited

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

raghumdani 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

raghumdani commented Apr 10, 2024

⚠️ Performance Alert ⚠️

Zyiqin-Miranda commented Apr 12, 2024

⚠️ Performance Alert ⚠️

raghumdani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghumdani 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

github-actions bot left a comment •

edited