-
Notifications
You must be signed in to change notification settings - Fork 591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update docs to mention builtin orchestrator #5140
Conversation
WalkthroughThe pull request introduces several updates to the FiftyOne documentation, particularly focusing on 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
docs/source/plugins/index.rst (1)
14-15
: LGTM with a minor suggestion for clarityThe update appropriately mentions the FiftyOne Teams Builtin Orchestrator. However, the phrase "like the" suggests there might be other orchestrators, which could be confusing if the builtin orchestrator is the primary or only supported option.
Consider this minor rewording for better clarity:
-that execute on a connected workflow orchestration tool like the FiftyOne Teams -Builtin Orchestrator. +that execute on the FiftyOne Teams Builtin Orchestrator, a connected workflow +orchestration tool.docs/source/index.rst (1)
Line range hint
611-612
: Remove the__SUB_NEW__
markersThe
__SUB_NEW__
markers appear to be temporary annotations and should be removed as they are not standard RST directives.- Dataset Zoo __SUB_NEW__ <dataset_zoo/index> - Model Zoo __SUB_NEW__ <model_zoo/index> + Dataset Zoo <dataset_zoo/index> + Model Zoo <model_zoo/index>docs/source/plugins/using_plugins.rst (1)
952-975
: Consider improving the formatting consistencyWhile the content is informative, there are some minor formatting inconsistencies:
- Line 959: The sentence doesn't start with a capital letter "would"
- Line 961: Consider rephrasing "We recognize that supporting Airflow..." to maintain a more formal documentation tone
- Line 973-974: The sentence appears incomplete, missing "Think of" at the start to match the similar note in other sections
Apply these changes to improve consistency:
-would typically take too long for a user to wait for them to complete within the app. +Would typically take too long for a user to wait for them to complete within the app. -We recognize that supporting Airflow or other workflow orchestrators can be an +Setting up Airflow or other workflow orchestrators can be an -FiftyOne Teams as the single source of truth on which you +Think of FiftyOne Teams as the single source of truth on which youdocs/source/teams/teams_plugins.rst (2)
357-358
: Consider enhancing the builtin orchestrator introduction.The introduction could be more explicit about the builtin orchestrator being the default and preferred choice. Consider adding a brief mention of its advantages, such as:
- Zero configuration required
- Seamless integration with FiftyOne Teams
- Built-in monitoring and management
-App that are executed on a connected workflow orchestrator like the provided -FiftyOne builtin orchestator. +App that are executed on a workflow orchestrator. FiftyOne Teams comes with a +builtin orchestrator that requires zero configuration and is ready to use out +of the box. Alternatively, you can connect to external orchestrators like +Apache Airflow.
440-444
: Enhance the builtin orchestrator configuration details.The section could benefit from:
- More details about the default worker configuration
- Clearer separation between builtin and additional orchestrator options
- Fixed punctuation
-FiftyOne Teams offers a builtin orchestrator which is configured as part of your -deployment. Three workers are provisioned by default, to get help scaling up -or down contact your Voxel51 support team. If you'd like to configure an additional -orchestrator, such as Airflow, continue reading below. +FiftyOne Teams offers a builtin orchestrator that comes pre-configured with your +deployment. By default, it provisions three workers that can handle multiple +concurrent operations. To adjust the number of workers based on your workload, +contact your Voxel51 support team. + +Advanced Configuration: +If you'd like to configure an additional orchestrator, such as Apache Airflow, +continue reading below.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
docs/source/index.rst
(1 hunks)docs/source/plugins/developing_plugins.rst
(1 hunks)docs/source/plugins/index.rst
(1 hunks)docs/source/plugins/using_plugins.rst
(2 hunks)docs/source/teams/teams_plugins.rst
(2 hunks)
🔇 Additional comments (3)
docs/source/index.rst (1)
378-379
: LGTM: Clear and accurate orchestrator reference
The update appropriately references the FiftyOne Teams Builtin Orchestrator, which aligns with the PR's objective to improve documentation clarity about this feature.
docs/source/plugins/using_plugins.rst (1)
834-834
: LGTM: Reference link addition
The added reference to the builtin orchestrator section is properly formatted and correctly links to the new content.
docs/source/plugins/developing_plugins.rst (1)
1132-1132
: Update reference to orchestrator
The change from Apache Airflow to "FiftyOne Teams Builtin Orchestrator" aligns with the PR objectives to update documentation about the builtin orchestrator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/source/teams/teams_plugins.rst (1)
357-358
: Fix typo and improve readabilityThere's a typo in "orchestator" and the sentence structure could be improved.
-App that are executed on a connected workflow orchestrator like the provided -FiftyOne builtin orchestator. +App that are executed on a connected workflow orchestrator, such as the provided +FiftyOne builtin orchestrator.docs/source/plugins/using_plugins.rst (1)
952-967
: Consider enhancing the builtin orchestrator sectionWhile the content effectively introduces the builtin orchestrator, consider adding:
- A brief overview of the key features or capabilities
- Examples of common use cases
- Any specific requirements or prerequisites
The note about contacting for FiftyOne Teams is well-placed and relevant.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
docs/source/plugins/using_plugins.rst
(2 hunks)docs/source/teams/teams_plugins.rst
(2 hunks)
🔇 Additional comments (2)
docs/source/teams/teams_plugins.rst (1)
440-442
: LGTM!
The information about the builtin orchestrator's default configuration and support options is clear and well-documented.
docs/source/plugins/using_plugins.rst (1)
834-834
: LGTM: Clear reference to the builtin orchestrator
The addition of the reference to the FiftyOne Teams Builtin Orchestrator enhances the documentation by providing users with information about this deployment option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more updates and now LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/source/teams/teams_plugins.rst (1)
432-433
: Fix RST heading format inconsistencyThe heading underline should match the length of the heading text for consistency with other RST headings in the document.
-Configuring your orchestrator(s) -________________________________ +Configuring your orchestrator(s) +_______________________________docs/source/plugins/using_plugins.rst (1)
874-917
: Improve RST structure consistency.The section structure could be improved for better maintainability:
- Use consistent section markers (either all dashes or all underscores).
- Fix indentation in the RST structure.
Apply this diff to improve the structure:
.. _delegated-orchestrator: Setting up an orchestrator -__________________________ +------------------------- .. _delegated-orchestrator-open-source: FiftyOne Open Source -------------------- +------------------ FiftyOne Open Source users can run delegated operations via the :ref:`fiftyone delegated launch <cli-fiftyone-delegated-launch>` CLI command: .. code-block:: shell fiftyone delegated launch This command starts a service that will continuously check for any queued delegated operations and execute them serially in its process. .. _delegated-orchestrator-teamas: FiftyOne Teams --------------- +-------------
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
docs/source/index.rst
(1 hunks)docs/source/plugins/developing_plugins.rst
(2 hunks)docs/source/plugins/index.rst
(1 hunks)docs/source/plugins/using_plugins.rst
(2 hunks)docs/source/release-notes.rst
(1 hunks)docs/source/teams/teams_plugins.rst
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/source/index.rst
- docs/source/plugins/index.rst
🔇 Additional comments (10)
docs/source/teams/teams_plugins.rst (4)
357-357
: LGTM: Clear description of execution environment
The updated text clearly explains that delegated operations are executed on a connected compute cluster.
435-441
: LGTM: Clear explanation of orchestrator options
The content effectively explains the builtin orchestrator while maintaining flexibility with external options.
453-457
: LGTM: Clear explanation of the Runs page and execution queue
The content effectively explains the Runs page functionality and the queue-based execution model.
461-462
: LGTM: Important clarification about operation tracking
The note clearly distinguishes between tracked (scheduled) and untracked (immediate) operations.
docs/source/plugins/using_plugins.rst (2)
832-840
: LGTM! Clear and concise introduction to delegated operations.
The explanation effectively communicates the purpose and benefits of delegated operations.
841-842
: Verify the cross-reference exists.
Please ensure that the :ref:
teams-delegated-operations`` target exists in the documentation.
✅ Verification successful
Cross-references are valid and correctly defined
Both referenced sections exist in the documentation:
teams-delegated-operations
is defined indocs/source/teams/teams_plugins.rst
delegated-orchestrator-open-source
is defined indocs/source/plugins/using_plugins.rst
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the referenced section label in RST files
rg ".. _teams-delegated-operations:" -t rst
Length of output: 113
Script:
#!/bin/bash
# Check if the referenced section exists in the current file
rg "delegated-orchestrator-open-source" -t rst
Length of output: 351
docs/source/plugins/developing_plugins.rst (3)
1131-1132
: LGTM: Clear explanation of delegated operations
The added text provides a clear and accurate explanation of how delegated operations can be used to schedule long-running tasks in the background while continuing to use the App.
1136-1141
: LGTM: Clear distinction between Teams and Open Source capabilities
The added text effectively clarifies the differences in delegated operation capabilities between:
- FiftyOne Teams: Out-of-the-box support with connected compute cluster
- FiftyOne Open Source: Small-scale support with local execution
3567-3567
: LGTM: Accurate reference to delegated execution
The added text correctly references the delegated execution feature, maintaining consistency with earlier documentation.
docs/source/release-notes.rst (1)
Line range hint 1-24
: LGTM! Release notes structure follows best practices
The release notes are well-organized with:
- Clear version numbering and release dates
- Consistent section headers (App, Core, Brain etc.)
- Proper reStructuredText formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changes locally, do not merge
Although if BM is good with it as exists now maybe I'll just throw my changes away and move on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, defer to product
What changes are proposed in this pull request?
Update docs to mention builtin orchestrator
What areas of FiftyOne does this PR affect?
fiftyone
Python library changesSummary by CodeRabbit
New Features
Bug Fixes
Documentation