Fix execute_operator() with delegation #5180
Conversation
WalkthroughThe changes introduced in this pull request primarily focus on refining the error handling and request parameter management within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (3)
fiftyone/operators/delegated.py (2)
Line range hint
522-532: Improve error handling specificity and message formatting.Consider these improvements to the error handling:
- Be more specific about which AttributeError you're catching
- Use string formatting instead of concatenation for the warning message
- except (AttributeError, Exception): - logger.warning( - "Failed to resolve output schema for the operation." - + traceback.format_exc(), - ) + except (AttributeError, Exception) as e: + logger.warning( + "Failed to resolve output schema for the operation: %s\n%s", + str(e), + traceback.format_exc(), + )
Line range hint
518-532: Add tests to prevent regression.The PR mentions that tests were not added due to time constraints. Given the critical nature of this fix, we should add tests to:
- Verify context reuse works correctly
- Ensure output schema resolution handles errors gracefully
- Validate the fix for operations scheduled from SDK
Would you like me to help create these tests? I can generate test cases that cover these scenarios.
fiftyone/operators/executor.py (1)
394-399: Improve error handlingThe error handling could be enhanced by including the exception details in the error traceback.
- except Exception as e: - return ExecutionResult(error=traceback.format_exc()) + except Exception as e: + return ExecutionResult( + error=traceback.format_exc(), + error_message=str(e) + )🧰 Tools
🪛 Ruff (0.7.0)
398-398: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/operators/delegated.py(1 hunks)fiftyone/operators/executor.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
fiftyone/operators/executor.py
398-398: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (3)
fiftyone/operators/delegated.py (1)
518-521: LGTM! Verify impact on existing operations.
The changes correctly reuse the existing context by setting parameters directly in ctx.request_params. This fixes the bug where context was being unnecessarily recreated.
Let's verify the impact on existing operations:
✅ Verification successful
The search results show that resolve_type_with_context is only used in two places:
- In
fiftyone/operators/executor.pywhere it's defined and exported - In
fiftyone/operators/delegated.pywhere we're modifying its usage
Let me verify if there are any other places where the execution context or operator types are being manipulated:
Changes are safe and properly integrated with the codebase
The verification confirms that the changes are safe because:
resolve_type_with_contextis only used in two places: its definition inexecutor.pyand our modified usage indelegated.py- The
request_params["target"]andrequest_params["results"]are only set in this specific location - The output schema resolution follows the same pattern used elsewhere in the codebase, as seen in the test files
- The error handling aligns with similar schema resolution code in
executor.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other places where resolve_type_with_context is called
rg -A 5 "resolve_type_with_context"
# Search for places where request_params["target"] or request_params["results"] are set
rg -A 5 'request_params\["(?:target|results)"\]'
Length of output: 2002
Script:
#!/bin/bash
# Search for ExecutionContext usage
rg -A 5 "class ExecutionContext"
# Search for places where operator types are resolved
rg -A 5 "(?:input|output)s?_schema"
Length of output: 10813
fiftyone/operators/executor.py (2)
382-382: LGTM: Simplified resolve_type implementation
The function now correctly delegates to resolve_type_with_context, which aligns with the PR objective of reusing the existing context instead of recreating it.
385-392: LGTM: Improved function signature
The new signature explicitly accepts operator and context parameters, which:
- Makes dependencies clearer
- Enables context reuse
- Fixes the bug where context was being unnecessarily recreated
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/operators/delegated.py(1 hunks)fiftyone/operators/executor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fiftyone/operators/delegated.py
🧰 Additional context used
🪛 Ruff (0.7.0)
fiftyone/operators/executor.py
401-401: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (2)
fiftyone/operators/executor.py (2)
381-382: LGTM! Clean refactor of resolve_type()
The function has been simplified by delegating to resolve_type_with_context(), which improves code maintainability.
385-396: LGTM! Improved function signature
The function signature has been updated to take more specific parameters (operator and context) instead of raw request parameters, which improves clarity and type safety.
| try: | ||
| return operator.resolve_type( | ||
| context, context.request_params.get("target", "inputs") | ||
| ) | ||
| except Exception as e: | ||
| return ExecutionResult(error=traceback.format_exc()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling
The error handling could be improved in two ways:
- The caught exception 'e' is unused
- The error message from the exception is not included in the ExecutionResult
Consider this improvement:
try:
return operator.resolve_type(
context, context.request_params.get("target", "inputs")
)
- except Exception as e:
- return ExecutionResult(error=traceback.format_exc())
+ except Exception as e:
+ return ExecutionResult(
+ error=traceback.format_exc(),
+ error_message=str(e)
+ )Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.7.0)
401-401: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
resolve_type_with_context()had morphed since introduction and was now recreating the Context even though we already have it. The bug was that when scheduled from the SDK,operator_uriis not a part ofrequest_params. When scheduled from the UI, it is, plus a whole bunch of other attributes. Introduced sometime pre-v1.0What changes are proposed in this pull request?
Repurpose/refactor
resolve_type_with_context()to be more what the name implies it's gonna do. Then makeresolve_type()use that also.If we reuse the
ExecutionContextwe already had (which was properly created), it is cleaner plus has a validoperator_uriwhich fixes this particular bug.How is this patch tested? If it is not, please explain why.
I tried to make some exposing tests but I was getting frustrated and I have limited hours before going out for a week. So ended up scrapping it.
We definitely need more testing in this area of the code.
Here's @brimoor's manual test that works now (no error on resolving output schema).
Summary by CodeRabbit
New Features
Documentation