Error and warning improvements #122

jjmaldonis · 2023-07-28T22:28:47Z

Summary:

This PR creates custom Exception objects when errors are thrown from the Deepgram SDK and provides support for the new warnings object in API responses.

Details:

Prior to this PR, Deepgram errors were distinguished by prefixing the error message with a "DG:" string. This PR removes the string prefix from all error messages and the exceptions were replaced with custom Exception objects. This allows users to use try/except blocks to handle errors from the Deepgram SDK, which is standard practice for Python libraries.

Two new exceptions will be thrown: DeepgramSetupError will be thrown if an error occurs while instantiating the Deepgram SDK (such as a missing API key), and DeepgramApiError will be thrown when errors are encountered during API requests. Both errors inherit from a DeepgramError base class exception, which allows users to catch all Deepgram exceptions in one except block.

The DeepgramApiError has a number of fields the user may find useful when handling this exception, such as an HTTP status code if one exists.

The new support for warnings defaults to printing a warning to stdout using Python's built-in warnings module. This is standard practice for handling warnings.

Two options were added to allow users to customize how warnings are handled. The Options class that is passed to the Deepgram SDK duration initialization has two new fields: suppress_warnings which prevents the warnings from being printed to the console, and raise_warnings_as_errors which raises warnings as errors if a warning is found in the API response.

Testing:

The pytest tests run successfully on Python 3.11 and Python 3.9. I did not test on Python 3.10 but there are no changes that should impact it.
The new support for warnings can be tested by hitting the Deepgram API with summarization=true&detect_language=true and using a non-English language. Here is the German audio file I used for testing.
- I tested suppress_warnings and raise_warnings_with_errors with this API call and they work correctly.
To test the new error handling, I used tier=nova&language=de (with any audio file/URL). It works well. There are errors I wasn't able to reproduce, such as 503 errors from Deepgram.
I also tested timeout errors, which raise the appropriate exception (no change).

jkroll-deepgram · 2023-07-31T20:47:54Z

deepgram/__init__.py

+        if 'api_key' not in options:
+            raise DeepgramSetupError("API key is required")
+
+        if "api_url" in options and options.get("api_url", None) is None:


nit: Consistently use single or double quotes, not both

I completely agree. The existing code has a mix of single quotes and double quotes. Python's standard is double quotes, so that's what I use. I did not want to add a ton of formatting changes to this PR, so I kept them minimal. I can change this one in particular because you raised it, but I'd like to go through and reformat the entire codebase in a later PR.

I see, I hadn't noticed that before! Yeah agreed that it's better served by doing a cleanup sweep in a separate PR.

jkroll-deepgram · 2023-07-31T20:48:56Z

deepgram/__init__.py

-            raise ValueError("DG: API key is required")
-        self.options = t_options
+        if not isinstance(options, (str, dict)):
+            raise DeepgramSetupError("`options` must be a dictionary or an API key string")


What do you think about using the name DeepgramAuthError rather than DeepgramSetupError? Is the setup only for API key / authentication?

The line you highlighted is not an Auth error and instead implies that the user passed the incorrect type to the constructor. We could also throw a TypeError here, but DeepgramSetupError keeps the error codes consistent. Also DeepgramSetupError inherits from ValueError which is applicable when the user passes the wrong value/type to the constructor.

jkroll-deepgram · 2023-07-31T20:49:45Z

deepgram/_types.py

+    suppress_warnings: bool
+    raise_warnings_as_errors: bool


Should these have default values?

TypedDicts do not have default values, and none of the other classes in this file have default values. Technically these types should have Optional around them, but very few of the other optional fields in this file use Optional so I left it off here as well. This is something we can clean up in the future.

We could also convert the TypedDicts to dataclasses (I prefer dataclasses to TypedDicts). Alternatively, we could use TypedDict's Required and NotRequired types defined in this pep: https://peps.python.org/pep-0655/

I don't have a strong preference - do you think TypedDicts are marginally more performant, but dataclasses are more flexible? We do have an awful lot of TypedDicts already, so it makes sense to defer to that standard for now.

My interest in an explicit default value is mainly to make clear what the behavior will be when the user doesn't set anything. But I guess in this case, it's logical for warnings to be warnings by default... rather than being suppressed or errors.

Yeah, TypedDicts are more performant than dataclasses. I forgot the SDKs are written with high performance as a top priority.

I completely agree with you, it would be really nice to have default values.

jkroll-deepgram · 2023-07-31T20:57:10Z

deepgram/_utils.py

+                if options.get("raise_warnings_as_errors") and "metadata" in body and "warnings" in body["metadata"]:
+                    raise DeepgramApiError({"warnings": body["metadata"]["warnings"]}, http_library_error=None)


Another alternative for when nested dictionary keys might not be present is chaining gets:

if options.get("raise_warnings_as_errors") and body.get("metadata", {}).get("warnings", {}):

The streaming test suite uses that style and I've been a fan of it.

This applies in a few cases throughout the PR.

Honestly I personally prefer the in operator rather than nested .gets. For complex "contains" checks, I have to really look a the .gets and the parentheses to understand how the code branches and which entries are default values vs. conditionals. For example, metadata and warnings are conditionals, but both {}s are default values. If I do not read a comma or parenthesis correctly, I may misunderstand which is which. This is particularly true when strings are used as default values.

On the other hand, I can typically read if statements with with in operator left-to-right (as if it were a sentence) and understand the logic. (Which I hope is true for the if statement I wrote.)

Python does not have a style recommendation for this, but if Deepgram wants to define it then that works for me.

The downside I see with the in syntax is that it gets verbose and repetitive for nested dictionaries. But you're only going 2 levels deep so it's not too bad. I'm fine with it as-is and I don't know of a specific style guide to be followed here. (Seems like the absence of a DG Python style guide is becoming a theme in this PR discussion!)

jkroll-deepgram · 2023-07-31T21:01:08Z

deepgram/errors.py

+                urllib.error.HTTPError,
+                urllib.error.URLError,
+                websockets.exceptions.InvalidHandshake,
+                aiohttp.ClientResponseError,


Does this mean we are presenting all of these errors as a type of Deepgram error? Would we maybe want other non-DG errors to bubble up rather than wrapping them as our own?

In the previous iteration of the code, the Python SDK was catching all non-DG errors and returning them as Exception, sometimes without using the from operator (!!!). That means the non-DG errors - and the information available in them - were inaccessible.

I agree that we could allow these errors to bubble up on their own rather than catch them and wrap them in a Deepgram error. It would allow users to be more explicit about how to handle errors, but it would also make error handling more difficult for the user because they would need to handle multiple exception types.

The two approaches are targeted at different audiences: The implementation where errors are wrapped up into a single DG exception makes it easy to do simply error handling and therefore makes it easy for newer programmers to handle failed API requests. On the other hand, the implementation where errors bubble up requires the user to be explicit about how they handle the different use cases, which is likely more useful for power users.

The code I wrote grabs the underlying non-DG error and adds it as a field to the DG error. I hope this satisfies both user groups because: the programmers with less experience can simply catch one error, and the power users can grab the underlying HTTP error and perform more advanced error handling.

Aside: I said, "the implementation where errors bubble up requires the user to be explicit about how they handle the different use cases". That isn't completely true because users could always write except Exception: to catch all errors, but I try to highly discourage the use of except Exception. It is almost always too broad. For example, if a user passes an incorrect type that results in a TypeError, they wouldn't want their retry logic to trigger and keep making API calls with the same TypeError.

After that long explanation, I hope this code serves both new users and power users by making it easy to handle failed API requests in both simple and complex ways.

jkroll-deepgram · 2023-07-31T21:02:59Z

deepgram/errors.py

+        if isinstance(args[0], dict) and "err_msg" in args[0]:
+            self.error = args[0]["err_msg"]
+            if "metadata" in args[0] and "warnings" in args[0]["metadata"]:
+                self.warnings = args[0]["metadata"]["warnings"]
+            elif "warnings" in args[0]:  # Occurs when `raise_warnings_as_errors` is enabled
+                self.warnings = args[0]["warnings"]
+            if "metadata" in args[0] and "request_id" in args[0]["metadata"]:  # Occurs when Deepgram returns a success response (for warnings)
+                self.request_id = uuid.UUID(args[0]["request_id"])
+            elif "request_id" in args[0]:  # Occurs when Deepgram returns a failed response
+                self.request_id = uuid.UUID(args[0]["request_id"])
+        elif isinstance(args[0], str):
+            self.error = args[0]
+        else:
+            self.error = str(args[0])


You're referring to args[0] a lot here, it would be helpful for readability to give that a named variable so it's clearer what that arg is.

Thanks, fixed. I created a new variable: error_or_warning_data = args[0]

jkroll-deepgram · 2023-08-01T15:28:22Z

Approved! But best to wait until signoff from Luke/DX before merging 🙏

jkroll-deepgram · 2023-08-01T15:30:41Z

Oh one more question - can you add some tests in tests/, such as to assert that certain exceptions are thrown when expected? Or that warnings are suppressed/raised to errors when expected?

jjmaldonis · 2023-08-07T14:52:12Z

Oh one more question - can you add some tests in tests/, such as to assert that certain exceptions are thrown when expected? Or that warnings are suppressed/raised to errors when expected?

I added two tests that ensure the appropriate errors are raised.

into error-and-warning-improvements

jjmaldonis added 2 commits July 28, 2023 16:28

created new Exception types and added support for warnings

28ec810

minor bugfixes

7e18371

jkroll-deepgram reviewed Jul 31, 2023

View reviewed changes

cleanup for PR review

395e3d5

jkroll-deepgram previously approved these changes Aug 1, 2023

View reviewed changes

bugfix

902b4f5

jjmaldonis dismissed jkroll-deepgram’s stale review via 902b4f5 August 7, 2023 16:41

jjmaldonis added 3 commits August 11, 2023 12:22

Merge branch 'main' of https://github.com/jjmaldonis/deepgram-python-sdk

370d586

into error-and-warning-improvements

added tests

e76e48b

changed the language used in a test

935dbe1

jkroll-deepgram approved these changes Aug 11, 2023

View reviewed changes

jpvajda merged commit 9574006 into deepgram:main Aug 14, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error and warning improvements #122

Error and warning improvements #122

jjmaldonis commented Jul 28, 2023

jkroll-deepgram Jul 31, 2023

jjmaldonis Aug 1, 2023

jkroll-deepgram Aug 1, 2023

jkroll-deepgram Jul 31, 2023

jjmaldonis Aug 1, 2023

jkroll-deepgram Jul 31, 2023

jjmaldonis Aug 1, 2023

jkroll-deepgram Aug 1, 2023

jjmaldonis Aug 1, 2023

jkroll-deepgram Jul 31, 2023

jjmaldonis Aug 1, 2023

jkroll-deepgram Aug 1, 2023

jkroll-deepgram Jul 31, 2023

jjmaldonis Aug 1, 2023

jkroll-deepgram Jul 31, 2023

jjmaldonis Aug 1, 2023

jkroll-deepgram commented Aug 1, 2023

jkroll-deepgram commented Aug 1, 2023

jjmaldonis commented Aug 7, 2023

		if options.get("raise_warnings_as_errors") and "metadata" in body and "warnings" in body["metadata"]:
		raise DeepgramApiError({"warnings": body["metadata"]["warnings"]}, http_library_error=None)

Error and warning improvements #122

Error and warning improvements #122

Conversation

jjmaldonis commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkroll-deepgram commented Aug 1, 2023

jkroll-deepgram commented Aug 1, 2023

jjmaldonis commented Aug 7, 2023