Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HandleRpcError: Add RpcError handling in tgis_utils #355

Merged
merged 2 commits into from
May 14, 2024

Conversation

gabe-l-hart
Copy link
Contributor

Description

This PR adds error handling for grpc.RpcErrors raised during calls to TGIS in tgis_utils.

Closes: #354

grpc.StatusCode.RESOURCE_EXHAUSTED: CaikitCoreStatusCode.UNKNOWN,
grpc.StatusCode.FAILED_PRECONDITION: CaikitCoreStatusCode.UNKNOWN,
grpc.StatusCode.ABORTED: CaikitCoreStatusCode.CONNECTION_ERROR,
grpc.StatusCode.OUT_OF_RANGE: CaikitCoreStatusCode.UNKNOWN,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Conversions are lossy, but kind of make sense from changing the view of runtime to library.... For some of these, like the OUT_OF_RANGE, might make sense to expose at library level as well ? Or may be we can map it to FAILED_PRE_CONDITION since there is a overlap as per doc here

Also the FAILED_PRECONDITION might come in handy, which is getting mapped to UNKNOWN above 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was trying to make this work without changing caikit itself. I think there is almost certainly an important API-breaking change conversation to be had about how the internal error codes map to well-known error code enums (grpc/http) and how to avoid information loss.

Copy link
Collaborator

@gkumbhat gkumbhat left a comment

Choose a reason for hiding this comment

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

Left a small mapping suggestion / question. Otherwise looks good to me!

Copy link
Collaborator

@gkumbhat gkumbhat left a comment

Choose a reason for hiding this comment

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

LGTM. We do require some more iteration w.r.t the error codes, but some changes need to be made on caikit side to include those codes. So for now, we can get this in and iterate on error codes..

However, iterating on error codes would be API breaking. 🚨

@gabe-l-hart gabe-l-hart force-pushed the HandleRpcError-354 branch from 129acb8 to b01c0e4 Compare May 14, 2024 17:30
Copy link
Collaborator

@gkumbhat gkumbhat left a comment

Choose a reason for hiding this comment

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

LGTM. Per the discussion, we will modify and add error code mapping in caikit itself in future, which would allow us to remove the mapping from caikit-nlp

@gkumbhat gkumbhat merged commit a5bf25e into caikit:main May 14, 2024
5 checks passed
@gabe-l-hart gabe-l-hart deleted the HandleRpcError-354 branch May 14, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not surface grpc.RpcError from downstream calls to TGIS
2 participants