-
Notifications
You must be signed in to change notification settings - Fork 679
improve error details for when an operator returns non-serializable value #6425
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
| from copy import deepcopy | ||
| from datetime import date, datetime | ||
| from functools import partial | ||
| from starlette.responses import Response | ||
| from json import JSONEncoder | ||
| import glob | ||
| import hashlib | ||
| import importlib | ||
|
|
@@ -3192,4 +3194,25 @@ def validate_hex_color(value): | |
| ) | ||
|
|
||
|
|
||
| class Encoder(JSONEncoder): | ||
| """Custom JSON encoder that handles numpy types.""" | ||
|
|
||
| def default(self, o): | ||
| if isinstance(o, np.floating): | ||
| return float(o) | ||
|
|
||
| if isinstance(o, np.integer): | ||
| return int(o) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. haven't thought it through completely, but why wouldn't we just handle the serialization here? eg if isinstance(o, DatasetView): return o._serialize()? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yah, I consider customizing but I think we should leave serialization of complex values to be case-by-case. For example, what if they have view in a nested object or what if another complex class is returned There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this work is release blocking, it's helpful to keep the change as minimal and safe as possible |
||
| return JSONEncoder.default(self, o) | ||
|
|
||
|
|
||
| async def create_response(response: dict): | ||
| """Creates a JSON response from the given dictionary.""" | ||
| return Response( | ||
| await run_sync_task(lambda: json_util.dumps(response, cls=Encoder)), | ||
| headers={"Content-Type": "application/json"}, | ||
| ) | ||
|
|
||
|
|
||
| fos = lazy_import("fiftyone.core.storage") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,17 @@ | |
| | | ||
| """ | ||
|
|
||
| from datetime import datetime, timedelta | ||
| import logging | ||
| import traceback | ||
|
|
||
| from datetime import datetime, timedelta | ||
| from typing import Union | ||
|
|
||
| from .operator import Operator | ||
| from fiftyone.core.utils import create_response | ||
| from starlette.exceptions import HTTPException | ||
| from starlette.responses import JSONResponse, Response | ||
|
|
||
| from .executor import ExecutionResult | ||
|
|
||
|
|
||
| class ProgressHandler(logging.Handler): | ||
|
|
@@ -107,3 +114,43 @@ def is_new(release_date, days=30): | |
| raise ValueError("release_date must be a string or datetime object") | ||
|
|
||
| return (datetime.now() - release_date).days <= days | ||
|
|
||
|
|
||
| async def create_operator_response( | ||
| result: ExecutionResult, | ||
| ) -> Union[Response, JSONResponse]: | ||
| """ | ||
| Creates a :class:`starlette.responses.JSONResponse` from the given | ||
| :class:`fiftyone.operators.executor.ExecutionResult` or returns a | ||
| server error :class:`starlette.responses.JSONResponse` if serialization fails. | ||
|
|
||
| Args: | ||
| result: the operator execution result | ||
|
|
||
| Returns: | ||
| :class:`starlette.responses.Response` or | ||
| :class:`starlette.responses.JSONResponse` | ||
| """ | ||
| try: | ||
| return await create_response(result.to_json()) | ||
| except Exception as e: | ||
| # Immediately re-raise starlette HTTP exceptions | ||
| if isinstance(e, HTTPException): | ||
| raise e | ||
|
|
||
| # Cast non-starlette HTTP exceptions as JSON with 500 status code | ||
| logging.exception(e) | ||
|
|
||
| msg = ( | ||
| f"Failed to serialize operator result. {e}." | ||
| + " Make sure that the return value of the operation is JSON-serializable." | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would users know that dataset and view are not serializable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily, but with stack trace and this message, it should give them pointers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is enough for the release. Always room to improve moving forward. |
||
| ) | ||
| return JSONResponse( | ||
| { | ||
| "kind": "Server Error", | ||
| "error": msg, | ||
| "message": msg, | ||
| "stack": traceback.format_exc(), | ||
| }, | ||
| status_code=500, | ||
| ) | ||
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.
does it make sense to move this out of
server/? seems very server specificThere 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.
I moved it out to core.utils as it is now also used by
create_operator_response