Skip to content

RSDK-5175 - return file upload server error #482

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 13, 2023
Merged

RSDK-5175 - return file upload server error #482

merged 4 commits into from
Nov 13, 2023

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Nov 6, 2023

Major changes

When there is no response to a file upload, throw the appropriate gRPC error.

Stacktrace (in relevant part) before change:

...
  File "/opt/homebrew/lib/python3.11/site-packages/viam/app/data_client.py", line 682, in _file_upload
    assert response is not None
           ^^^^^^^^^^^^^^^^^^^^

Stacktrace (in relevant part) after change:

  File "/opt/homebrew/lib/python3.11/site-packages/viam/app/data_client.py", line 680, in _file_upload
    await stream.recv_trailing_metadata()  # causes us to throw appropriate gRPC error.
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/grpclib/client.py", line 484, in recv_trailing_metadata
    self._raise_for_grpc_status(status, message, details)
  File "/opt/homebrew/lib/python3.11/site-packages/grpclib/client.py", line 345, in _raise_for_grpc_status
    raise GRPCError(status, message, details)
grpclib.exceptions.GRPCError: (<Status.UNKNOWN: 2>, 'no robot part found', None)

@stuqdog stuqdog requested a review from a team as a code owner November 6, 2023 13:37
@stuqdog stuqdog requested review from njooma and cheukt November 6, 2023 13:37
cheukt
cheukt approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

should we do this in other parts of the SDK as well? e.g. I think upload_module_file has this too

@stuqdog stuqdog requested a review from cheukt November 7, 2023 13:18
cheukt
cheukt approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

LGTM!

njooma
njooma reviewed Nov 10, 2023
View reviewed changes
Copy link
Member

@njooma njooma left a comment

Choose a reason for hiding this comment

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

One q

src/viam/app/app_client.py Outdated
response = await stream.recv_message()
assert response is not None
if not response:
await stream.recv_message() # causes us to throw appropriate gRPC error.
Copy link
Member

Choose a reason for hiding this comment

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

You do recv_trailing_metadata in the other two functions, but recv_message here. Any difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, thanks for catching that! Mistake on my part, it should be recv_trailing_metadata :) Fixed!

@stuqdog stuqdog merged commit fd47f61 into viamrobotics:main Nov 13, 2023
@stuqdog stuqdog deleted the return-server-error-in-data-client branch November 13, 2023 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njooma njooma njooma approved these changes

@cheukt cheukt cheukt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stuqdog @njooma @cheukt