-
Notifications
You must be signed in to change notification settings - Fork 557
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
Execution Store #4827
Execution Store #4827
Conversation
WalkthroughThe changes introduce a new execution store repository within the FiftyOne framework, enhancing the management of execution stores using MongoDB. Key components include the addition of the Changes
Possibly related PRs
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
|
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: 12
Outside diff range and nitpick comments (1)
tests/unittests/execution_store_tests.py (1)
251-266
: Add assertions to validate operation outcomes.The test currently checks that
insert_one
is called with specific arguments, but it does not validate the behavior after the operation. Consider adding assertions to verify the state of the system or any return values.Add assertions like:
# Verify that the operation result is as expected self.assertIsNotNone(result)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- fiftyone/factory/repo_factory.py (2 hunks)
- fiftyone/factory/repos/execution_store.py (1 hunks)
- fiftyone/operators/executor.py (1 hunks)
- fiftyone/operators/store/init.py (1 hunks)
- fiftyone/operators/store/models.py (1 hunks)
- fiftyone/operators/store/permissions.py (1 hunks)
- fiftyone/operators/store/service.py (1 hunks)
- fiftyone/operators/store/store.py (1 hunks)
- tests/unittests/execution_store_tests.py (1 hunks)
Additional context used
Ruff
fiftyone/operators/store/permissions.py
8-8: Redefinition of unused
Dict
from line 6Remove definition:
Dict
(F811)
8-8: Redefinition of unused
List
from line 6Remove definition:
List
(F811)
8-8: Redefinition of unused
Optional
from line 6Remove definition:
Optional
(F811)
9-9: Redefinition of unused
BaseModel
from line 5Remove definition:
BaseModel
(F811)
fiftyone/operators/store/service.py
10-10:
traceback
imported but unusedRemove unused import:
traceback
(F401)
fiftyone/operators/store/store.py
10-10:
fiftyone.factory.repo_factory.RepositoryFactory
imported but unusedRemove unused import:
fiftyone.factory.repo_factory.RepositoryFactory
(F401)
tests/unittests/execution_store_tests.py
9-9:
time
imported but unusedRemove unused import:
time
(F401)
11-11:
unittest.mock
imported but unusedRemove unused import:
unittest.mock
(F401)
12-12:
unittest.mock.patch
imported but unusedRemove unused import:
unittest.mock.patch
(F401)
14-14:
fiftyone
imported but unusedRemove unused import:
fiftyone
(F401)
15-15:
bson.ObjectId
imported but unusedRemove unused import:
bson.ObjectId
(F401)
23-23: Redefinition of unused
ExecutionStoreService
from line 18Remove definition:
ExecutionStoreService
(F811)
Additional comments not posted (33)
fiftyone/operators/store/__init__.py (1)
1-20
: LGTM!The
__init__.py
file is well-structured and follows the standard pattern for exposing the public API of a Python package. The docstring provides a clear description of the module, and the__all__
list is properly defined to include the relevant classes and service.The imports are organized and sourced from the appropriate submodules, maintaining a clean separation of concerns.
Overall, the code changes in this file are good to go!
fiftyone/operators/store/permissions.py (2)
12-17
: LGTM!The
StorePermissions
class structure looks good. The fields for roles, users, and plugins permissions are appropriately typed as optional dictionaries with default empty dictionary values. This allows flexibility in defining permissions for a store.
19-26
: LGTM!The
default
static method is a good addition for easy initialization ofStorePermissions
with default values. Providing full access (read and write) to the admin role by default is a reasonable choice. The method correctly returns aStorePermissions
instance.fiftyone/factory/repo_factory.py (2)
16-19
: LGTM!The new import statements for
ExecutionStoreRepo
andMongoExecutionStoreRepo
are necessary and follow the existing code style.
52-65
: Excellent addition of theexecution_store_repo()
factory method!The new static method follows the factory pattern and is consistent with the existing
delegated_operation_repo()
method. It ensures efficient management ofMongoExecutionStoreRepo
instances by reusing existing instances when possible.The method signature, docstring, and implementation logic are all properly defined.
fiftyone/operators/store/models.py (2)
10-33
: LGTM!The
KeyDocument
class is well-defined with appropriate fields for representing a key in the store. The optional TTL field and the permissions dictionary provide flexibility in managing key expiration and access control. The example schema in theConfig
class is helpful for understanding the model's usage.
36-57
: LGTM!The
StoreDocument
class effectively represents a store in the execution store by inheriting fromKeyDocument
and overriding thekey
andvalue
fields. Setting thekey
field to a constant value ensures a consistent identifier for store documents. The optionalvalue
field allows storing additional metadata for the store. The example schema in theConfig
class demonstrates a well-structured permissions system for roles, users, groups, and plugins, enabling fine-grained access control.fiftyone/factory/repos/execution_store.py (5)
1-3
: LGTM!The docstring provides a clear and concise description of the module's purpose.
9-13
: LGTM!The
_where
helper function correctly constructs a query dictionary based on the providedstore_name
and optionalkey
. It will be useful for querying the execution store repository.
16-64
: LGTM!The
ExecutionStoreRepo
class provides a solid foundation for execution store repositories. The methods cover the essential operations for managing stores and keys, such as creating stores, setting keys, getting keys, updating TTLs, deleting keys, and deleting stores. The use ofStoreDocument
andKeyDocument
models ensures consistent data structure and validation.
67-70
: LGTM!The
MongoExecutionStoreRepo
class correctly inherits fromExecutionStoreRepo
and sets theCOLLECTION_NAME
attribute to"execution_store"
. This class will provide a MongoDB-specific implementation of the execution store repository.
72-73
: LGTM!The
__init__
method correctly initializes theMongoExecutionStoreRepo
instance by calling the parent class constructor with the providedcollection
parameter. This ensures that the_collection
attribute is properly set in the parent class.fiftyone/operators/store/store.py (9)
17-20
: LGTM!The
create
method correctly instantiates and returns a newExecutionStore
instance with the providedstore_name
and a newExecutionStoreService
instance.
22-29
: LGTM!The constructor method correctly initializes the
ExecutionStore
instance with the providedstore_name
andstore_service
. The docstring provides a clear description of the parameters.
31-43
: LGTM!The
get
method correctly retrieves the value associated with the providedkey
from the store using theget_key
method of theExecutionStoreService
instance. It handles the case when the key is not found by returningNone
. The docstring provides a clear description of the parameters and return value.
45-53
: LGTM!The
set
method correctly sets thevalue
associated with the providedkey
in the store using theset_key
method of theExecutionStoreService
instance. It also accepts an optionalttl
parameter to set the time-to-live for the key. The docstring provides a clear description of the parameters.
55-61
: LGTM!The
delete
method correctly deletes the key from the store using thedelete_key
method of theExecutionStoreService
instance. The docstring provides a clear description of the parameter.
63-72
: LGTM!The
has
method correctly checks the existence of the providedkey
in the store using thehas_key
method of theExecutionStoreService
instance. It returns a boolean indicating whether the key exists. The docstring provides a clear description of the parameter and return value.
74-76
: LGTM!The
clear
method correctly clears all the data in the store using theclear_store
method of theExecutionStoreService
instance. The docstring provides a clear description of the method's functionality.
78-85
: LGTM!The
update_ttl
method correctly updates the TTL for the specifiedkey
using theupdate_ttl
method of theExecutionStoreService
instance. The docstring provides a clear description of the parameters.
87-96
: LGTM!The
get_ttl
method correctly retrieves the TTL for the specifiedkey
using theget_ttl
method of theExecutionStoreService
instance. It returns the TTL in milliseconds orNone
if the key does not have a TTL. The docstring provides a clear description of the parameter and return value.fiftyone/operators/store/service.py (9)
20-24
: LGTM!The
__init__
method correctly initializes the_repo
attribute based on the providedrepo
or creates a new one using theRepositoryFactory
.
26-39
: LGTM!The
create_store
method correctly creates a new store using the_repo
with the providedstore_name
andpermissions
(or default permissions if not provided).
41-55
: LGTM!The
set_key
method correctly sets the value of a key in the specified store using the_repo
with the providedstore_name
,key
,value
, andttl
.
57-67
: LGTM!The
get_key
method correctly retrieves the value of a key from the specified store using the_repo
with the providedstore_name
andkey
.
69-79
: LGTM!The
delete_key
method correctly deletes the specified key from the store using the_repo
with the providedstore_name
andkey
.
81-94
: LGTM!The
update_ttl
method correctly updates the TTL of the specified key in the store using the_repo
with the providedstore_name
,key
, andnew_ttl
.
96-108
: LGTM!The
set_permissions
method correctly sets the permissions for the specified store using the_repo
with the providedstore_name
andpermissions
.
110-119
: LGTM!The
list_stores
method correctly lists all stores matching the given criteria using the_repo
with the providedsearch
criteria and additional keyword arguments.
121-130
: LGTM!The
delete_store
method correctly deletes the specified store using the_repo
with the providedstore_name
.fiftyone/operators/executor.py (1)
833-845
: LGTM!The
create_store
method implementation looks good:
- It correctly imports the
ExecutionStore
class.- Creates a new store by calling
ExecutionStore.create()
with the providedstore_name
.- Returns the created store instance.
tests/unittests/execution_store_tests.py (2)
259-265
: Verify arguments ininsert_one
assertion.In the
test_operator_execute_set_key
method, ensure that the data passed toinsert_one
includes all necessary fields, such as TTL if applicable.Review the
insert_one
call to confirm that all essential fields are included. If TTL is a required field, consider adding it to the mocked data and the assertion.
158-158
: Align parameter name inupdate_ttl
method call.After renaming the parameter in
MockStoreRepo
, ensure that the method call matches the new parameter name for clarity.Apply this diff to update the method call:
-self.svc.update_ttl(store_name=store_name, key=key, new_ttl=new_ttl) +self.svc.update_ttl(store_name=store_name, key=key, new_ttl=new_ttl)Note: If you prefer to keep the parameter name as
ttl
, update the method call accordingly.Likely invalid or redundant comment.
bf5b35b
to
19857d3
Compare
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: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- fiftyone/factory/repo_factory.py (2 hunks)
- fiftyone/operators/store/init.py (1 hunks)
- fiftyone/operators/store/models.py (1 hunks)
- fiftyone/operators/store/service.py (1 hunks)
- tests/unittests/execution_store_tests.py (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- fiftyone/factory/repo_factory.py
- fiftyone/operators/store/init.py
- fiftyone/operators/store/models.py
- fiftyone/operators/store/service.py
Additional context used
Ruff
tests/unittests/execution_store_tests.py
9-9:
time
imported but unusedRemove unused import:
time
(F401)
11-11:
unittest.mock
imported but unusedRemove unused import:
unittest.mock
(F401)
12-12:
unittest.mock.patch
imported but unusedRemove unused import:
unittest.mock.patch
(F401)
19-19: Redefinition of unused
ExecutionStoreService
from line 15Remove definition:
ExecutionStoreService
(F811)
Additional comments not posted (1)
tests/unittests/execution_store_tests.py (1)
9-9
: Remove unused importtime
.The
time
module is imported but not used in the code. Consider removing it to clean up the imports.Apply this diff to remove the unused import:
-import time
Likely invalid or redundant comment.
Tools
Ruff
9-9:
time
imported but unusedRemove unused import:
time
(F401)
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: 3
Outside diff range and nitpick comments (1)
tests/unittests/execution_store_tests.py (1)
146-240
: Integration tests forExecutionStore
.The
TestExecutionStoreIntegration
class provides integration tests for the main operations of theExecutionStore
, such as setting a key, getting a key, listing keys, deleting a key, and clearing the store. The tests use a mocked collection and assert the expected interactions with the mocked collection, ensuring the correctness of the store's operations.Inquiry about the commented-out test.
I noticed that there is a commented-out test for updating a key (lines 173-196). Is this functionality not yet implemented or is there a reason for not including this test?
If you need any assistance with implementing or testing the update functionality, please let me know, and I'll be happy to help.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- fiftyone/operators/store/models.py (1 hunks)
- fiftyone/operators/store/service.py (1 hunks)
- fiftyone/operators/store/store.py (1 hunks)
- tests/unittests/execution_store_tests.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- fiftyone/operators/store/models.py
- fiftyone/operators/store/service.py
Additional context used
Ruff
fiftyone/operators/store/store.py
10-10:
fiftyone.factory.repo_factory.RepositoryFactory
imported but unusedRemove unused import:
fiftyone.factory.repo_factory.RepositoryFactory
(F401)
tests/unittests/execution_store_tests.py
11-11:
unittest.mock
imported but unusedRemove unused import:
unittest.mock
(F401)
12-12:
unittest.mock.patch
imported but unusedRemove unused import
(F401)
12-12:
unittest.mock.ANY
imported but unusedRemove unused import
(F401)
15-15:
fiftyone.operators
imported but unusedRemove unused import:
fiftyone.operators
(F401)
17-17:
fiftyone.operators.store.models.StoreDocument
imported but unusedRemove unused import:
fiftyone.operators.store.models.StoreDocument
(F401)
20-20: Redefinition of unused
ExecutionStoreService
from line 16Remove definition:
ExecutionStoreService
(F811)
Additional comments not posted (13)
fiftyone/operators/store/store.py (11)
18-20
: LGTM!The
create
method correctly instantiates anExecutionStore
with the providedstore_name
and an instance ofExecutionStoreService
.
22-29
: LGTM!The constructor method correctly initializes the
ExecutionStore
instance with the providedstore_name
andstore_service
.
31-43
: LGTM!The
get
method correctly retrieves the value associated with the providedkey
from the store using theget_key
method ofself._store_service
. It handles the case when the key is not found by returningNone
.
45-53
: LGTM!The
set
method correctly sets thevalue
in the store under the providedkey
with an optionalttl
using theset_key
method ofself._store_service
.
55-64
: Verify the return type ofdelete_key
.The
delete
method correctly deletes the providedkey
from the store using thedelete_key
method ofself._store_service
.However, the docstring mentions that the method returns a boolean indicating whether the key was deleted, but the method directly returns the result of
delete_key
without any explicit boolean conversion. Please verify that thedelete_key
method ofExecutionStoreService
indeed returns a boolean to ensure consistency with the docstring.
66-75
: LGTM!The
has
method correctly checks if the providedkey
exists in the store using thehas_key
method ofself._store_service
and returns the result as a boolean.
77-79
: LGTM!The
clear
method correctly clears all the data in the store by calling thedelete_store
method ofself._store_service
withself.store_name
.
81-88
: LGTM!The
update_ttl
method correctly updates the TTL for the providedkey
with thenew_ttl
value by calling theupdate_ttl
method ofself._store_service
with the appropriate arguments.
90-99
: LGTM!The
get_ttl
method correctly retrieves the TTL for the providedkey
by calling theget_ttl
method ofself._store_service
with the appropriate arguments and returns the result as an integer orNone
.
101-107
: LGTM!The
list_keys
method correctly retrieves a list of all keys in the store by calling thelist_keys
method ofself._store_service
withself.store_name
and returns the result as a list of strings.
10-10
: Remove unused import.The following comment from the previous review is still applicable:
coderabbitai[bot]: Remove unused import.
The import statement
from fiftyone.factory.repo_factory import RepositoryFactory
is unused in the code. Please remove it to keep the code clean and avoid potential confusion.Apply this diff to remove the unused import:
-from fiftyone.factory.repo_factory import RepositoryFactory
Please address this issue to maintain code cleanliness.
Tools
Ruff
10-10:
fiftyone.factory.repo_factory.RepositoryFactory
imported but unusedRemove unused import:
fiftyone.factory.repo_factory.RepositoryFactory
(F401)
tests/unittests/execution_store_tests.py (2)
25-46
: LGTM!The
IsDateTime
class andassert_delta_seconds_approx
function are well-defined and serve a specific purpose in the test suite. The custom equality check fordatetime.datetime
instances and the helper for asserting approximate time differences can be useful in time-related tests.
49-144
: Comprehensive integration tests forExecutionStoreService
.The
ExecutionStoreServiceIntegrationTests
class provides a comprehensive set of integration tests for theExecutionStoreService
. The tests cover various operations such as setting a key, getting a key, creating a store, deleting a key, updating TTL, deleting a store, and listing keys.The use of a mocked collection allows testing the service without relying on an actual database, and the assertions verify that the service interacts with the mocked collection as expected. The tests are well-structured and provide good coverage for the
ExecutionStoreService
.
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: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- fiftyone/factory/repos/execution_store.py (1 hunks)
- fiftyone/operators/store/models.py (1 hunks)
- fiftyone/operators/store/service.py (1 hunks)
- fiftyone/operators/store/store.py (1 hunks)
- tests/unittests/execution_store_tests.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- fiftyone/operators/store/models.py
- fiftyone/operators/store/service.py
Additional context used
Ruff
fiftyone/operators/store/store.py
10-10:
fiftyone.factory.repo_factory.RepositoryFactory
imported but unusedRemove unused import:
fiftyone.factory.repo_factory.RepositoryFactory
(F401)
tests/unittests/execution_store_tests.py
11-11:
unittest.mock
imported but unusedRemove unused import:
unittest.mock
(F401)
12-12:
unittest.mock.patch
imported but unusedRemove unused import
(F401)
12-12:
unittest.mock.ANY
imported but unusedRemove unused import
(F401)
15-15:
fiftyone.operators
imported but unusedRemove unused import:
fiftyone.operators
(F401)
17-17:
fiftyone.operators.store.models.StoreDocument
imported but unusedRemove unused import:
fiftyone.operators.store.models.StoreDocument
(F401)
20-20: Redefinition of unused
ExecutionStoreService
from line 16Remove definition:
ExecutionStoreService
(F811)
Additional comments not posted (13)
fiftyone/operators/store/store.py (1)
10-10
: Remove unused import.The import statement
from fiftyone.factory.repo_factory import RepositoryFactory
is unused in the code. Please remove it to keep the code clean and avoid potential confusion.Apply this diff to remove the unused import:
-from fiftyone.factory.repo_factory import RepositoryFactory
Likely invalid or redundant comment.
Tools
Ruff
10-10:
fiftyone.factory.repo_factory.RepositoryFactory
imported but unusedRemove unused import:
fiftyone.factory.repo_factory.RepositoryFactory
(F401)
fiftyone/factory/repos/execution_store.py (7)
1-14
: LGTM!The module docstring and the private helper function
_where
are well-implemented.
- The docstring accurately describes the purpose of the module.
- The
_where
function constructs a query dictionary based on the providedstore_name
and optionalkey
arguments, which is a common pattern for building MongoDB queries.- The function is marked as private using the leading underscore convention, indicating that it is intended for internal use within the module.
17-31
: LGTM!The
ExecutionStoreRepo
class and its__init__
andcreate_store
methods are well-implemented.
- The class serves as a base class for execution store repositories, providing common functionality.
- The
__init__
method initializes the class with a MongoDB collection, allowing it to work with a specific collection.- The
create_store
method creates a new store document with the providedstore_name
and optionalpermissions
.- It uses the
StoreDocument
model to create a new document instance and inserts it into the MongoDB collection using theinsert_one
method.- The created
StoreDocument
instance is returned, which is a good practice for allowing further operations on the created document.
33-37
: LGTM!The
list_stores
method is well-implemented.
- It retrieves all unique store names from the MongoDB collection using the
distinct
method.- By specifying the "store_name" field as the argument to
distinct
, it ensures that only the store names are returned, without any duplicates.- The method returns the distinct store names as a list of strings, which is a suitable format for further processing or displaying to the user.
38-69
: LGTM!The
set_key
method is well-implemented and handles both updating existing keys and inserting new ones.
- It uses the
KeyDocument
model to create a new document instance with the providedstore_name
,key
,value
, and the current timestamp.- The update operations are prepared using the
$set
and$setOnInsert
operators, which allows for updating existing fields and setting fields only on insert.- The upsert operation is performed using the
update_one
method with theupsert
parameter set toTrue
, ensuring that a new document is inserted if it doesn't exist.- The
_where
helper function is used to construct the query based on thestore_name
andkey
, promoting code reuse and readability.- The method handles the case of a new document being inserted by setting the
created_at
field, and updates theupdated_at
field for existing documents.- The updated or inserted
KeyDocument
instance is returned, providing flexibility for further operations.
71-75
: LGTM!The
get_key
method is well-implemented and retrieves a key document from the specified store.
- It uses the
find_one
method of the MongoDB collection to retrieve a single document that matches the providedstore_name
andkey
.- The
_where
helper function is used to construct the query, promoting code reuse and readability.- If a matching document is found, it is parsed into a
KeyDocument
instance using theKeyDocument
model.- If no matching document is found,
None
is returned, indicating the absence of the requested key.- The method returns either a
KeyDocument
instance orNone
, providing a clear indication of the presence or absence of the requested key.
77-80
: LGTM!The
list_keys
method is well-implemented and lists all keys in the specified store.
- It uses the
find
method of the MongoDB collection to retrieve all documents that match the providedstore_name
.- The
_where
helper function is used to construct the query, promoting code reuse and readability.- The
{"key": 1}
projection is used to retrieve only the "key" field of the matching documents, optimizing the query performance by fetching only the required data.- A list comprehension is used to extract the "key" value from each document in the
keys
cursor, providing a concise and efficient way to obtain the list of keys.- The method returns the list of keys, allowing for further processing or display to the user.
82-128
: LGTM!The
update_ttl
,delete_key
, anddelete_store
methods of theExecutionStoreRepo
class, as well as theMongoExecutionStoreRepo
class, are well-implemented.
update_ttl
method:
- It updates the TTL (Time-To-Live) of a specific key in the store.
- The expiration timestamp is calculated based on the provided
ttl
using theget_expiration
method of theKeyDocument
model.- The
update_one
method is used to update the "expires_at" field of the document that matches thestore_name
andkey
.- The method returns a boolean indicating whether the update modified any documents, providing feedback on the success of the TTL update.
delete_key
method:
- It deletes a specific key from the store.
- The
delete_one
method is used to delete the document that matches thestore_name
andkey
.- The method returns a boolean indicating whether the delete operation deleted any documents, providing feedback on the success of the key deletion.
delete_store
method:
- It deletes an entire store and all its associated keys.
- The
delete_many
method is used to delete all documents that match thestore_name
.- The method returns the count of deleted documents, providing information on the number of keys that were removed.
MongoExecutionStoreRepo
class:
- It is a MongoDB-specific implementation of the execution store repository.
- The class inherits from the
ExecutionStoreRepo
class, leveraging its methods and functionality.- The
__init__
method is overridden to call the parent class's__init__
method and then create the necessary indexes by calling the_create_indexes
method.- The
_create_indexes
method ensures thetests/unittests/execution_store_tests.py (5)
25-27
: LGTM!The
IsDateTime
class is a simple and effective way to check if an object is an instance ofdatetime.datetime
. The implementation is correct and concise.
30-31
: LGTM!The
assert_delta_seconds_approx
function is a useful utility for asserting that a time delta is approximately equal to a given number of seconds within a specified epsilon. The implementation is correct and straightforward.
34-46
: LGTM!The
TeskKeyDocument
class contains two well-written test methods that cover theget_expiration
method of theKeyDocument
class. The tests assert the correct behavior for both non-None and None TTL values.
49-155
: LGTM!The
ExecutionStoreServiceIntegrationTests
class contains a comprehensive set of test methods that cover various functionalities of theExecutionStoreService
andExecutionStoreRepo
classes. The tests are well-structured, use appropriate mocks, and make thorough assertions to ensure the correctness of the tested methods.
157-228
: LGTM!The
TestExecutionStoreIntegration
class contains a comprehensive set of test methods that cover various functionalities of theExecutionStore
class. The tests are well-structured, use appropriate mocks, and make thorough assertions to ensure the correctness of the tested methods.
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.
Marking as requested changes so a pydantic
discussion happens
5e12362
to
9f9182b
Compare
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: 20
🧹 Outside diff range and nitpick comments (5)
fiftyone/operators/store/models.py (1)
31-35
: StoreDocument structure looks good, consider adding a docstring to the 'value' fieldThe
StoreDocument
class is well-structured and appropriately inherits fromKeyDocument
. The constantkey
value and optionalvalue
field are good design choices.To improve clarity, consider adding a docstring to the
value
field explaining its purpose and potential contents:class StoreDocument(KeyDocument): """Model representing a Store.""" key: str = "__store__" value: Optional[Dict[str, Any]] = None """Optional metadata or configuration for the store."""This addition would provide more context about the purpose of the
value
field inStoreDocument
.fiftyone/factory/repo_factory.py (1)
52-65
: LGTM: Newexecution_store_repo()
method is well-implemented.The new
execution_store_repo()
method is correctly implemented and follows the existing pattern in the class. It properly uses the singleton pattern to ensure only one instance ofMongoExecutionStoreRepo
is created and reused.Consider expanding the docstring to provide more information about the method's purpose and return value. For example:
""" Factory method for creating or retrieving an execution store repository. Returns: ExecutionStoreRepo: An instance of the execution store repository. """fiftyone/operators/executor.py (1)
850-862
: LGTM! Consider adding a return type hint.The
create_store
method is well-implemented and documented. Here are some suggestions for improvement:
Add a return type hint to enhance code readability:
def create_store(self, store_name) -> ExecutionStore:Consider addressing the TODO comment about resolving the circular import in the future. This might involve restructuring the imports or using a type comment if the circular import cannot be resolved easily.
fiftyone/operators/store/service.py (2)
90-96
: Remove reference to criteria inlist_stores
docstringThe docstring for the
list_stores
method mentions "matching the given criteria," but the method does not accept any criteria parameters.Suggested Edit:
def list_stores(self): - """Lists all stores matching the given criteria. + """Lists all stores. Returns: a list of :class:`fiftyone.store.models.StoreDocument` """ return self._repo.list_stores()
109-115
: Add a Returns section to the docstring oflist_keys
methodThe
list_keys
method is missing aReturns
section in its docstring. Adding this section improves consistency and clarifies what the method returns.Suggested Addition:
def list_keys(self, store_name): """Lists all keys in the specified store. Args: store_name: the name of the store + Returns: + a list of :class:`fiftyone.store.models.KeyDocument` """ return self._repo.list_keys(store_name)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- fiftyone/factory/repo_factory.py (2 hunks)
- fiftyone/factory/repos/execution_store.py (1 hunks)
- fiftyone/operators/executor.py (1 hunks)
- fiftyone/operators/store/init.py (1 hunks)
- fiftyone/operators/store/models.py (1 hunks)
- fiftyone/operators/store/service.py (1 hunks)
- fiftyone/operators/store/store.py (1 hunks)
- tests/unittests/execution_store_tests.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- fiftyone/operators/store/init.py
🧰 Additional context used
🪛 Ruff
fiftyone/operators/store/store.py
10-10:
fiftyone.factory.repo_factory.RepositoryFactory
imported but unusedRemove unused import:
fiftyone.factory.repo_factory.RepositoryFactory
(F401)
tests/unittests/execution_store_tests.py
11-11:
unittest.mock
imported but unusedRemove unused import:
unittest.mock
(F401)
12-12:
unittest.mock.patch
imported but unusedRemove unused import
(F401)
12-12:
unittest.mock.ANY
imported but unusedRemove unused import
(F401)
15-15:
fiftyone.operators
imported but unusedRemove unused import:
fiftyone.operators
(F401)
17-17:
fiftyone.operators.store.models.StoreDocument
imported but unusedRemove unused import:
fiftyone.operators.store.models.StoreDocument
(F401)
20-20: Redefinition of unused
ExecutionStoreService
from line 16Remove definition:
ExecutionStoreService
(F811)
🔇 Additional comments (5)
fiftyone/operators/store/models.py (2)
1-35
: Reconsider the use of Pydantic and explore standard library alternativesThe use of Pydantic for model definitions in this file introduces a new dependency that other parts of the project have intentionally avoided. While Pydantic offers robust features, it's worth considering whether the benefits outweigh the cost of adding a new dependency.
Consider refactoring these models to use
dataclasses
from the Python standard library, which can provide similar functionality without introducing a new dependency. This approach would align better with the rest of the project and simplify the dependency management.Here's a high-level example of how this refactoring might look:
from dataclasses import dataclass, field from typing import Optional, Dict, Any import datetime @dataclass class KeyDocument: store_name: str key: str value: Any created_at: datetime.datetime = field(default_factory=datetime.datetime.now) updated_at: Optional[datetime.datetime] = None expires_at: Optional[datetime.datetime] = None @staticmethod def get_expiration(ttl: Optional[int]) -> Optional[datetime.datetime]: # ... (same implementation as before) @dataclass class StoreDocument(KeyDocument): key: str = "__store__" value: Optional[Dict[str, Any]] = NoneThis refactoring maintains the structure and functionality of the models while eliminating the Pydantic dependency. If additional validation or serialization features are needed, consider implementing them using standard Python techniques or exploring lightweight alternatives that are already in use within the project.
To assess the impact of removing Pydantic, we can check its usage across the project:
#!/bin/bash # Search for Pydantic usage across the project rg --type python 'from pydantic import|import pydantic'This will help determine if Pydantic is used elsewhere or if this is an isolated case.
1-8
: 🛠️ Refactor suggestionConsider alternatives to Pydantic and enhance the docstring
The use of Pydantic for model definitions has been a point of discussion in previous reviews. Consider using
dataclasses
instead, as it's part of the Python standard library and doesn't introduce additional dependencies. This aligns with the approach taken in other parts of the project.The file-level docstring could be more informative. Consider expanding it to briefly explain the purpose of the
KeyDocument
andStoreDocument
models and their role in the execution store.Here's a suggested improvement for the docstring:
""" Store and key models for the execution store. This module defines two main classes: - KeyDocument: Represents a key-value pair in the execution store. - StoreDocument: Represents a store entity, extending KeyDocument. These models provide a structured approach to managing data in the execution store. """To check if Pydantic is used elsewhere in the project:
✅ Verification successful
Pydantic usage is appropriately confined to
models.py
The verification confirms that Pydantic is only utilized within
fiftyone/operators/store/models.py
. This localized usage is suitable for defining robust data models without introducing unnecessary dependencies across the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of Pydantic in the project rg --type python 'from pydantic import'Length of output: 88
Script:
#!/bin/bash # Search for other uses of Pydantic in the project rg --type py 'from pydantic import|import pydantic'Length of output: 126
fiftyone/factory/repo_factory.py (2)
16-19
: LGTM: New imports are correctly added.The new imports for
ExecutionStoreRepo
andMongoExecutionStoreRepo
are correctly added and are consistent with the existing import style in the file. These imports are necessary for the newexecution_store_repo()
method.
Line range hint
1-65
: Summary: New execution store repository functionality added successfully.The changes to this file successfully introduce the new execution store repository functionality. The implementation is consistent with the existing code patterns and follows good practices. The new
execution_store_repo()
method integrates well with theRepositoryFactory
class and provides a clean way to manageExecutionStoreRepo
instances.These changes align with the PR objectives of introducing a new
ExecutionStore
component based on the existingDelegationOpsSvc()
. The implementation seems solid and ready for integration with the rest of the system.To ensure the new functionality is properly integrated, please run the following verification script:
This script will help verify that the new
ExecutionStoreRepo
is being properly imported and used throughout the codebase, and identify any remaining TODOs related to the implementation.✅ Verification successful
ExecutionStore repository integration verified successfully.
The new
ExecutionStoreRepo
is properly integrated and utilized within the codebase:
RepositoryFactory.execution_store_repo()
is used infiftyone/operators/store/service.py
.No TODOs related to
ExecutionStore
were found, indicating the implementation is complete and requires no further actions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of ExecutionStoreRepo in the codebase # Test 1: Check for imports of ExecutionStoreRepo echo "Checking for imports of ExecutionStoreRepo:" rg --type python "from fiftyone\.factory\.repos\.execution_store import ExecutionStoreRepo" # Test 2: Check for usage of RepositoryFactory.execution_store_repo() echo "Checking for usage of RepositoryFactory.execution_store_repo():" rg --type python "RepositoryFactory\.execution_store_repo\(\)" # Test 3: Check for any TODOs related to ExecutionStore echo "Checking for TODOs related to ExecutionStore:" rg --type python "TODO.*ExecutionStore"Length of output: 628
Script:
#!/bin/bash # Description: Verify the usage of ExecutionStoreRepo in the codebase using file extension patterns # Test 1: Check for imports of ExecutionStoreRepo echo "Checking for imports of ExecutionStoreRepo:" rg -g "*.py" "from fiftyone\.factory\.repos\.execution_store import ExecutionStoreRepo" # Test 2: Check for usage of RepositoryFactory.execution_store_repo() echo "Checking for usage of RepositoryFactory.execution_store_repo():" rg -g "*.py" "RepositoryFactory\.execution_store_repo\(\)" # Test 3: Check for any TODOs related to ExecutionStore echo "Checking for TODOs related to ExecutionStore:" rg -g "*.py" "TODO.*ExecutionStore"Length of output: 619
fiftyone/operators/store/store.py (1)
10-10
: Unused importRepositoryFactory
is still present.The previous review comment about the unused import
from fiftyone.factory.repo_factory import RepositoryFactory
is still applicable.🧰 Tools
🪛 Ruff
10-10:
fiftyone.factory.repo_factory.RepositoryFactory
imported but unusedRemove unused import:
fiftyone.factory.repo_factory.RepositoryFactory
(F401)
"expires_at": time.time() + 60000, | ||
} | ||
value = self.store.get("widget_1") | ||
assert value == {"name": "Widget One", "value": 100} |
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.
🛠️ Refactor suggestion
Use self.assertEqual
instead of plain assert
.
Replace assert value == {"name": "Widget One", "value": 100}
with self.assertEqual(value, {"name": "Widget One", "value": 100})
for clearer test assertions.
Apply this diff:
- assert value == {"name": "Widget One", "value": 100}
+ self.assertEqual(value, {"name": "Widget One", "value": 100})
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
assert value == {"name": "Widget One", "value": 100} | |
self.assertEqual(value, {"name": "Widget One", "value": 100}) |
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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- fiftyone/operators/store/service.py (1 hunks)
🔇 Additional comments (9)
fiftyone/operators/store/service.py (9)
1-15
: LGTM: Imports and logger setup.The imports are relevant to the functionality of the
ExecutionStoreService
class, and the logger is set up correctly. The previously mentioned unusedtraceback
import has been removed, addressing the past review comment.
18-26
: LGTM: Well-structured ExecutionStoreService class.The
ExecutionStoreService
class is well-designed with a clear purpose. The constructor correctly initializes the repo, allowing for dependency injection or using a default repo. The class provides a comprehensive set of methods for managing execution store operations.
27-36
: LGTM: create_store method.The
create_store
method is well-documented and correctly implemented. It provides a clear interface for creating a new store, delegating the actual creation to the underlying repository.
38-54
: LGTM: set_key method, but verify TTL consistency.The
set_key
method is well-implemented and documented. It correctly handles the optional TTL parameter in seconds. However, we need to verify the consistency of TTL units with theupdate_ttl
method when we review it later.
56-66
: LGTM: get_key method.The
get_key
method is well-documented and correctly implemented. It provides a clear interface for retrieving a key's value from a specified store, delegating the actual retrieval to the underlying repository.
68-78
: LGTM: delete_key method.The
delete_key
method is well-documented and correctly implemented. It provides a clear interface for deleting a key from a specified store, delegating the actual deletion to the underlying repository. The boolean return value appropriately indicates the success or failure of the operation.
97-103
: LGTM: list_stores method.The
list_stores
method is well-documented and correctly implemented. It provides a clear interface for retrieving all stores, delegating the actual listing to the underlying repository. The method signature and documentation are consistent with its purpose of listing all stores without any filtering criteria.
105-114
: LGTM: delete_store method.The
delete_store
method is well-documented and correctly implemented. It provides a clear interface for deleting a specified store, delegating the actual deletion to the underlying repository. The return type ofStoreDocument
is appropriate, likely representing the deleted store's information.
116-125
: LGTM: list_keys method.The
list_keys
method is well-documented and correctly implemented. It provides a clear interface for retrieving all keys in a specified store, delegating the actual listing to the underlying repository. The return type ofList[str]
is appropriate for representing a list of keys.
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: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- fiftyone/operators/store/service.py (1 hunks)
- fiftyone/operators/store/store.py (1 hunks)
- tests/unittests/execution_store_tests.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fiftyone/operators/store/service.py
- fiftyone/operators/store/store.py
🧰 Additional context used
🪛 Ruff
tests/unittests/execution_store_tests.py
11-11:
unittest.mock.patch
imported but unusedRemove unused import
(F401)
11-11:
unittest.mock.ANY
imported but unusedRemove unused import
(F401)
18-18: Redefinition of unused
ExecutionStoreService
from line 14Remove definition:
ExecutionStoreService
(F811)
🔇 Additional comments (2)
tests/unittests/execution_store_tests.py (2)
1-226
: Overall, the test suite provides good coverage of the ExecutionStore functionality.The tests cover various scenarios and edge cases for the ExecutionStore and related components. However, there are a few areas for improvement:
- Consider adding more edge cases, such as testing with empty stores or invalid inputs.
- Ensure consistent use of
unittest
assertion methods throughout the test suite.- Add docstrings to test methods to clearly describe what each test is verifying.
- Consider using parameterized tests for similar test cases with different inputs.
Implementing these suggestions will further enhance the robustness and maintainability of the test suite.
🧰 Tools
🪛 Ruff
11-11:
unittest.mock.patch
imported but unusedRemove unused import
(F401)
11-11:
unittest.mock.ANY
imported but unusedRemove unused import
(F401)
18-18: Redefinition of unused
ExecutionStoreService
from line 14Remove definition:
ExecutionStoreService
(F811)
124-125
:⚠️ Potential issueFix time delta calculation in
test_update_ttl
.The current calculation of
time_delta
may result in a negative value ifactual_expires_at
is later thanexpected_expiration
. To ensure a positive time delta, reverse the subtraction order.Apply this change to correct the calculation:
- time_delta = expected_expiration - actual_expires_at + time_delta = actual_expires_at - expected_expiration assert_delta_seconds_approx(time_delta, 0, epsilon=0.0001)Likely invalid or redundant comment.
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: 5
🧹 Outside diff range and nitpick comments (3)
tests/unittests/execution_store_tests.py (1)
9-11
: Remove unused imports to improve code cleanliness.The
patch
andANY
imports fromunittest.mock
are not used in the code. Consider removing them to keep the import statements clean.Apply this diff to remove the unused imports:
import time import unittest -from unittest.mock import patch, MagicMock, ANY, Mock +from unittest.mock import MagicMock, Mock🧰 Tools
🪛 Ruff
11-11:
unittest.mock.patch
imported but unusedRemove unused import
(F401)
11-11:
unittest.mock.ANY
imported but unusedRemove unused import
(F401)
fiftyone/operators/store/store.py (1)
13-13
: Remove unused logger instanceThe
logger
is instantiated but never used in this class. Consider removing it to keep the code clean.Apply this diff to remove the unused logger:
-logger = logging.getLogger(__name__)
fiftyone/operators/store/service.py (1)
46-53
: Ensure consistent parameter documentation in docstringsThe parameter types and default values in the docstrings are not consistently specified. To enhance clarity and maintain consistency, consider explicitly stating the parameter types and default values using standard conventions.
For example, update the
ttl
parameter in theset_key
method:Args: store_name: the name of the store key: the key to set value: the value to set - ttl (None): an optional TTL in seconds + ttl (Optional[int], default=None): an optional TTL in seconds Returns: a :class:`fiftyone.store.models.KeyDocument`Similarly, review other method docstrings to ensure parameter types and default values are clearly specified.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- fiftyone/factory/repos/execution_store.py (1 hunks)
- fiftyone/operators/store/service.py (1 hunks)
- fiftyone/operators/store/store.py (1 hunks)
- tests/unittests/execution_store_tests.py (1 hunks)
🧰 Additional context used
🪛 Ruff
fiftyone/operators/store/service.py
19-19: Undefined name
ExecutionStoreRepo
(F821)
tests/unittests/execution_store_tests.py
11-11:
unittest.mock.patch
imported but unusedRemove unused import
(F401)
11-11:
unittest.mock.ANY
imported but unusedRemove unused import
(F401)
🔇 Additional comments (8)
tests/unittests/execution_store_tests.py (2)
1-225
: Overall, well-structured and comprehensive test suite.The test suite covers a wide range of scenarios for the execution store functionality, including key operations, store management, and integration tests. The use of mocking is appropriate for isolating the tests from external dependencies.
A few minor improvements have been suggested to enhance code quality:
- Removing unused imports
- Correcting a class name typo
- Using unittest assertion methods consistently
- Fixing a time delta calculation
- Ensuring consistency in timestamp handling
After addressing these points, the test suite will be even more robust and maintainable. Great job on creating a thorough set of tests!
🧰 Tools
🪛 Ruff
11-11:
unittest.mock.patch
imported but unusedRemove unused import
(F401)
11-11:
unittest.mock.ANY
imported but unusedRemove unused import
(F401)
123-124
:⚠️ Potential issueCorrect the time delta calculation in
test_update_ttl
.The calculation of
time_delta
asexpected_expiration - actual_expires_at
may result in a negative value ifactual_expires_at
is later thanexpected_expiration
. To ensure a positive time delta, reverse the subtraction.Apply this diff to correct the calculation:
- time_delta = expected_expiration - actual_expires_at + time_delta = actual_expires_at - expected_expirationThis will make
time_delta
positive ifactual_expires_at
is later thanexpected_expiration
.Likely invalid or redundant comment.
fiftyone/operators/store/store.py (2)
52-61
: LGTMThe
set
method is implemented correctly, and the TTL handling is consistent across methods.
38-51
: Implementation ofget
method is correctThe
get
method correctly retrieves the value for a given key and returnsNone
if the key is not found.fiftyone/factory/repos/execution_store.py (4)
10-14
: Previous comment still applies: Use type hints for_where
function
25-29
: Previous comment still applies: Add error handling for database insertion increate_store
method
76-80
: Previous comment still applies: Add type hints to theget_key
method parameters and return type
113-131
: Previous comment still applies: Correct index existence checks in_create_indexes
method
{"store_name": "widgets", "key": "widget_2"}, | ||
] | ||
keys = self.store_repo.list_keys("widgets") | ||
assert keys == ["widget_1", "widget_2"] |
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.
🛠️ Refactor suggestion
Use unittest assertion methods consistently.
Replace plain assert
statements with appropriate unittest
assertion methods for better error messages and test output.
Apply these changes throughout the test classes:
- assert keys == ["widget_1", "widget_2"]
+ self.assertEqual(keys, ["widget_1", "widget_2"])
- assert stores == ["widgets", "gadgets"]
+ self.assertEqual(stores, ["widgets", "gadgets"])
- assert value == {"name": "Widget One", "value": 100}
+ self.assertEqual(value, {"name": "Widget One", "value": 100})
- assert keys == ["widget_1", "widget_2"]
+ self.assertEqual(keys, ["widget_1", "widget_2"])
- assert deleted
+ self.assertTrue(deleted)
Also applies to: 149-149, 193-193, 205-205, 214-214
"created_at": time.time(), | ||
"updated_at": time.time(), | ||
"expires_at": time.time() + 60000, |
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.
🛠️ Refactor suggestion
Use datetime.datetime.now()
instead of time.time()
.
For consistency with other parts of the code that use datetime
objects, consider using datetime.datetime.now()
for timestamp fields.
Apply this diff:
- "created_at": time.time(),
- "updated_at": time.time(),
- "expires_at": time.time() + 60000,
+ "created_at": datetime.datetime.now(),
+ "updated_at": datetime.datetime.now(),
+ "expires_at": datetime.datetime.now() + datetime.timedelta(seconds=60000),
This change ensures consistency in timestamp handling across the codebase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
"created_at": time.time(), | |
"updated_at": time.time(), | |
"expires_at": time.time() + 60000, | |
"created_at": datetime.datetime.now(), | |
"updated_at": datetime.datetime.now(), | |
"expires_at": datetime.datetime.now() + datetime.timedelta(seconds=60000), |
class TeskKeyDocument(unittest.TestCase): | ||
def test_get_expiration(self): | ||
ttl = 1 | ||
now = datetime.datetime.now() | ||
expiration = KeyDocument.get_expiration(ttl) | ||
time_delta = expiration - now | ||
assert_delta_seconds_approx(time_delta, ttl) | ||
assert isinstance(expiration, datetime.datetime) | ||
|
||
def test_get_expiration_none(self): | ||
ttl = None | ||
expiration = KeyDocument.get_expiration(ttl) | ||
assert expiration is None |
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.
Correct class name typo and improve assertions.
- There's a typo in the class name: "Tesk" should be "Test".
- Use
unittest
assertion methods instead of plainassert
statements for better error messages.
Apply these changes to improve the test class:
-class TeskKeyDocument(unittest.TestCase):
+class TestKeyDocument(unittest.TestCase):
def test_get_expiration(self):
ttl = 1
now = datetime.datetime.now()
expiration = KeyDocument.get_expiration(ttl)
time_delta = expiration - now
- assert_delta_seconds_approx(time_delta, ttl)
- assert isinstance(expiration, datetime.datetime)
+ self.assertAlmostEqual(time_delta.total_seconds(), ttl, delta=EPSILON)
+ self.assertIsInstance(expiration, datetime.datetime)
def test_get_expiration_none(self):
ttl = None
expiration = KeyDocument.get_expiration(ttl)
- assert expiration is None
+ self.assertIsNone(expiration)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
class TeskKeyDocument(unittest.TestCase): | |
def test_get_expiration(self): | |
ttl = 1 | |
now = datetime.datetime.now() | |
expiration = KeyDocument.get_expiration(ttl) | |
time_delta = expiration - now | |
assert_delta_seconds_approx(time_delta, ttl) | |
assert isinstance(expiration, datetime.datetime) | |
def test_get_expiration_none(self): | |
ttl = None | |
expiration = KeyDocument.get_expiration(ttl) | |
assert expiration is None | |
class TestKeyDocument(unittest.TestCase): | |
def test_get_expiration(self): | |
ttl = 1 | |
now = datetime.datetime.now() | |
expiration = KeyDocument.get_expiration(ttl) | |
time_delta = expiration - now | |
self.assertAlmostEqual(time_delta.total_seconds(), ttl, delta=EPSILON) | |
self.assertIsInstance(expiration, datetime.datetime) | |
def test_get_expiration_none(self): | |
ttl = None | |
expiration = KeyDocument.get_expiration(ttl) | |
self.assertIsNone(expiration) |
class ExecutionStoreService: | ||
"""Service for managing execution store operations.""" | ||
|
||
def __init__(self, repo: Optional["ExecutionStoreRepo"] = None): |
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.
Resolve undefined 'ExecutionStoreRepo' in type hint
The type hint for repo
in the __init__
method references ExecutionStoreRepo
, but ExecutionStoreRepo
is imported inside the method. This causes an undefined name error during static analysis (line 19). To fix this issue, import ExecutionStoreRepo
under an if TYPE_CHECKING:
block at the top of the file.
Apply this diff to resolve the error:
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from fiftyone.factory.repo_factory import ExecutionStoreRepo
+
import logging
from typing import Optional, List
from fiftyone.operators.store.models import StoreDocument, KeyDocument
This ensures that the type hint is recognized by type checkers without affecting the runtime imports.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
def __init__(self, repo: Optional["ExecutionStoreRepo"] = None): | |
from typing import TYPE_CHECKING | |
if TYPE_CHECKING: | |
from fiftyone.factory.repo_factory import ExecutionStoreRepo | |
import logging | |
from typing import Optional, List | |
from fiftyone.operators.store.models import StoreDocument, KeyDocument | |
def __init__(self, repo: Optional["ExecutionStoreRepo"] = None): |
🧰 Tools
🪛 Ruff
19-19: Undefined name
ExecutionStoreRepo
(F821)
update_fields = { | ||
"$set": { | ||
k: v | ||
for k, v in key_doc.to_mongo_dict().items() | ||
if k | ||
not in {"_id", "created_at", "expires_at", "store_name", "key"} | ||
}, | ||
"$setOnInsert": { | ||
"store_name": store_name, | ||
"key": key, | ||
"created_at": now, | ||
"expires_at": expiration if ttl else None, | ||
}, | ||
} |
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.
Potential issue: TTL is not updated when the key exists in set_key
method
When updating an existing key using set_key
, the expires_at
field is not updated because it's excluded from the $set
operation. As a result, the TTL remains unchanged, even if a new ttl
value is provided.
To ensure that the TTL is updated when a new ttl
is provided, consider including expires_at
in the $set
fields by removing it from the exclusion list.
Apply this diff to fix the issue:
update_fields = {
"$set": {
k: v
for k, v in key_doc.to_mongo_dict().items()
if k
- not in {"_id", "created_at", "expires_at", "store_name", "key"}
+ not in {"_id", "created_at", "store_name", "key"}
},
"$setOnInsert": {
"store_name": store_name,
"key": key,
"created_at": now,
- "expires_at": expiration if ttl else None,
},
+ }
+ if ttl is not None:
+ update_fields["$set"]["expires_at"] = expiration
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
update_fields = { | |
"$set": { | |
k: v | |
for k, v in key_doc.to_mongo_dict().items() | |
if k | |
not in {"_id", "created_at", "expires_at", "store_name", "key"} | |
}, | |
"$setOnInsert": { | |
"store_name": store_name, | |
"key": key, | |
"created_at": now, | |
"expires_at": expiration if ttl else None, | |
}, | |
} | |
update_fields = { | |
"$set": { | |
k: v | |
for k, v in key_doc.to_mongo_dict().items() | |
if k | |
not in {"_id", "created_at", "store_name", "key"} | |
}, | |
"$setOnInsert": { | |
"store_name": store_name, | |
"key": key, | |
"created_at": now, | |
}, | |
} | |
if ttl is not None: | |
update_fields["$set"]["expires_at"] = expiration |
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.
Tested with using the store in panel
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.
🍨
We'll have to test this in teams context - I am thinking about potential store/key name conflicts between different users. but might not be a big deal
What changes are proposed in this pull request?
The new
ExecutionStore
including several abstractions.This was based on the
DelegationOpsSvc()
.How is this patch tested? If it is not, please explain why.
There are two suites: unit and integration tests, plus an example python panel in fiftyone-plugins.
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?
fiftyone
Python library changesSummary by CodeRabbit
Release Notes
New Features
Tests
ExecutionStoreService
and related functionalities to ensure reliability and correctness.