Documentation for the ExecutionStore #5144
Conversation
WalkthroughThe pull request introduces significant updates to 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: 0
🧹 Outside diff range and nitpick comments (3)
docs/source/plugins/developing_plugins.rst (3)
2438-2444: Consider adding more context about use casesThe introduction to the Execution Store could be enhanced by providing concrete examples of when to use it, such as:
- Caching expensive computations
- Storing user preferences
- Sharing data between panel instances
2463-2464: Add explanation about TTL behaviorThe TTL example should explain what happens when data expires:
- Is it automatically removed?
- Are there any events/callbacks when data expires?
- How to handle expired data in the code?
2481-2492: Clarify global store usageThe global store example would benefit from:
- Explaining when to use global store vs regular store
- Mentioning any performance implications
- Describing scope and visibility of global data
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
docs/source/plugins/developing_plugins.rst(1 hunks)
🔇 Additional comments (2)
docs/source/plugins/developing_plugins.rst (2)
2497-2502: Great note about store scoping!
The note about store scoping to dataset is clear and important for users to understand the lifecycle management.
2433-2502: Overall documentation quality is excellent
The new sections are well-integrated into the existing documentation with:
- Clear explanations
- Practical code examples
- Proper context and warnings
Minor suggestions for enhancement:
- Consider adding more inline comments in code examples to explain complex operations
- Add cross-references to related sections where appropriate
| @@ -101,19 +102,24 @@ def update_ttl(self, key: str, new_ttl: int) -> None: | |||
| """ | |||
| self._store_service.update_ttl(self.store_name, key, new_ttl) | |||
|
|
|||
| def get_ttl(self, key: str) -> Optional[int]: | |||
There was a problem hiding this comment.
Realized in testing that this method was broken
| @@ -19,6 +19,7 @@ | |||
| ) | |||
| from .utils import ProgressHandler, is_new | |||
| from .panel import Panel, PanelConfig | |||
| from .store import ExecutionStore | |||
There was a problem hiding this comment.
By convention, I prefer that anything that we're publicly documenting for users to be accessible via the highest reasonable package level. For example in this case:
# happy
from fiftyone.operators import ExecutionStore
# sad; too low-level for end users
from fiftyone.operators.store import ExecutionStore
| @@ -233,9 +221,7 @@ def has_store_global(self, store_name): | |||
| """Determines whether a store with the given name exists across all | |||
| datasets and the global context. | |||
| """ | |||
| result = self._collection.find_one( | |||
There was a problem hiding this comment.
Realized in testing that this method was broken
| @@ -78,33 +78,21 @@ def has_store(self, store_name) -> bool: | |||
|
|
|||
| def list_stores(self) -> list[str]: | |||
| """Lists the stores associated with the current context.""" | |||
| result = self._collection.find( | |||
There was a problem hiding this comment.
Realized in testing that this method was broken
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
fiftyone/operators/store/service.py (1)
Line range hint
52-60: Document the metadata parameter in create_store method.Given this PR's focus on ExecutionStore documentation, the
create_storemethod's docstring should document themetadataparameter.Here's the suggested docstring update:
def create_store( self, store_name: str, metadata: Optional[dict[str, Any]] = None ) -> StoreDocument: """Creates a new store with the specified name. Args: store_name: the name of the store + metadata: optional metadata to associate with the store Returns: a :class:`fiftyone.store.models.StoreDocument` """docs/source/plugins/developing_plugins.rst (5)
2457-2458: Consider adding TTL explanation.The concept of TTL (time-to-live) is used here but not explicitly explained. Consider adding a brief explanation of what TTL means and its implications for data persistence.
- store.set("my_key", {"foo": "bar"}, ttl=60) + # Store data that will automatically expire after 60 seconds + store.set("my_key", {"foo": "bar"}, ttl=60) # ttl is in seconds
2513-2516: Enhance global store warning.The warning about global stores could be more specific about the potential consequences of not using TTLs.
- Global stores have no automatic garbage collection, so take care when - creating and using global stores whose keys do not utilize TTLs. + Global stores have no automatic garbage collection. Keys without TTLs in global + stores will persist indefinitely and could lead to memory/storage issues over + time. Consider always using TTLs or implementing manual cleanup strategies.
Line range hint
2517-2533: Add cross-reference to panel data section.Consider adding a cross-reference to the panel data section for better navigation and understanding.
:ref:`Panel data <panel-data>` (which may be large), on the other hand, is *not* explicitly persisted. Instead it should be hydrated when the panel is -loaded using the pattern :ref:`demonstrated here <panel-data>`. +loaded using the pattern :ref:`demonstrated here <panel-data>`. For more details +about panel data management, see the :ref:`Panel Data <panel-data>` section.
Line range hint
2534-2550: Add security best practices note.Consider adding a note about security best practices when handling secrets.
def on_load(self, ctx): url = ctx.secrets["FIFTYONE_CVAT_URL"] username = ctx.secrets["FIFTYONE_CVAT_USERNAME"] password = ctx.secrets["FIFTYONE_CVAT_PASSWORD"] + +.. note:: + + Security best practices: + - Never log or display secrets in your panel + - Avoid storing secrets in panel state or data + - Use secrets only in secure API calls + - Handle missing secrets gracefully
Line range hint
2551-2580: Enhance common patterns overview.Consider expanding the introduction to provide more context about when to use each pattern.
Common patterns --------------- Most panels make use of common patterns like callbacks, menus, interactive -plots, and walkthrough layouts. +plots, and walkthrough layouts. These patterns serve different purposes: + +- **Callbacks**: For handling user interactions and events +- **Menus**: For organizing actions and navigation +- **Interactive Plots**: For data visualization and exploration +- **Walkthrough Layouts**: For guided user experiences and tutorials
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
docs/source/plugins/developing_plugins.rst(1 hunks)fiftyone/factory/repos/execution_store.py(2 hunks)fiftyone/operators/__init__.py(1 hunks)fiftyone/operators/executor.py(2 hunks)fiftyone/operators/store/service.py(1 hunks)fiftyone/operators/store/store.py(2 hunks)tests/unittests/execution_store_unit_tests.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/unittests/execution_store_unit_tests.py
🧰 Additional context used
🪛 Ruff
fiftyone/operators/__init__.py
22-22: .store.ExecutionStore imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (8)
fiftyone/operators/__init__.py (1)
22-22: LGTM! The import location follows our convention.
The high-level import location is perfect for end-user documentation, making it easily accessible via from fiftyone.operators import ExecutionStore.
🧰 Tools
🪛 Ruff
22-22: .store.ExecutionStore imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
fiftyone/operators/store/store.py (1)
9-9: LGTM: Clean datetime import
The datetime import is appropriately placed and follows Python import best practices by importing specifically what's needed.
fiftyone/operators/store/service.py (1)
66-66: LGTM! Explicit parameter passing improves code clarity.
Making the metadata parameter explicit in the method call enhances code readability and maintainability.
fiftyone/operators/executor.py (2)
880-887: LGTM! Improved store retrieval with better reliability.
The changes to the store method are well-implemented:
- The docstring now clearly describes the auto-creation behavior
- Using
dataset._doc.idinstead of dataset name provides better reliability as IDs are immutable - The implementation is more concise by leveraging
ExecutionStore.create
880-887: Verify backward compatibility of store method changes
Since this modifies existing behavior, we should verify that current usage patterns are preserved.
✅ Verification successful
Let me gather more information about the ExecutionStore tests and implementation to verify the auto-creation behavior.
Let me check one more thing to see the actual implementation of the store method and its auto-creation behavior.
Auto-creation behavior is properly tested and implemented
Based on the comprehensive test suite in execution_store_unit_tests.py and the implementation details, the store auto-creation behavior is well-tested and maintains backward compatibility:
- The
ExecutionStoreServiceIntegrationTestsclass includes specific tests for store creation - The implementation in
store/service.pyshows proper handling of store creation - Existing usage patterns in the codebase (found in docs and model evaluation code) align with this behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of the store method to verify compatibility
rg -A 3 "\.store\("
# Search for any tests related to ExecutionStore
rg -l "ExecutionStore.*test"
Length of output: 769
Script:
#!/bin/bash
# Check the test implementation for store creation
cat tests/unittests/execution_store_unit_tests.py
# Look for ExecutionStore implementation
ast-grep --pattern 'class ExecutionStore'
# Check for store method implementation in executor
ast-grep --pattern 'def store($$$)'
Length of output: 13285
Script:
#!/bin/bash
# Find the implementation of the store method in executor.py
rg -A 10 "def store\(" fiftyone/operators/executor.py
# Look for store creation logic
rg -A 5 "create_store" fiftyone/operators/store
Length of output: 1241
fiftyone/factory/repos/execution_store.py (3)
81-85: Efficient retrieval of unique store names using aggregation pipeline
The use of an aggregation pipeline to group by store_name enhances performance and ensures accurate retrieval of unique store names associated with the current context.
90-91: Accurate counting of stores using aggregation pipeline
By grouping and counting unique store_name entries, the aggregation pipeline provides an accurate count of stores associated with the current context.
225-225: Verify the impact of removing the key filter in has_store_global
Removing the key filter in has_store_global changes the method's behavior to check for any document with the given store_name, regardless of the key. Ensure that this aligns with the intended functionality and does not introduce unintended results.
Run the following script to identify usages of has_store_global and review their context:
✅ Verification successful
Let me gather more context about the implementation and usage of this store functionality.
Let me try a different approach to understand the store implementation.
Removing the key filter in has_store_global is correct and intentional
The change to remove the key filter in has_store_global is correct because:
- The method's purpose is to check for store existence across all contexts, not specific keys
- All other key-specific operations (
has_key,get_key,set_key, etc.) maintain their key filters - The test cases in
execution_store_service_tests.pyverify this behavior by only checking store existence without key consideration - The store implementation maintains a clear separation between store-level operations (global checks) and key-level operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all usages of `has_store_global` in the codebase.
rg 'has_store_global\(' -A 3 -B 3
Length of output: 2653
Script:
#!/bin/bash
# Search for the collection definition and store document structure
rg -B 5 -A 5 '_collection.*=.*MongoCollection'
# Search for other methods in the execution store that work with store_name
ast-grep --pattern 'def $_($$$) {
$$$
store_name
$$$
}'
Length of output: 112
Script:
#!/bin/bash
# Look for the store implementation and related methods
rg -B 10 -A 10 'class ExecutionStore'
# Search for methods that use store_name with key
rg 'store_name.*key' -B 3 -A 3
Length of output: 20811
| @@ -19,6 +19,7 @@ | |||
| ) | |||
| from .utils import ProgressHandler, is_new | |||
| from .panel import Panel, PanelConfig | |||
| from .store import ExecutionStore | |||
There was a problem hiding this comment.
Add ExecutionStore to all to expose it properly.
The ExecutionStore is imported but not included in the __all__ list, which means it won't be exposed when users do from fiftyone.operators import *. Since this is being documented for users, it should be properly exposed.
Add this line before the __all__ definition:
from .store import ExecutionStore
from .categories import Categories
+ __all__ = ["ExecutionStore"] # Add this before the dynamic __all__ computation
# This enables Sphinx refs to directly use paths imported here
__all__ = [k for k, v in globals().items() if not k.startswith("_")]Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff
22-22: .store.ExecutionStore imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/store/store.py (1)
9-9: Add type hints for metadata dictionary valuesWhile the
datetimeimport is used indirectly throughkey_docattributes, we should explicitly use it in type hints for better code clarity.-from datetime import datetime +from datetime import datetime from typing import Any, Optional +MetadataDict = dict[str, datetime]🧰 Tools
🪛 Ruff
9-9:
datetime.datetimeimported but unusedRemove unused import:
datetime.datetime(F401)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
fiftyone/operators/store/store.py(2 hunks)
🧰 Additional context used
🪛 Ruff
fiftyone/operators/store/store.py
9-9: datetime.datetime imported but unused
Remove unused import: datetime.datetime
(F401)
Documentation for the ExecutionStore
Summary by CodeRabbit