-
Notifications
You must be signed in to change notification settings - Fork 43
Add more expressive exception when function not found #1779
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
base: main
Are you sure you want to change the base?
Conversation
ad4c3a1 to
f4e64bd
Compare
| except QiskitServerlessException as e: | ||
| raise QiskitServerlessException( | ||
| f"Failed to retrieve '{provider}/{title}'. " | ||
| "Please check that your API credentials and the function name are correct. " | ||
| "You can view the list of available functions for your credentials using `.list()`." | ||
| ) from 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.
The try/except will catch any exception, not only 404, and it will show a generic error, hiding the original cause. It will be less confusing for the user, but it will give the user zero chance to know what happened (imagine if they ask us why this error occurs, we won't be able to tell them).
On the other hand, the fix is only done in the functions call, what happen with the others?
So, my suggestion is to attack the problem at its root: the safe_json_request function in the json.py here:
qiskit-serverless/client/qiskit_serverless/utils/json.py
Lines 142 to 147 in 06dcf42
| if response is not None and not response.ok: | |
| raise QiskitServerlessException( | |
| format_err_msg( | |
| response.status_code, | |
| str(response.text), | |
| ) |
And check if the response.status_code is a common status (404, 403, 50x), and wrap using the same json structure (I mean, same fields) but a more descriptive explanation.
Let me know what you think about it. :)
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.
The try/except will catch only exceptions of type QiskitServerlessExceptions as specified in line 591, and it will raise a custom message on top of the original exception (thanks to the from e in line 596), so the original 404 is visible to users. Here is an example of the full trace:
---------------------------------------------------------------------------
QiskitServerlessException Traceback (most recent call last)
File ~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/core/clients/serverless_client.py:581, in ServerlessClient.function(self, title, provider)
580 try:
--> [581](https://file+.vscode-resource.vscode-cdn.net/Users/ept/qiskit_workspace/qiskit-function-templates/~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/core/clients/serverless_client.py:581) response_data = safe_json_request_as_dict(
582 request=lambda: requests.get(
583 f"{self.host}/api/{self.version}/programs/get_by_title/{title}",
584 headers=get_headers(
585 token=self.token, instance=self.instance, channel=self.channel
586 ),
587 params={"provider": provider},
588 timeout=REQUESTS_TIMEOUT,
589 )
590 )
591 except QiskitServerlessException as e:
File ~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/utils/json.py:[109](https://file+.vscode-resource.vscode-cdn.net/Users/ept/qiskit_workspace/qiskit-function-templates/~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/utils/json.py:109), in safe_json_request_as_dict(request)
98 """Returns parsed json data from request.
99
100 Args:
(...)
107 parsed json response as dict structure
108 """
--> 109 response = safe_json_request(request)
110 if isinstance(response, Dict):
File ~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/utils/json.py:[143](https://file+.vscode-resource.vscode-cdn.net/Users/ept/qiskit_workspace/qiskit-function-templates/~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/utils/json.py:143), in safe_json_request(request)
142 if response is not None and not response.ok:
--> 143 raise QiskitServerlessException(
144 format_err_msg(
145 response.status_code,
146 str(response.text),
147 )
148 )
150 decoding_error_message: Optional[str] = None
QiskitServerlessException:
| Message: Http bad request.
| Code: 404
The above exception was the direct cause of the following exception:
QiskitServerlessException Traceback (most recent call last)
Cell In[4], [line 1](vscode-notebook-cell:?execution_count=4&line=1)
----> [1](vscode-notebook-cell:?execution_count=4&line=1) tem = serverless.load("algorithmiq/tem-dev")
File ~/qiskit_workspace/qiskit-function-templates/.venv_hamsim/lib/python3.11/site-packages/qiskit_ibm_catalog/catalog.py:94, in QiskitFunctionsCatalog.load(self, title, provider)
82 def load(
83 self, title: str, provider: Optional[str] = None
84 ) -> Optional[RunnableQiskitFunction]:
85 """Loads Qiskit function by title
86
87 Args:
(...)
92 Optional[QiskitFunction]: qiskit function
93 """
---> [94](https://file+.vscode-resource.vscode-cdn.net/Users/ept/qiskit_workspace/qiskit-function-templates/~/qiskit_workspace/qiskit-function-templates/.venv_hamsim/lib/python3.11/site-packages/qiskit_ibm_catalog/catalog.py:94) return self._client.function(title=title, provider=provider)
File ~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/core/decorators.py:480, in trace_decorator_factory.<locals>.generated_decorator.<locals>.decorator_trace.<locals>.wrapper(*args, **kwargs)
474 function_name = (
475 traced_function
476 if isinstance(traced_function, str)
477 else func.__name__
478 )
479 with tracer.start_as_current_span(f"{traced_feature}.{function_name}"):
--> [480](https://file+.vscode-resource.vscode-cdn.net/Users/ept/qiskit_workspace/qiskit-function-templates/~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/core/decorators.py:480) result = func(*args, **kwargs)
481 return result
File ~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/core/clients/serverless_client.py:592, in ServerlessClient.function(self, title, provider)
581 response_data = safe_json_request_as_dict(
582 request=lambda: requests.get(
583 f"{self.host}/api/{self.version}/programs/get_by_title/{title}",
(...)
589 )
590 )
591 except QiskitServerlessException as e:
--> [592](https://file+.vscode-resource.vscode-cdn.net/Users/ept/qiskit_workspace/qiskit-function-templates/~/qiskit_workspace/qiskit-serverless/client/qiskit_serverless/core/clients/serverless_client.py:592) raise QiskitServerlessException(
593 f"Failed to retrieve '{provider}/{title}'. "
594 "Please check that your API credentials and the function name are correct. \n"
595 "You can view the list of available functions for your credentials using `.list()`."
596 ) from e
598 response_data["client"] = self
599 the_function = RunnableQiskitFunction.from_json(response_data)
QiskitServerlessException: Failed to retrieve 'algorithmiq/tem-dev'. Please check that your API credentials and the function name are correct.
You can view the list of available functions for your credentials using `.list()`.
I agree that this mechanism is not as fine-grained as we'd like, after all, the serverless exception is wrapped twice and this is suboptimal, but safe_json_request is not the place to raise such a concrete error, as the function is reused in many methods of the client (and this error refers specifically to the failure to retrieve a function).
We could combine a better error handling from safe_json_request (analyzing the code) + a high level wrapping like this one. I will think about it.
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.
Oh, you are right Elena! We can not set such a message in a place that is called from everywhere! it's too specific!
This is interesting... what do you think about sending the messages to replace to the safe_json_request function? A new parameter error_messages: Optional[Dict[int, str]] = None. Then, inside the safe_json_request function, you could check the response.status_code and find the message. You could have a default mapping like:
default_messages = {
# I don't know why Python returns a "Http bad request" in 404 errors!
# But this is a better message and it's still generic enough and
# it can be used from any place
404: "Resource not found",
}But in the functions endpoint, you define a better mapping to overwrite the defaults:
response_data = safe_json_request_as_dict(
request=lambda: requests.get(
f"{self.host}/api/{self.version}/programs/get_by_title/{title}",
# ....
error_messages={
404: f"Function '{provider}/{title}' not found. \
Please verify the function name or use `.list()` to see available functions",
# more messagesThere 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.
One question, initially from the backend we are returning the source of the error (If I remember correctly). Can we just use maybe the error that the backend returns?
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.
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.
Yeah, I think that we are starting doing good job on meaningfully backend errors. If some of them are not so clear, we can try to improve them.
Summary
This PR addresses #1773 by providing a more informative exception than a 404 error when a function is not found. The full trace is exposed so the 404 is still visible for debugging.
Details and comments