SD2 1406 add error code handling nyckel #353
Conversation
WalkthroughThe changes introduce a new error handling method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NyckelApi
Client->>NyckelApi: image__automl_classification__upload_data_async__launch_job()
NyckelApi->>NyckelApi: Send HTTP request
NyckelApi->>NyckelApi: If response error, call handle_provider_error(response)
NyckelApi->>NyckelApi: Map status code to specific ProviderException
NyckelApi-->>Client: Return result or raise ProviderException
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
edenai_apis/apis/nyckel/nyckel_api.py (2)
380-381: Exception chaining removed - consider preserving original cause.You've removed the
from expexception chaining that was present in the line above (377-378). While this makes the code more consistent with similar error handling patterns in the file, it removes information about the original cause of the failure.- raise ProviderException("Could not upload image data", 400) + raise ProviderException("Could not upload image data", 400) from exp
543-577: Well-structured error handler with clear error messages.The new
handle_provider_errormethod is well-documented and provides specific error messages for different HTTP status codes, which improves the developer experience when debugging issues.However, there's a typo in the error message for status code 409.
- "Resouce conflict. Commonly raised when trying to create a sample that already exists in the function (Nyckel does not allow duplicate samples). When annotating an existing sample, use the PUT samples endpoint instead." + "Resource conflict. Commonly raised when trying to create a sample that already exists in the function (Nyckel does not allow duplicate samples). When annotating an existing sample, use the PUT samples endpoint instead."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
edenai_apis/apis/nyckel/nyckel_api.py(3 hunks)edenai_apis/apis/nyckel/nyckel_helpers.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
edenai_apis/apis/nyckel/nyckel_api.py (1)
edenai_apis/utils/exception.py (2)
ProviderException(14-26)status_code(23-26)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
edenai_apis/apis/nyckel/nyckel_helpers.py (1)
19-22: Good refactoring for clarity and maintainability.Extracting the query string into a separate variable before using it with
urllib.parse.quote_plusimproves code readability and makes the logic easier to follow.edenai_apis/apis/nyckel/nyckel_api.py (1)
352-353: Good replacement of generic error handling with specific error codes.Replacing the generic exception with a specialized error handler improves the API's usability by providing more descriptive error messages based on specific HTTP status codes.
| def handle_provider_error(self, response: requests.Response): | ||
| """ | ||
| 402 Billing issue. Mostly likely because you have exceeded the free tier quota. | ||
| 403 Forbidden. Check your credentials. | ||
| 409 Resouce conflict. Commonly raised when trying to create a sample that already exists in the function (Nyckel does not allow duplicate samples). When annotating an existing sample, use the PUT samples endpoint instead. | ||
| 429 Throttled. You have exceeded either 25 requests per second or 25 concurrent requests. | ||
| 500 Internal error. Retry -- ideally with exponential backoff. | ||
| 503 Service temporarily unavailable. Retry -- ideally with exponential backoff. | ||
| """ | ||
| if response.status_code == 402: | ||
| raise ProviderException( | ||
| "Billing issue. Mostly likely because you have exceeded the free tier quota." | ||
| ) | ||
| elif response.status_code == 403: | ||
| raise ProviderException("Forbidden. Check your credentials.") | ||
| elif response.status_code == 409: | ||
| raise ProviderException( | ||
| "Resouce conflict. Commonly raised when trying to create a sample that already exists in the function (Nyckel does not allow duplicate samples). When annotating an existing sample, use the PUT samples endpoint instead." | ||
| ) | ||
| elif response.status_code == 429: | ||
| raise ProviderException( | ||
| "Throttled. You have exceeded either 25 requests per second or 25 concurrent requests." | ||
| ) | ||
| elif response.status_code == 500: | ||
| raise ProviderException( | ||
| "Internal error. Retry -- ideally with exponential backoff." | ||
| ) | ||
| elif response.status_code == 503: | ||
| raise ProviderException( | ||
| "Service temporarily unavailable. Retry -- ideally with exponential backoff." | ||
| ) | ||
| else: | ||
| raise ProviderException( | ||
| "Something went wrong when running the prediction !!", 500 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handler should pass status code to ProviderException.
The current implementation doesn't pass the response status code to the ProviderException constructor in most cases, which means this information is lost. The ProviderException class supports a code parameter which should be utilized.
- raise ProviderException(
- "Billing issue. Mostly likely because you have exceeded the free tier quota."
- )
+ raise ProviderException(
+ "Billing issue. Mostly likely because you have exceeded the free tier quota.",
+ response.status_code
+ )Apply similar changes for all other exception cases in the method.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def handle_provider_error(self, response: requests.Response): | |
| """ | |
| 402 Billing issue. Mostly likely because you have exceeded the free tier quota. | |
| 403 Forbidden. Check your credentials. | |
| 409 Resouce conflict. Commonly raised when trying to create a sample that already exists in the function (Nyckel does not allow duplicate samples). When annotating an existing sample, use the PUT samples endpoint instead. | |
| 429 Throttled. You have exceeded either 25 requests per second or 25 concurrent requests. | |
| 500 Internal error. Retry -- ideally with exponential backoff. | |
| 503 Service temporarily unavailable. Retry -- ideally with exponential backoff. | |
| """ | |
| if response.status_code == 402: | |
| raise ProviderException( | |
| "Billing issue. Mostly likely because you have exceeded the free tier quota." | |
| ) | |
| elif response.status_code == 403: | |
| raise ProviderException("Forbidden. Check your credentials.") | |
| elif response.status_code == 409: | |
| raise ProviderException( | |
| "Resouce conflict. Commonly raised when trying to create a sample that already exists in the function (Nyckel does not allow duplicate samples). When annotating an existing sample, use the PUT samples endpoint instead." | |
| ) | |
| elif response.status_code == 429: | |
| raise ProviderException( | |
| "Throttled. You have exceeded either 25 requests per second or 25 concurrent requests." | |
| ) | |
| elif response.status_code == 500: | |
| raise ProviderException( | |
| "Internal error. Retry -- ideally with exponential backoff." | |
| ) | |
| elif response.status_code == 503: | |
| raise ProviderException( | |
| "Service temporarily unavailable. Retry -- ideally with exponential backoff." | |
| ) | |
| else: | |
| raise ProviderException( | |
| "Something went wrong when running the prediction !!", 500 | |
| ) | |
| def handle_provider_error(self, response: requests.Response): | |
| """ | |
| 402 Billing issue. Mostly likely because you have exceeded the free tier quota. | |
| 403 Forbidden. Check your credentials. | |
| 409 Resouce conflict. Commonly raised when trying to create a sample that already exists in the function (Nyckel does not allow duplicate samples). When annotating an existing sample, use the PUT samples endpoint instead. | |
| 429 Throttled. You have exceeded either 25 requests per second or 25 concurrent requests. | |
| 500 Internal error. Retry -- ideally with exponential backoff. | |
| 503 Service temporarily unavailable. Retry -- ideally with exponential backoff. | |
| """ | |
| if response.status_code == 402: | |
| raise ProviderException( | |
| "Billing issue. Mostly likely because you have exceeded the free tier quota.", | |
| response.status_code | |
| ) | |
| elif response.status_code == 403: | |
| raise ProviderException( | |
| "Forbidden. Check your credentials.", | |
| response.status_code | |
| ) | |
| elif response.status_code == 409: | |
| raise ProviderException( | |
| "Resouce conflict. Commonly raised when trying to create a sample that already exists in the function (Nyckel does not allow duplicate samples). When annotating an existing sample, use the PUT samples endpoint instead.", | |
| response.status_code | |
| ) | |
| elif response.status_code == 429: | |
| raise ProviderException( | |
| "Throttled. You have exceeded either 25 requests per second or 25 concurrent requests.", | |
| response.status_code | |
| ) | |
| elif response.status_code == 500: | |
| raise ProviderException( | |
| "Internal error. Retry -- ideally with exponential backoff.", | |
| response.status_code | |
| ) | |
| elif response.status_code == 503: | |
| raise ProviderException( | |
| "Service temporarily unavailable. Retry -- ideally with exponential backoff.", | |
| response.status_code | |
| ) | |
| else: | |
| raise ProviderException( | |
| "Something went wrong when running the prediction !!", | |
| response.status_code | |
| ) |
Summary by CodeRabbit