ErrorHandling: make GetComputationClient() return StatusOr<T> type. #9384
ErrorHandling: make GetComputationClient() return StatusOr<T> type. #9384
GetComputationClient() return StatusOr<T> type. #9384Conversation
ghpvnist
left a comment
There was a problem hiding this comment.
Thanks for the change, added a few comments.
391d353 to
3344bb0
Compare
|
Incorporated the changes suggested by @zhanyong-wan comment. |
ghpvnist
left a comment
There was a problem hiding this comment.
Can you create a unit test for the new API? runtime_test.cpp in the same directory could work.
|
@ghpvnist What kind of tests did you have in mind? |
This directory has precedent of tests within the same directory. I prefer to have tests close where the files are stored that way, future refactoring changes would be easier. Wdyt? @zhanyong-wan
I was thinking the typical unit tests for test coverage purposes. Something along the lines of:
|
a351a69 to
7e0da02
Compare
While I prefer to have tests in the same directory as the code too, pytorch uses the |
|
@zhanyong-wan @ghpvnist Let me know if there's anything else you'd like to see in this PR. |
|
Hi @ysiraichi , I'll take a look soon. In the future, when the PR is ready for another review, could you click on the spinning-arrows icon next to the reviewer name? That'll let us know that you are done with editing and it's our turn. Thanks! |
|
Ah, ok. Got it. I don't think I ever noticed that feature. Thanks for the tip. |
Ref: #9339
This PR creates a new version of
GetComputationClient()that returns aStatusOr<T>type for better error handling. Since this function is frequently used in the codebase, its old version remains, but annotated withABSL_DEPRECATED()(following #9381 example) and renamed toGetComputationClientOrDie(). The new version is created with the old name. Summarizing:GetComputationClient()is renamed toGetComputationClientOrDie()torch_xla::runtime::status::GetComputationClient()function that returnsStatusOr<T>typeXLA_CHECK()for throwing an error on non-ok status, maintaining its old behaviorConsumeAndMaybeThrow()function for throwing errors if non-ok status reach the Python API boundary