Update docs for query performance mode #5070
Conversation
|
Warning Rate limit exceeded@CamronStaley has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
|
| @@ -57,7 +58,7 @@ const QueryPerformanceToast = ({ | |||
| throw new Error("no query performance element"); | |||
| } | |||
|
|
|||
| if (!shown || disabled) { | |||
| if (!shown || disabled || !queryPerformance) { | |||
There was a problem hiding this comment.
if QP mode is False, we won't be showing the toast 👍 maybe add a comment here to explain this logic
| :ref:`App config <configuring-fiftyone-app>`, or for a particular dataset by | ||
| clicking on the "Gear" icon above the sample grid in the App. | ||
|
|
||
| .. note:: | ||
|
|
||
| When lightning mode is enabled through the "Gear" icon in the App, the | ||
| When query performance mode is enabled through the "Gear" icon in the App, the |
There was a problem hiding this comment.
Do we want to keep Query Performance or query performance for consistency?
There was a problem hiding this comment.
Good point. Note the language can and may change after product feedback
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/packages/core/src/components/QueryPerformanceToast.tsx (1)
61-61: Add a comment explaining the queryPerformance conditionWhile the logic is correct, it would be helpful to add a comment explaining why we check for queryPerformance. This maintains code clarity for future developers.
+ // Only show the toast when query performance mode is enabled and relevant if (!shown || disabled || !queryPerformance) {docs/source/user_guide/app.rst (1)
Line range hint
615-630: Consider adding a note about index creation timingWhile the documentation about compound indexes for grouped datasets is accurate, it would be helpful to mention that creating indexes before adding samples is more efficient.
Consider adding a note like:
dataset = foz.load_zoo_dataset("quickstart-groups") + # Note: Creating indexes before adding samples is more efficient # Index a specific field dataset.create_index("ground_truth.detections.label") dataset.create_index([("group.name", 1), ("ground_truth.detections.label", 1)])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
docs/source/images/app/app-lightning-mode.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/app/app-query-performance-mode.gifis excluded by!**/*.gif,!**/*.gif
📒 Files selected for processing (4)
app/packages/core/src/components/QueryPerformanceToast.tsx(2 hunks)docs/source/user_guide/app.rst(6 hunks)docs/source/user_guide/config.rst(3 hunks)fiftyone/core/config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/components/QueryPerformanceToast.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.
🔇 Additional comments (8)
app/packages/core/src/components/QueryPerformanceToast.tsx (1)
41-41: LGTM: Clean state management implementation
The new Recoil state integration is well-implemented using the appropriate hook and follows React conventions.
fiftyone/core/config.py (2)
361-361: LGTM: Default query performance mode enabled by default
The change to enable query performance mode by default (True) aligns with the transition from "Lightning mode" to "Query Performance mode" and improves the out-of-the-box performance for users.
358-362: Verify the impact of removed lightning_threshold
The AI summary indicates that lightning_threshold was removed. Let's verify that all references to this attribute have been properly cleaned up.
✅ Verification successful
Let me gather more context about the lightning mode references to ensure we have a complete picture.
The removal of lightning_threshold is clean and safe
The codebase scan shows that while there are still references to "lightning" features (like the lightning resolver and lightning mode in the app), there are no remaining references to the specific lightning_threshold configuration parameter that was removed. The existing lightning-related code serves different functionality and is not impacted by this removal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to lightning_threshold
rg -i "lightning_threshold"
# Search for any remaining references to "Lightning mode" in documentation
rg -i "lightning.?mode"
Length of output: 724
Script:
#!/bin/bash
# Search for any remaining references to lightning in the app configuration
ast-grep --pattern 'lightning'
# Check for any references in Python files
rg -t py 'lightning'
# Look for any configuration related to lightning in JSON/YAML files
rg -t json -t yaml 'lightning'
Length of output: 2458
docs/source/user_guide/config.rst (1)
671-678: LGTM! Well-documented configuration options.
The new query performance configuration options are clearly documented with appropriate descriptions, environment variables, and default values. The cross-references to additional documentation are helpful.
docs/source/user_guide/app.rst (4)
522-528: LGTM: Clear introduction to Query Performance mode
The introduction effectively explains what Query Performance mode is and how it can be enabled. The documentation is clear and accurate.
534-535: LGTM: Clear persistence behavior note
The note clearly explains how the Query Performance mode setting persists in the browser for each dataset.
Line range hint 537-605: LGTM: Well-documented example with BDD100K dataset
The example effectively demonstrates:
- How to load a dataset
- How to create an index
- How to enable Query Performance mode
The code snippets and explanations are clear and accurate.
Line range hint 647-684: LGTM: Comprehensive coverage of wildcard indexing
The documentation thoroughly covers:
- Global wildcard indexing
- Field-specific wildcard indexing
- Important warnings about RAM usage
- Special considerations for video datasets
| "default_query_performance": true, | ||
| "disable_frame_filtering": false, | ||
| "enable_query_performance": true, |
There was a problem hiding this comment.
Fix duplicate configuration option in CLI example.
The disable_frame_filtering option appears twice in the CLI example output. Remove the duplicate entry to maintain consistency with the Python example.
"default_query_performance": true,
"disable_frame_filtering": false,
"enable_query_performance": true,
"loop_videos": false,
"media_fallback": false,
- "disable_frame_filtering": false,
"multicolor_keypoints": false,Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@CamronStaley, glad I could help!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
89f97d5
* update documentation for qp mode * update video * remove missed lightning threshold references * remove duplicate
What changes are proposed in this pull request?
Update docs for query performance mode
How is this patch tested? If it is not, please explain why.
local dev
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
Query Performancemode for optimizing query handling in the app.default_query_performanceandenable_query_performance.Bug Fixes
QueryPerformanceToastto show only when relevant data is available.Documentation