[Docs] Treat delegated execution separately #5184
Conversation
WalkthroughThe pull request includes significant updates to the documentation for using plugins in FiftyOne, particularly regarding the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (3)
docs/source/plugins/using_plugins.rst (2)
656-705: Consider adding error handling documentation.While the delegation example is clear, it would be helpful to document how errors in delegated operations are handled and how users can monitor the operation's status.
Line range hint
767-858: Consider documenting limitations and best practices.The section provides excellent examples of delegating function calls. Consider adding:
- Any limitations on the types of functions that can be delegated
- Best practices for handling long-running delegated operations
- Memory considerations for large datasets
fiftyone/operators/executor.py (1)
264-270: LGTM! Consider enhancing the error message.The added error handling for inputs schema serialization is a good defensive programming practice. The code gracefully handles potential serialization failures without blocking the delegation process.
Consider including the error type in the warning message for better debugging:
- f"Failed to resolve inputs schema for the operation: {str(e)}" + f"Failed to resolve inputs schema for the operation: {type(e).__name__}: {str(e)}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
docs/source/plugins/using_plugins.rst(5 hunks)fiftyone/operators/executor.py(1 hunks)
🔇 Additional comments (2)
docs/source/plugins/using_plugins.rst (2)
597-598: LGTM! Clear and concise example.
The example effectively demonstrates how to compute metadata using the operator.
712-712: LGTM! Clear section introduction.
The introduction effectively sets up the direct execution section.
| @@ -893,7 +927,7 @@ delegated operations and execute them serially in its process. | |||
|
|
|||
| You must also ensure that the | |||
| :ref:`allow_legacy_orchestrators <configuring-fiftyone>` config flag is set | |||
| in the environment where you run the App, e.g. by setting: | |||
| in the environment where you run the App/SDK, e.g. by setting: | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider highlighting this requirement more prominently.
This environment configuration is crucial for using delegated operations. Consider moving this information to the beginning of the delegated operations section or adding it to a "Prerequisites" subsection.
|
|
||
| Here's some examples of delegating common tasks that can be expressed in the | ||
| above forms: | ||
| In fact, the |
There was a problem hiding this comment.
I think we need a kitchen sink example of __call__, request_delegation, allow_delegated_execution, adding a delegate property, and determining if an operator was scheduled (which we know is missing). In other words, an example operator that illustrates everything that you should do when implementing allow_delegated_execution.
There was a problem hiding this comment.
determining if an operator was scheduled (which we know is missing)
I fixed this here: it is available via ctx.delegated
adding a delegate property
This is now effectively deprecated. Plugin devs should always use execution_options() as pictured here, since the plugin framework can be trusted as authoritative on which execution modes the current deployment supports
I think we need a kitchen sink example of call, request_delegation, allow_delegated_execution,
That's what I tried to do here. There are multiple examples of increasing complexity here, including a last one that links over to here for additional context
There was a problem hiding this comment.
|
@ritch I'd like to go ahead and merge this as-is if you're onboard. We can upgrade the docs further in a follow-up PR if needed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/source/plugins/using_plugins.rst (2)
656-695: Consider adding a note about backward compatibility.While the documentation clearly explains the new delegation approach using
request_delegation=True, it might be helpful to add a note about backward compatibility for users migrating from older versions.@@ -661,6 +661,11 @@ `request_delegation=True` flag to :func:`execute_operator() <fiftyone.operators.execute_operator>`. +.. note:: + + This approach replaces the previous method of using the `delegate` parameter + directly in operator calls, providing a more consistent way to handle + delegated execution across all operators.
800-858: Consider adding error handling examples.The new section on delegating function calls provides comprehensive examples but could benefit from examples of error handling and status checking.
@@ -857,6 +857,15 @@ persistent=True, ) + # Example with error handling + try: + result = delegate( + "compute_metadata", + dataset=dataset, + timeout=3600 # optional timeout in seconds + ) + except fo.DelegatedOperationError as e: + print(f"Operation failed: {e}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
docs/source/plugins/developing_plugins.rst(3 hunks)docs/source/plugins/using_plugins.rst(5 hunks)
🔇 Additional comments (6)
docs/source/plugins/using_plugins.rst (3)
597-598: LGTM: Clear and concise example of compute_metadata usage.
The example effectively demonstrates the basic usage of the compute_metadata operator without unnecessary complexity.
699-705: LGTM: Clear example of delegated execution.
The example effectively demonstrates how to schedule a delegated operation using the new approach.
929-929: Consider highlighting this requirement more prominently.
The environment configuration is crucial for using delegated operations. Consider moving this information to the beginning of the delegated operations section.
docs/source/plugins/developing_plugins.rst (3)
1145-1148: LGTM! Well-structured section header
The section header is properly formatted and provides a clear introduction to the execution options content.
1186-1189: LGTM! Clear subsection header
The subsection header is well-formatted and appropriately introduces the dynamic execution options content.
1241-1243: LGTM! Clear explanation of fallback behavior
The text effectively explains what happens when resolve_delegation() is not implemented or returns None, which is crucial information for plugin developers.
Summary by CodeRabbit
Documentation
delegateparameter from thecompute_metadataoperator.Bug Fixes
inputsisNone.