[Proposal] rename ctx.create_store to ctx.store #4954
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 40 files out of 122 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThe changes involve significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor
participant Store
User->>Executor: Call store(store_name)
Executor->>Executor: Check if store exists
alt Store does not exist
Executor->>Store: Create scoped store with specified name
end
Executor->>User: Return scoped store
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 (2)
fiftyone/operators/executor.py (2)
Line range hint
850-864: New methodstorelooks good, but consider adding error handling.The new
storemethod is well-implemented and provides a clear improvement over the deprecatedcreate_storemethod. It creates a scoped store name, which helps prevent naming conflicts between different operators. However, there are a couple of suggestions for improvement:
- Consider adding error handling in case the
ExecutionStore.createmethod fails.- The docstring could be more detailed, explaining the scoping of the store name and any potential side effects.
Here's a suggested improvement with error handling and an expanded docstring:
def store(self, store_name): """ Create (if not previously created) and use a store with the specified name. This method creates a scoped store name by prefixing the given store_name with the operator's URI. This ensures unique store names across different operators. Args: store_name: the name of the store Returns: a :class:`fiftyone.operators.store.ExecutionStore` Raises: RuntimeError: If the store creation fails """ from fiftyone.operators.store import ExecutionStore scoped_store_name = f"{self._operator_uri}_{store_name}" try: return ExecutionStore.create(scoped_store_name) except Exception as e: raise RuntimeError(f"Failed to create store '{scoped_store_name}': {str(e)}")
867-879: Deprecation ofcreate_storemethod is handled correctly.The deprecation of the
create_storemethod is implemented correctly. It provides a clear message to users about using the newstoremethod instead. The implementation also ensures backward compatibility by calling the newstoremethod.Consider adding a deprecation warning to make it more visible to users:
import warnings def create_store(self, store_name): """ DEPRECATED: use `ctx.store('my_store')` instead. Creates a new store with the specified name. Args: store_name: the name of the store Returns: a :class:`fiftyone.operators.store.ExecutionStore` """ warnings.warn( "The 'create_store' method is deprecated. Use 'store' method instead.", DeprecationWarning, stacklevel=2 ) return self.store(store_name)
imanjra
force-pushed
the
chore/store-tweaks
branch
from
16919f5 to
7e64a0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/executor.py (1)
864-877: LGTM! Well-handled deprecation.The
create_storemethod is correctly marked as deprecated and redirects to the newstoremethod. This approach maintains backward compatibility while encouraging users to switch to the new API.Consider adding a deprecation warning using the
warningsmodule to provide a more visible alert to users still using this method. For example:import warnings def create_store(self, store_name): warnings.warn( "The 'create_store' method is deprecated. Use 'ctx.store(store_name)' instead.", DeprecationWarning, stacklevel=2 ) return self.store(store_name)This will emit a warning when the method is called, making it easier for developers to identify and update deprecated code usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- fiftyone/operators/executor.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
fiftyone/operators/executor.py (1)
Line range hint
850-857: LGTM! Improved store creation and usage.The new
storemethod is a well-designed replacement for the deprecatedcreate_store. It provides a more intuitive API by combining the creation and usage of a store in a single method. The implementation is concise and correct.
imanjra
force-pushed
the
chore/store-tweaks
branch
from
7e64a0d to
6be85f3
Compare
imanjra
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/executor.py (1)
Line range hint
875-883: LGTM! Consider adding a minor clarification to the docstring.The implementation of the
storemethod looks good. It correctly creates (if not already created) and uses a store with the specified name, returning anExecutionStoreobject. This aligns with the PR objective of replacing the deprecatedcreate_storemethod.Consider adding a brief explanation of what a "store" is in the context of FiftyOne operations to the docstring. This would improve clarity for developers who might be new to the concept. For example:
def store(self, store_name): """ Create (if not previously created) and use a store with the specified name. + + A store in FiftyOne is a persistent key-value storage mechanism used for + caching data across multiple operator executions. Args: store_name: the name of the store Returns: a :class:`fiftyone.operators.store.ExecutionStore` """
imanjra
force-pushed
the
chore/store-tweaks
branch
from
6be85f3 to
8cbe320
Compare
What changes are proposed in this pull request?
Add
ctx.storeand deprecatects.create_storeHow is this patch tested? If it is not, please explain why.
(Details)
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
storemethod for enhanced store management, allowing for scoped store names based on operator URI.create_storemethod as deprecated, guiding users to transition to the new method.Bug Fixes
set_progressmethod to improve error handling when progress is set without a defined function.Documentation
storemethod's documentation to reflect its new functionality and usage.create_storemethod to provide transition guidance.