Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Handle gRPC error explicitly #602

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Handle gRPC error explicitly #602

merged 1 commit into from
Aug 11, 2023

Conversation

iaroslav-ciupin
Copy link
Contributor

@iaroslav-ciupin iaroslav-ciupin commented Aug 10, 2023

TL;DR

gRPC errors should NOT be wrapped into codes.Internal, but converted to FlyteAdminError with same status code and message.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #602 (2c363ec) into master (86c00cc) will increase coverage by 1.62%.
Report is 1 commits behind head on master.
The diff coverage is 81.81%.

❗ Current head 2c363ec differs from pull request most recent head 53764a6. Consider uploading reports for the commit 53764a6 to get more accurate results

@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
+ Coverage   58.66%   60.29%   +1.62%     
==========================================
  Files         171      171              
  Lines       16464    13462    -3002     
==========================================
- Hits         9659     8117    -1542     
+ Misses       5954     4493    -1461     
- Partials      851      852       +1     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
plugins/registry.go 100.00% <ø> (ø)
auth/handlers.go 35.79% <62.50%> (+0.68%) ⬆️
pkg/rpc/adminservice/util/transformers.go 100.00% <100.00%> (ø)

... and 154 files with indirect coverage changes

Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, when have you observed flyteadmin passing along raw gRPC errors?

@iaroslav-ciupin
Copy link
Contributor Author

@katrogan when I execute pyflyte run ... and get a validation error back(like no node can satisfy task resource...), I actual get back

code = codes.Internal
message = code:codes.InvalidArgument message: no node can satisfy task resource...

and same error gets flytepropeller(saw that in logs). Basically InvalidArgument was wrapped into Internal

@katrogan katrogan merged commit d492640 into master Aug 11, 2023
14 checks passed
@katrogan katrogan deleted the improve-error-handling branch August 11, 2023 19:52
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Signed-off-by: Iaroslav Ciupin <iaroslav@union.ai>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants