-
Notifications
You must be signed in to change notification settings - Fork 31
Improve ocrd-network-client error handling #1337
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
Conversation
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.
Thanks, I appreciate better error messages, we just have to make sure that no information gets lost with transforming assertions to proper exceptions and the assertions that remain are properly labelled.
src/ocrd_network/cli/client.py
Outdated
try: | ||
processors_list = client.check_deployed_processors() | ||
except RequestException as e: | ||
print(f"{getattr(e, 'detail_message', str(e))}") |
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.
print(f"{getattr(e, 'detail_message', str(e))}") | |
print(getattr(e, 'detail_message', str(e))) |
The f-string is unnecessary.
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.
Where is e.detail_message
defined? I don't see it in the requests.RequestException source code https://github.com/psf/requests/blob/420d16bc7ef326f7b65f90e4644adc0f6a0e1d44/src/requests/exceptions.py#L12
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.
Self defined in the other file in method _raise_if_error
. This is the way to get the detail-message into the exception to print it afterwards to the user
Co-authored-by: Konstantin Baierer <kba@users.noreply.github.com>
Error handling by the OCR-D-network-client is currently based on assertions. If the requests to the processing server do not return a HTTP 200 code, the program fails. However, it does so without displaying the cause of the error. I have now changed this so that the error message is read from the HTTP-server-response and displayed.
Testing: To test this, you need a running OCR-D network, which can be started with this ocrd-all PR 468
Then make a request with the client, for example:
ocrd network client processing run --address http://localhost:8000 ocrd-dummy -I DEFAULT -O MY-OUTPUT
. The path to the metsfile is missing which causes an error here. Currently, the output is as follows:AssertionError: Processing server: http://localhost:8000/processor/run/ocrd-dummy, 404
This has now been changed and following error message is displayed:
Mets file path not existing: mets.xml Cannot create DB workspace entry, `mets.xml` does not exist!
. The error-message are generated by the processing server and they are forwarded to the caller with this PR.