-
Notifications
You must be signed in to change notification settings - Fork 4.4k
surface specific GRPC errors more visibly #4930
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -440,6 +440,7 @@ UnityInputProto Exchange(UnityOutputProto unityOutput) | |
{ | ||
return null; | ||
} | ||
|
||
try | ||
{ | ||
var message = m_Client.Exchange(WrapMessage(unityOutput, 200)); | ||
|
@@ -455,8 +456,33 @@ UnityInputProto Exchange(UnityOutputProto unityOutput) | |
QuitCommandReceived?.Invoke(); | ||
return message.UnityInput; | ||
} | ||
catch | ||
catch (RpcException rpcException) | ||
{ | ||
// Log more verbose errors if they're something the user can possibly do something about. | ||
switch (rpcException.Status.StatusCode) | ||
{ | ||
case StatusCode.Unavailable: | ||
// This can happen when python disconnects. Ignore it to avoid noisy logs. | ||
break; | ||
case StatusCode.ResourceExhausted: | ||
// This happens is the message body is too large. There's no way to | ||
// gracefully handle this, but at least we can show the message and the | ||
// user can try to reduce the number of agents or observation sizes. | ||
Debug.LogError($"GRPC Exception: {rpcException.Message}. Disconnecting from trainer."); | ||
break; | ||
default: | ||
// Other unknown errors. Log at INFO level. | ||
Debug.Log($"GRPC Exception: {rpcException.Message}. Disconnecting from trainer."); | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could be convinced to log other exceptions here (maybe as Info instead of Error). That would at least help us for future unknown unknowns. |
||
} | ||
m_IsOpen = false; | ||
QuitCommandReceived?.Invoke(); | ||
return null; | ||
} | ||
catch (Exception ex) | ||
{ | ||
// Fall-through for other error types | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment about logging here - I don't think we ever hit it, so it would be nice to know when we do, but it's also potentially noisy to users. |
||
Debug.LogError($"GRPC Exception: {ex.Message}. Disconnecting from trainer."); | ||
m_IsOpen = false; | ||
QuitCommandReceived?.Invoke(); | ||
return null; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens roughly 20%-25% of the time when hitting Ctrl-C from python. Not exactly sure why it happens instead of the non-200 status (handled above).