improve error details for when an operator returns non-serializable value #6425
Conversation
WalkthroughAdds a shared JSON response encoder and async Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server as Server Route
participant Exec as Operator Executor
participant OpUtils as operators.utils<br/>create_operator_response
participant Core as core.utils<br/>create_response
participant Starlette as Starlette Response
Client->>Server: POST /operators/execute
Server->>Exec: execute()
Exec-->>Server: ExecutionResult
Server->>OpUtils: create_operator_response(result)
alt Success
OpUtils->>Core: create_response(result.to_dict())
Core-->>OpUtils: JSON Response
OpUtils-->>Server: Response
Server-->>Client: 200 JSON
else HTTPException
OpUtils-->>Server: re-raise
Server-->>Client: HTTP error
else Serialization error
OpUtils->>OpUtils: log error + traceback
OpUtils-->>Server: 500 JSON {"error":"server-error","trace":...}
Server-->>Client: 500 JSON
end
sequenceDiagram
autonumber
actor User
participant UI as OperatorPromptOutput
alt No outputFields and no errors
UI-->>User: Render nothing (early return)
else Error present
UI->>UI: reason = error.bodyResponse.error || default
UI-->>User: Render ErrorView(reason, details)
else Has outputFields
UI-->>User: Render OperatorIO
end
sequenceDiagram
autonumber
participant App as ErrorBoundary
participant View as ErrorDisplayMarkup
Note over App,View: OperatorError display labeling change
App->>View: pass OperatorError
View->>View: if error.message -> add "Message" entry
View->>View: add "Trace" entry for stack
View-->>App: Render entries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fiftyone/core/utils.py (1)
3197-3208: Consider expanding numpy type coverage.The
Encodercurrently only handlesnp.floatingandnp.integer. If operators can return other numpy types (e.g.,np.ndarray,np.bool_,np.str_), they will fail serialization.Consider extending the encoder to handle additional numpy types:
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) + + if isinstance(o, np.bool_): + return bool(o) + + if isinstance(o, np.ndarray): + return o.tolist() return JSONEncoder.default(self, o)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx(1 hunks)app/packages/operators/src/components/OperatorPromptOutput.tsx(2 hunks)fiftyone/core/utils.py(2 hunks)fiftyone/operators/server.py(2 hunks)fiftyone/operators/utils.py(2 hunks)fiftyone/server/decorators.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/operators/src/components/OperatorPromptOutput.tsxapp/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx
🔇 Additional comments (6)
app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx (1)
75-86: LGTM! Improved error display for OperatorError.The conditional check for
error.messageprevents rendering an empty Message section, and the consistent "Trace" label improves clarity.app/packages/operators/src/components/OperatorPromptOutput.tsx (1)
11-16: LGTM! Dynamic error messaging improves user experience.The early return avoids unnecessary rendering, and the dynamic
reasonprovides more context from server responses. The chainingerror?.bodyResponse?.errorassumes the server provides this structure, which aligns with the centralized response creation infiftyone/operators/utils.py.fiftyone/core/utils.py (1)
3210-3216: LGTM! Centralized response creation improves maintainability.The
create_responsefunction properly delegates JSON encoding to a background task usingrun_sync_task, which is appropriate for async contexts.fiftyone/operators/server.py (1)
110-110: LGTM! Improved error handling for operator responses.Delegating to
create_operator_responsecentralizes JSON serialization and provides better error messages when operators return non-serializable values, which is the objective of this PR.fiftyone/server/decorators.py (1)
19-19: LGTM! Eliminates code duplication.Using the centralized
create_responsefromfiftyone.core.utilsremoves duplicate encoding logic and ensures consistent JSON serialization across the codebase.fiftyone/operators/utils.py (1)
119-156: LGTM! Comprehensive error handling with helpful developer guidance.The
create_operator_responsefunction provides excellent error handling and a clear message when serialization fails: "Make sure that the return value of the operation is JSON-serializable." This directly addresses the PR objective.Note: The type hint
Response | JSONResponseuses Python 3.10+ syntax. Ensure the project targets Python 3.10 or later, or useUnion[Response, JSONResponse]for compatibility.
imanjra
force-pushed
the
bugfix/panel-event-return
branch
from
b937771 to
850f874
Compare
| ) | ||
|
|
||
|
|
||
| class Encoder(JSONEncoder): |
There was a problem hiding this comment.
does it make sense to move this out of server/? seems very server specific
There was a problem hiding this comment.
I moved it out to core.utils as it is now also used by create_operator_response
|
|
||
| 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.
Would users know that dataset and view are not serializable?
There was a problem hiding this comment.
Not necessarily, but with stack trace and this message, it should give them pointers
There was a problem hiding this comment.
I think this is enough for the release. Always room to improve moving forward.
|
|
||
| if isinstance(o, np.integer): | ||
| return int(o) | ||
|
|
There was a problem hiding this comment.
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.
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.
Given that this work is release blocking, it's helpful to keep the change as minimal and safe as possible
imanjra
force-pushed
the
bugfix/panel-event-return
branch
from
850f874 to
e716408
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx (1)
85-85: Add defensive check forerror.stackexistence.The trace entry is added unconditionally for
OperatorErrorwithout checking iferror.stackexists, which is inconsistent with the handling of other error types on lines 87-88. This could result in displaying undefined content.Apply this diff to add a defensive check:
- messages.push({ message: "Trace", content: error.stack }); + if (error.stack) { + messages.push({ message: "Trace", content: error.stack }); + }fiftyone/core/utils.py (1)
3210-3215: Consider documenting the parameter in the docstring.The function correctly delegates serialization to a background thread and returns a properly formatted JSON response. However, the docstring could be improved.
Apply this diff to enhance the docstring:
async def create_response(response: dict): - """Creates a JSON response from the given dictionary.""" + """Creates a JSON response from the given dictionary. + + Args: + response: a dictionary to serialize as JSON + + Returns: + a Starlette Response with JSON content + """ return Response( await run_sync_task(lambda: json_util.dumps(response, cls=Encoder)), headers={"Content-Type": "application/json"}, )fiftyone/operators/utils.py (1)
119-156: Excellent error handling for operator serialization.The function provides structured error handling that clearly communicates serialization failures to users. The error message appropriately guides users to ensure return values are JSON-serializable, which addresses the PR's objective of improving error details.
Minor note: The error response includes both
"error"and"message"fields with identical content (lines 150-152). This redundancy is likely for backward compatibility, but if not required, you could remove one field.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx(1 hunks)app/packages/operators/src/components/OperatorPromptOutput.tsx(2 hunks)fiftyone/core/utils.py(2 hunks)fiftyone/operators/server.py(2 hunks)fiftyone/operators/utils.py(2 hunks)fiftyone/server/decorators.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fiftyone/operators/server.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/operators/src/components/OperatorPromptOutput.tsxapp/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx
🔇 Additional comments (7)
app/packages/operators/src/components/OperatorPromptOutput.tsx (3)
11-11: LGTM! Early return improves efficiency.The early return when there are no outputFields and no errors is appropriate and improves performance.
14-16: LGTM! Dynamic error message construction improves error reporting.The extraction of the error message from
error?.bodyResponse?.errorand dynamic construction of the reason string enhances error details as intended by this PR.
13-13: Verify thatexecutorexists before accessingresult.Line 13 accesses
operatorPrompt.executor.resultwithout checking ifexecutorexists. The early return on line 11 ensures at least one ofoutputFields,executorError, orresolveErroris truthy, but if onlyoutputFieldsis truthy and no errors exist,executormight be undefined, leading to a runtime error.app/packages/components/src/components/ErrorBoundary/ErrorBoundary.tsx (1)
76-78: LGTM! Conditional rendering improves clarity.Adding the "Message" entry only when
error.messageexists is good defensive programming and improves the error display structure.fiftyone/core/utils.py (2)
3197-3208: LGTM! Minimal NumPy type handling.The
Encoderclass correctly handles NumPy scalar types during JSON serialization. The minimal approach is appropriate based on past discussions about keeping serialization case-by-case rather than attempting to handle all complex types.
3218-3218: LGTM! Lazy import for storage module.The lazy import singleton is a good pattern for commonly used modules and is already being utilized elsewhere in the codebase (e.g., line 2039).
fiftyone/server/decorators.py (1)
19-19: LGTM! Consolidates response creation.The change successfully consolidates response creation logic to
fiftyone.core.utils, reducing code duplication while maintaining the same behavior.Also applies to: 37-37
4 participants
What changes are proposed in this pull request?
improve error details for when an operator returns non-serializable value
How is this patch tested? If it is not, please explain why.
Using a test operator and python panel event
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
improve error details for when an operator returns non-serializable value
Todo
Update docs to note the rule that operator, python-panel lifecycle methods, and python-panel events must return JSON-serializable values. If a complex value to be returned (i.e. DatasetView), it must be serialized or parsed into serialiazble object
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Bug Fixes
Refactor