refactor: remove pydantic from execution_store #4877
Conversation
WalkthroughThe pull request introduces several modifications across three files related to the handling of store documents and key documents. In Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutionStoreRepo
participant MongoDB
Client->>ExecutionStoreRepo: create_store(store_name, value)
ExecutionStoreRepo->>MongoDB: insert(to_mongo_dict())
Client->>ExecutionStoreRepo: set_key(store_name, key, value, ttl)
ExecutionStoreRepo->>MongoDB: update($set: to_mongo_dict(exclude_keys))
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
|
| @@ -24,10 +24,8 @@ def __init__(self, collection: Collection): | |||
|
|
|||
| def create_store(self, store_name, permissions=None) -> StoreDocument: | |||
There was a problem hiding this comment.
Note: Going to remove permissions ins a follow up commit
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- fiftyone/factory/repos/execution_store.py (2 hunks)
- fiftyone/operators/store/models.py (2 hunks)
- fiftyone/operators/store/store.py (0 hunks)
💤 Files with no reviewable changes (1)
- fiftyone/operators/store/store.py
🔇 Additional comments (3)
fiftyone/factory/repos/execution_store.py (3)
27-28: Initialization ofStoreDocumentand MongoDB insertion updated correctlyThe
StoreDocumentis now initialized withstore_name=store_name, value=permissions, and the insertion usesstore_doc.to_mongo_dict(). This change aligns with the refactoring from Pydantic models to dataclasses and ensures proper serialization for MongoDB insertion.
41-45: Includingexpires_atinKeyDocumentinitializationAdding
expires_at=expirationto theKeyDocumentinitialization ensures that key expiration is correctly handled. This modification supports the proper management of TTL (time-to-live) for keys in the execution store.
50-55: Constructing$setupdate fields withto_mongo_dict()Using
key_doc.to_mongo_dict()and excluding specific keys to build the$setoperation is appropriate. This approach ensures that only the intended fields are updated in the MongoDB document, aligning with the transition away from Pydantic.
| @dataclass | ||
| class KeyDocument: | ||
| """Model representing a key in the store.""" | ||
|
|
||
| store_name: str | ||
| key: str | ||
| value: Any | ||
| created_at: datetime.datetime = Field( | ||
| created_at: datetime.datetime = field( | ||
| default_factory=datetime.datetime.now | ||
| ) | ||
| _id: Optional[Any] = None | ||
| updated_at: Optional[datetime.datetime] = None | ||
| expires_at: Optional[datetime.datetime] = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing data validation for KeyDocument fields
Transitioning from Pydantic's BaseModel to dataclasses removes the automatic data validation that Pydantic provides. To maintain data integrity, especially when instances of KeyDocument are created from external sources, consider adding validation methods or using libraries like pydantic.dataclasses to incorporate validation into your dataclasses.
| created_at: datetime.datetime = field( | ||
| default_factory=datetime.datetime.now | ||
| ) |
There was a problem hiding this comment.
Use timezone-aware timestamps for created_at
The created_at field uses datetime.datetime.now, which returns a naive datetime object without timezone information. This can lead to inconsistencies across different systems. Consider using timezone-aware datetime objects to ensure consistent timestamping.
You can modify the default_factory to include timezone information:
- created_at: datetime.datetime = field(
- default_factory=datetime.datetime.now
- )
+ created_at: datetime.datetime = field(
+ default_factory=lambda: datetime.datetime.now(datetime.timezone.utc)
+ )📝 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: datetime.datetime = field( | |
| default_factory=datetime.datetime.now | |
| ) | |
| created_at: datetime.datetime = field( | |
| default_factory=lambda: datetime.datetime.now(datetime.timezone.utc) | |
| ) |
Removes pydantic from execution store implementation. Uses dataclasses instead.
Summary by CodeRabbit
New Features
KeyDocumentto convert instances into MongoDB-compatible dictionaries.Bug Fixes
Refactor
KeyDocumentfrom a Pydantic model to a simpler dataclass structure.ExecutionStoreclass, streamlining the code.