Allow legacy orchestration #5176
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
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 (4)
fiftyone/core/config.py (1)
123-128: LGTM! Consider adding docstring documentation.The implementation follows the established patterns and uses a secure default value of
False. However, consider adding a docstring to document the purpose and impact of this configuration option.def __init__(self, d=None): if d is None: d = {} + # Whether to allow legacy orchestrators for backward compatibility + # Default is False for security self.allow_legacy_orchestrators = self.parse_bool( d, "allow_legacy_orchestrators", env_var="FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS", default=False, )app/packages/operators/src/state.ts (1)
290-290: Consider providing more context in the descriptionWhile "Run this operation in the background" is user-friendly, it might be too vague. Consider adding more context about resource usage and execution environment.
- description: "Run this operation in the background", + description: "Run this operation in the background using dedicated compute resources",docs/source/plugins/using_plugins.rst (1)
894-901: LGTM! Consider adding a note about persistence.The documentation clearly explains the requirement for the
FIFTYONE_ALLOW_LEGACY_ORCHESTRATORSenvironment variable.Consider adding a note about persisting this configuration in the FiftyOne config file for a more permanent solution:
export FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS=true + +.. note:: + + You can also set this configuration permanently in your + :ref:`FiftyOne config <configuring-fiftyone>`: + + .. code-block:: json + + { + "allow_legacy_orchestrators": true + }fiftyone/operators/executor.py (1)
1280-1280: LGTM! Consider adding a docstring.The logic change correctly implements dynamic orchestrator registration control based on the
allow_legacy_orchestratorsconfiguration.Add a docstring to clarify the property's behavior:
@property def orchestrator_registration_enabled(self): + """Whether orchestrator registration is enabled. + + Returns: + bool: True if registration is enabled (legacy orchestrators disabled), + False if registration is disabled (legacy orchestrators allowed) + """ return not fo.config.allow_legacy_orchestrators
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/packages/operators/src/state.ts(2 hunks)docs/source/plugins/using_plugins.rst(1 hunks)docs/source/user_guide/config.rst(3 hunks)fiftyone/core/config.py(1 hunks)fiftyone/operators/executor.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/operators/src/state.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (4)
fiftyone/core/config.py (1)
123-128: Verify environment variable usage.
Let's verify that the environment variable FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS is properly referenced throughout the codebase.
✅ Verification successful
Environment variable FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS is properly referenced and used
The environment variable is consistently used across the codebase:
- Properly defined in config.py with correct default value (False)
- Correctly documented in user guides with clear description
- Actively used in executor.py for controlling operation scheduling behavior
- Well-documented in plugin usage guides with examples
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to the environment variable
echo "Searching for environment variable references..."
rg "FIFTYONE_ALLOW_LEGACY_ORCHESTRATORS"
# Search for any code that might be checking this configuration
echo "Searching for configuration usage..."
rg "allow_legacy_orchestrators"
# Search for any legacy orchestrator related code
echo "Searching for legacy orchestrator references..."
rg -i "legacy.*orchestrator"
Length of output: 2463
app/packages/operators/src/state.ts (1)
372-373: LGTM! Clear and informative warning message
The updated warning message effectively guides users by:
- Explaining the requirement for delegated execution
- Providing documentation links
- Offering a clear upgrade path
docs/source/user_guide/config.rst (2)
113-115: LGTM! Well-documented configuration field.
The new configuration field allow_legacy_orchestrators is properly documented with clear description, environment variable, and default value.
183-183: LGTM! Consistent example updates.
The new configuration field is properly added to both Python and CLI example outputs, maintaining consistency with the configuration table.
Also applies to: 233-233
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/packages/operators/src/SplitButton.tsx (3)
131-133: LGTM! Consider using MUI's built-in button stylesThe cursor style implementation is correct, but consider using MUI's built-in disabled button styling for consistency.
sx={{ - cursor: option.onClick ? "pointer" : "default", + ...(!option.onClick && { + pointerEvents: "none", + cursor: "default" + }) }}
Line range hint
28-35: Add TypeScript interface for component propsThe component lacks proper TypeScript type definitions which impacts maintainability and type safety.
interface Option { id: string; label: string; choiceLabel?: string; description?: string; tag?: string; selected?: boolean; default?: boolean; disabled?: boolean; onClick?: () => void; onSelect: () => void; } interface SplitButtonProps { options: Option[]; submitOnEnter?: boolean; onSubmit: () => void; disabled?: boolean; disabledReason?: string; }
Line range hint
42-44: Add prop-types validation for runtime checkingWhile TypeScript provides compile-time type checking, consider adding prop-types for runtime validation.
import PropTypes from 'prop-types'; SplitButton.propTypes = { options: PropTypes.arrayOf( PropTypes.shape({ id: PropTypes.string.isRequired, label: PropTypes.string.isRequired, choiceLabel: PropTypes.string, description: PropTypes.string, tag: PropTypes.string, selected: PropTypes.bool, default: PropTypes.bool, disabled: PropTypes.bool, onClick: PropTypes.func, onSelect: PropTypes.func.isRequired, }) ).isRequired, submitOnEnter: PropTypes.bool, onSubmit: PropTypes.func.isRequired, disabled: PropTypes.bool, disabledReason: PropTypes.string, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/packages/operators/src/SplitButton.tsx(1 hunks)app/packages/operators/src/constants.ts(1 hunks)app/packages/operators/src/state.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/operators/src/state.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/operators/src/SplitButton.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/constants.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (1)
app/packages/operators/src/constants.ts (1)
19-19: Verify the usage patterns of IS_OSS constant
Let's verify how this constant is used across the codebase to ensure our suggested changes are safe.
| @@ -16,3 +16,4 @@ export enum QueueItemStatus { | |||
| } | |||
| export const PANEL_STATE_CHANGE_DEBOUNCE = 500; | |||
| export const PANEL_STATE_PATH_CHANGE_DEBOUNCE = 250; | |||
| export const IS_OSS = true; // false in fiftyone-teams | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the configuration management approach
The hard-coded boolean with an inline comment suggesting manual modification for different versions could lead to maintenance issues. Consider these improvements:
- Add explicit type annotation for better type safety
- Consider moving this to a configuration system that can be controlled at build/runtime
- export const IS_OSS = true; // false in fiftyone-teams
+ export const IS_OSS: boolean = process.env.FIFTYONE_IS_OSS === 'true';Consider these architectural improvements:
- Use environment variables or build-time configuration
- Implement feature flags system for version-specific features
- Consider using dependency injection for version-specific behavior
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
app/packages/operators/src/SplitButton.tsx (3)
132-134: Consider using a more semantic approach for cursor stylingWhile the current implementation works, it would be more semantic to base the cursor style on the disabled state rather than the presence of onClick.
sx={{ - cursor: option.onClick ? "pointer" : "default", + cursor: option.disabled ? "default" : "pointer", }}
151-160: Consider using theme typography instead of hardcoded font sizeThe hardcoded font size could be replaced with theme-based typography for better consistency and maintainability.
<Box sx={{ - fontSize: "11px", - "& *": { fontSize: "inherit" }, + typography: 'caption', + "& *": { fontSize: 'inherit' }, }} >
175-181: Enhance component performance and styling consistencyConsider these improvements:
- Memoize the component to prevent unnecessary rerenders in lists
- Move inline styles to sx prop for consistency with Material-UI patterns
-function PrimaryWithTag({ label, tag, disabled }) { +const PrimaryWithTag = React.memo(({ label, tag, disabled }: PrimaryWithTagProps) => { const theme = useTheme(); const tagEl = tag ? ( <span - style={{ - fontSize: "11px", - color: disabled ? theme.text.secondary : theme.custom.primarySoft, - marginLeft: "5px", - }} + sx={{ + typography: 'caption', + color: (theme) => disabled ? theme.text.secondary : theme.custom.primarySoft, + ml: 1, + }} > {tag} </span> ) : null; return ( - <div style={{ display: "flex", alignItems: "center" }}> + <Box sx={{ display: "flex", alignItems: "center" }}> {label} {tagEl} - </div> + </Box> ); -} +});app/packages/operators/src/state.ts (2)
331-349: LGTM: Enhanced UX for unavailable orchestratorsGood addition of user guidance when orchestrators are not available. The Markdown component effectively provides a documentation link for self-help.
Consider adding error boundary around the Markdown component for resilience:
- const markdownDesc = React.createElement( - Markdown, - null, - "[Learn how](https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations) to run this operation in the background" - ); + const markdownDesc = React.createElement( + React.Suspense, + { fallback: "Loading documentation link..." }, + React.createElement( + Markdown, + null, + "[Learn how](https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations) to run this operation in the background" + ) + );
397-399: LGTM: Clear warning message with documentation linkThe warning message effectively communicates the requirement for delegated execution and provides a helpful documentation link.
Consider adding error boundary similar to the previous suggestion:
- const warningMessage = React.createElement(Markdown, null, warningStr); + const warningMessage = React.createElement( + React.Suspense, + { fallback: "Loading warning message..." }, + React.createElement(Markdown, null, warningStr) + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/operators/src/SplitButton.tsx(4 hunks)app/packages/operators/src/state.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/operators/src/SplitButton.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/operators/src/state.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (4)
app/packages/operators/src/SplitButton.tsx (1)
14-14: LGTM!
The Box import is correctly added and follows Material-UI import conventions.
app/packages/operators/src/state.ts (3)
241-242: LGTM: Type improvements enhance flexibility
The changes improve type definitions by:
- Allowing rich content (React nodes) in descriptions
- Making onClick optional which better reflects the component's behavior
261-261: LGTM: Fixed typo in variable name
Corrected spelling from "hasAvailableOrchestators" to "hasAvailableOrchestrators".
297-297: LGTM: Improved user-facing message clarity
The description now uses more generic and user-friendly language, focusing on the user's perspective rather than implementation details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/source/plugins/developing_plugins.rst (1)
995-996: Documentation enhancement for execution context propertiesThe addition of
ctx.delegatedandctx.requesting_delegated_executionproperties provides important context about the execution mode of operations. This helps developers understand whether their operation is being executed immediately or has been delegated to a background task.Consider adding examples showing how these properties can be used in practice, such as:
def execute(self, ctx): if ctx.delegated: # Configure for background execution batch_size = 1000 else: # Configure for immediate execution batch_size = 100 # Process in batches for batch in ctx.dataset.iter_batches(batch_size): # Process batch pass
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/source/images/plugins/operators/operator-user-delegation.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (2)
docs/source/plugins/developing_plugins.rst(1 hunks)fiftyone/operators/executor.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fiftyone/operators/executor.py
Summary by CodeRabbit
Release Notes
New Features
Documentation
User Interface Improvements