Added two env vars for enabling query performance #4917
Conversation
fiftyone/core/config.py
Outdated
| self.lightning_threshold = self.parse_int( | ||
| d, | ||
| "lightning_threshold", | ||
| env_var="FIFTYONE_APP_LIGHTNING_THRESHOLD", | ||
| default=None, | ||
| ) | ||
| self.enable_query_performance = self.parse_bool( | ||
| d, "enable_query_performance", | ||
| env_var="FIFTYONE_APP_ENABLE_QUERY_PERFORMANCE", default=True |
There was a problem hiding this comment.
Why are we defaulting to true for both enable and default?
Also the naming is super confusing. I have no idea what query performance true/false actually does without digging into implementation detail
There was a problem hiding this comment.
@kaixi-wang : do you have a suggestion for name change?
There was a problem hiding this comment.
FIFTYONE_ENABLE_QUERY_PERFORMANCE=true/false: whether to disable the entire query performance feature, including the panel, toasts, etc; default: true
FIFTYONE_DEFAULT_QUERY_PERFORMANCE=true/false: whether to enable query performance by default for each dataset (if query performance is enabled systemwide per first env var); default: true
minhtuev
force-pushed
the
feat/qp-env-config
branch
from
610e8b0
to
403b6a4
Compare
WalkthroughThe changes involve updates to various components and configurations related to query performance. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OptionsComponent
participant RecoilState
participant SubComponents
User->>OptionsComponent: Interacts with Options
OptionsComponent->>RecoilState: Check modal state
RecoilState-->>OptionsComponent: Return modal state
OptionsComponent->>SubComponents: Render based on state
SubComponents-->>OptionsComponent: Display appropriate components
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 (5)
app/packages/core/src/components/Actions/Options.tsx (1)
230-230: Approved: Terminology update from "Lightning mode" to "Query Performance mode"This change aligns well with the PR objectives and provides a more descriptive term for the feature.
Consider updating any related documentation or user guides to reflect this terminology change for consistency across the application.
app/schema.graphql (2)
Line range hint
1-5: LGTM: New histogram aggregation capability addedThe addition of the
histogramValuesfield to theAggregateinput type enhances the aggregation capabilities, allowing for histogram-based data analysis. This is a valuable feature for data visualization and analysis.Consider updating the documentation to explain the usage and benefits of this new histogram aggregation feature.
Line range hint
437-441: LGTM: New exclude field added to LightningPathInputThe addition of the
excludefield toLightningPathInputenhances the flexibility of lightning queries by allowing specific values to be excluded from the results. The included description clearly explains the field's purpose.Consider updating any related documentation or examples to demonstrate the usage of this new
excludefield in lightning queries.fiftyone/server/query.py (1)
378-379: LGTM! Consider updating documentation.The addition of
enable_query_performanceanddefault_query_performancefields aligns well with the PR objectives to introduce environment variables for controlling query performance mode. This change enhances the configurability of the application.Consider updating the relevant documentation to reflect these new configuration options and their impact on query performance. This will help users understand how to leverage these new features effectively.
fiftyone/core/config.py (1)
351-357: Enhance the deprecation notice forlightning_threshold.While it's good to see a comment explaining the reason for deprecation, consider adding more information to help users migrate:
- Add a deprecation warning using the
warningsmodule.- Provide information about when this attribute will be removed (e.g., in which version).
- If possible, add a brief explanation or link to documentation about the new timer-based approach.
Example:
import warnings # deprecate lightning threshold in favor of a timer-based approach self.lightning_threshold = self.parse_int( d, "lightning_threshold", env_var="FIFTYONE_APP_LIGHTNING_THRESHOLD", default=None, ) warnings.warn( "The 'lightning_threshold' attribute is deprecated and will be removed in version X.Y. " "Use the new timer-based approach instead. See [DOC_LINK] for more information.", DeprecationWarning, stacklevel=2 )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
app/packages/app/src/pages/__generated__/IndexPageQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/fragments/__generated__/configFragment.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/fragments/__generated__/estimatedCountsFragment.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**,!**/__generated__/**
📒 Files selected for processing (5)
- app/packages/core/src/components/Actions/Options.tsx (1 hunks)
- app/packages/relay/src/fragments/configFragment.ts (1 hunks)
- app/schema.graphql (8 hunks)
- fiftyone/core/config.py (1 hunks)
- fiftyone/server/query.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/components/Actions/Options.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/relay/src/fragments/configFragment.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 (11)
app/packages/relay/src/fragments/configFragment.ts (1)
14-15: LGTM: New fields align with PR objectivesThe addition of
enableQueryPerformanceanddefaultQueryPerformancefields to theconfigobject in theconfigFragmentis well-implemented and aligns perfectly with the PR objectives. These new fields will allow for better control over query performance modes, as intended.app/packages/core/src/components/Actions/Options.tsx (2)
232-232: Approved: Consistent terminology update in titleThis change maintains consistency with the previous update, ensuring a coherent user interface.
Line range hint
1-424: Summary: Terminology update enhances clarityThe changes in this file are focused on updating the terminology from "Lightning mode" to "Query Performance mode". This update aligns with the PR objectives and improves clarity for users without altering the component's functionality.
To ensure consistency across the codebase, please run the following script to check for any remaining instances of "Lightning mode" that may need updating:
app/schema.graphql (6)
7-7: LGTM: Improved formatting for union typesThe formatting changes to
AggregateResultandAggregationResponsesunion types improve readability and maintain consistency in the schema definition. These changes don't affect functionality and are a good practice for schema maintenance.Also applies to: 31-31
473-485: LGTM: Improved formatting for mutation definitionsThe formatting changes to the
Mutationtype definitions improve readability and maintain a consistent style across the schema. These changes don't affect functionality and contribute to better schema maintenance.
509-511: LGTM: Improved formatting for query definitionsThe formatting changes to the
Querytype definitions enhance readability and maintain a consistent style across the schema. These changes are cosmetic and don't impact functionality, contributing to better schema maintenance.Also applies to: 520-521, 526-526
447-447: LGTM: Improved formatting for LightningResults union typeThe formatting change to the
LightningResultsunion type improves readability and maintains a consistent style across the schema. This change is cosmetic and doesn't affect functionality.
229-229: Change in nullability for estimatedSampleCount fieldThe
estimatedSampleCountfield in theDatasettype has been changed from non-nullable (Int!) to nullable (Int). This change provides more flexibility when the count might not be available or calculated.Ensure that client-side code is updated to handle potential null values for this field. Run the following script to identify potential usage of this field:
#!/bin/bash # Description: Check for estimatedSampleCount usage in client-side code # Search for estimatedSampleCount in JavaScript/TypeScript files rg --type js 'estimatedSampleCount'
39-40: LGTM: New query performance configuration fields addedThe addition of
enableQueryPerformanceanddefaultQueryPerformancefields to theAppConfigtype aligns with the PR objective of introducing environment variables for query performance control. These required fields will ensure consistent configuration across the application.Please verify that existing queries and mutations involving the
AppConfigtype have been updated to include these new required fields. Run the following script to check for potential issues:✅ Verification successful
LGTM: Verified Query Performance Configuration Fields
The addition of
enableQueryPerformanceanddefaultQueryPerformancefields to theAppConfigtype has been successfully verified. No existing queries or mutations were found that require updates to include these new required fields.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for AppConfig usage in queries and mutations # Search for AppConfig in GraphQL operations rg --type graphql 'AppConfig.*\{' -A 10Length of output: 35
Script:
#!/bin/bash # Description: Search for all usages of AppConfig in GraphQL operations # Search for any occurrence of AppConfig in GraphQL files rg --type graphql 'AppConfig' -A 10Length of output: 67
Script:
#!/bin/bash # Description: Search for all usages of AppConfig in GraphQL operations # Search for any occurrence of AppConfig in .graphql and .gql files rg 'AppConfig' --glob '*.graphql' --glob '*.gql' -A 10Length of output: 2242
fiftyone/server/query.py (1)
378-379: Verify handling of new fields in related codeWhile the addition of
enable_query_performanceanddefault_query_performancefields toAppConfigis straightforward, it's important to ensure that these new fields are properly handled in related parts of the code.Please run the following script to check if the
Queryclass and any serialization/deserialization logic forAppConfighave been updated to handle these new fields:If the script doesn't return any results, consider updating the
Queryclass and any serialization/deserialization logic to handle the new fields.fiftyone/core/config.py (1)
358-365:⚠️ Potential issueReview query performance attributes and consider renaming.
The addition of
enable_query_performanceanddefault_query_performanceattributes is noted. However, there are some concerns:
Both attributes default to
True. Consider if this is the desired default behavior, especially forenable_query_performance. Enabling new performance features by default might affect existing systems.The naming of
default_query_performanceis ambiguous. It's not clear what a boolean value represents for "default" performance. Consider a more descriptive name that clearly indicates its purpose, such asuse_high_performance_query_modeoroptimize_query_performance.Add comments explaining the purpose and impact of these attributes to help users understand how they affect the system's behavior.
To verify the impact and usage of these new attributes, let's search for their usage in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
fiftyone/core/config.py (1)
351-357: Consider adding more details about the deprecation oflightning_threshold.While it's good to mark
lightning_thresholdas deprecated, it would be helpful to provide more information:
- Add a deprecation warning using the
warningsmodule.- Include information about when this attribute will be removed.
- Provide guidance on what users should use instead (the mentioned "timer-based approach").
This will help users prepare for the future removal of this attribute and understand how to migrate their code.
Here's a suggested implementation:
import warnings # At the top of the file, add: from fiftyone.core.config import TIMER_BASED_APPROACH_DOCS_URL # Assuming this constant exists # Then modify the existing code: warnings.warn( "The 'lightning_threshold' attribute is deprecated and will be removed in version X.Y. " f"Use the new timer-based approach instead. See {TIMER_BASED_APPROACH_DOCS_URL} for details.", DeprecationWarning, stacklevel=2 ) self.lightning_threshold = self.parse_int( d, "lightning_threshold", env_var="FIFTYONE_APP_LIGHTNING_THRESHOLD", default=None, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/schema.graphql (3 hunks)
- fiftyone/core/config.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/schema.graphql
🧰 Additional context used
| self.enable_query_performance = self.parse_bool( | ||
| d, | ||
| "enable_query_performance", | ||
| env_var="FIFTYONE_APP_ENABLE_QUERY_PERFORMANCE", | ||
| default=True, | ||
| ) | ||
| self.default_query_performance = self.parse_bool( | ||
| d, | ||
| "default_query_performance", | ||
| env_var="FIFTYONE_APP_DEFAULT_QUERY_PERFORMANCE", | ||
| default=True, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider naming and default values for new query performance attributes.
The addition of enable_query_performance and default_query_performance is a good step towards improving query performance. However, there are a few points to consider:
-
The naming of these attributes could be more descriptive. For example,
enable_query_performance_optimizationanduse_query_performance_optimization_by_defaultwould be clearer. -
Both attributes default to
True, which might change behavior for existing users. Consider defaultingdefault_query_performancetoFalseto maintain backwards compatibility. -
The purpose and interaction of these two attributes are not immediately clear from their names. Consider adding comments to explain their roles and how they affect query performance.
Here's a suggested implementation with improved naming and comments:
# Enable the query performance optimization feature
self.enable_query_performance_optimization = self.parse_bool(
d,
"enable_query_performance_optimization",
env_var="FIFTYONE_APP_ENABLE_QUERY_PERFORMANCE_OPTIMIZATION",
default=True,
)
# Use query performance optimization by default when enabled
self.use_query_performance_optimization_by_default = self.parse_bool(
d,
"use_query_performance_optimization_by_default",
env_var="FIFTYONE_APP_USE_QUERY_PERFORMANCE_OPTIMIZATION_BY_DEFAULT",
default=False, # Default to False for backwards compatibility
)Also, consider adding documentation for these new attributes to help users understand their purpose and usage.
What changes are proposed in this pull request?
Added two env vars that control the default behaviors for query performance mode
How is this patch tested? If it is not, please explain why.
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
New Features
Bug Fixes
Documentation
Chores