simplify delegated operation doc #5291
Conversation
WalkthroughThis pull request modifies the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fiftyone/factory/repos/delegated_operation_doc.py(2 hunks)fiftyone/operators/executor.py(1 hunks)
🔇 Additional comments (3)
fiftyone/factory/repos/delegated_operation_doc.py (3)
27-27: LGTM! Type safety improvement
The parameter type change from dict to ExecutionContext enhances type safety and makes the API more explicit.
33-34: LGTM! Runtime type check added
The assertion ensures type safety at runtime and simplifies the code by removing the previous conditional conversion logic.
111-113: LGTM! Improved serialization encapsulation
The serialization logic is now properly encapsulated in the ExecutionContext class through the _get_serialized_request_params() method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fiftyone/factory/repos/delegated_operation.py (2)
Line range hint
203-216: Add documentation for context type conversionThe context processing logic handles conversion from dict to ExecutionContext, but this behavior isn't documented. Consider adding docstring comments to explain this type conversion.
Add documentation like this:
if isinstance(op.context, dict): + # Convert dictionary context to ExecutionContext for type safety + # This ensures backward compatibility with older operations context = ExecutionContext( request_params=op.context.get("request_params", {}) )
Line range hint
192-204: Enhance debug logging for dataset resolutionWhile the error handling is appropriate, the debug logging could be more informative for troubleshooting purposes.
Consider enhancing the logging like this:
- logger.debug("Could not resolve dataset_id for operation. ") + logger.debug( + "Could not resolve dataset_id for operation. " + "Operator: %s, Context: %s", + op.operator, + context + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/factory/repos/delegated_operation.py(1 hunks)
🔇 Additional comments (1)
fiftyone/factory/repos/delegated_operation.py (1)
217-217: LGTM: Explicit context assignment ensures type safety
The explicit assignment of the processed context to op.context ensures type safety by guaranteeing that the context is always an ExecutionContext instance before serialization.
Let's verify that all existing operations in the database have valid contexts:
✅ Verification successful
LGTM: Context handling is consistent and type-safe
The explicit assignment of context to op.context is correct and consistent with the codebase's patterns. The verification shows that:
ExecutionContextis properly defined infiftyone/operators/executor.py- The context assignment in
delegated_operation.pyfollows the same pattern used in tests and other operational code - Type checking is enforced through
isinstance(op.context, ExecutionContext)checks - The context is properly serialized and deserialized throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any operations with invalid context types in the database
# Expected: All operations should have a valid ExecutionContext
# Search for context-related imports or usage
rg -A 2 "ExecutionContext" --type python
# Search for any direct dictionary assignments to context
ast-grep --pattern 'context = {$$$}'
Length of output: 110
Script:
#!/bin/bash
# Let's try a different approach to verify the context handling
# Search for ExecutionContext class definition and usage
rg -l "class ExecutionContext"
# Search for context-related patterns in Python files
rg "ExecutionContext" --type-add 'python:*.py' --type python
# Look for context assignments in operation-related files
rg "op\.context" --type-add 'python:*.py' --type python
# Find all Python files that might handle operation contexts
fd -e py | xargs rg "context.*=.*" --type-add 'python:*.py' --type python
Length of output: 19311
| @@ -214,6 +214,7 @@ def queue_operation(self, **kwargs: Any) -> DelegatedOperationDocument: | |||
| context.request_params["dataset_id"] = str(op.dataset_id) | |||
| context.request_params["dataset_name"] = context.dataset.name | |||
|
|
|||
| op.context = context | |||
There was a problem hiding this comment.
This fixes an existing issue that I think might explain some of the weirdness/handling of op.context as dict!
Without this change this is a no-op.
| @@ -889,6 +889,11 @@ def store(self, store_name): | |||
| dataset_id = self.dataset._doc.id | |||
| return ExecutionStore.create(store_name, dataset_id) | |||
|
|
|||
| def _get_serialized_request_params(self): | |||
There was a problem hiding this comment.
Not really liking this as the interface...but didn't feel that changing .serialize() was the right move. However there is no need for panel_state in a serialized ctx. Instead it should really be outside of request_params to begin with. I've started that change elsewhere, but its a much more complex change and regresses some functionality in the dashboard plugin.
paramsfrom operations docsSummary by CodeRabbit
New Features
Improvements