-
Notifications
You must be signed in to change notification settings - Fork 591
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
refactor: remove pydantic from execution_store #4877
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Going to remove permissions ins a follow up commit
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: 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 ofStoreDocument
and MongoDB insertion updated correctlyThe
StoreDocument
is 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_at
inKeyDocument
initializationAdding
expires_at=expiration
to theKeyDocument
initialization 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$set
update fields withto_mongo_dict()
Using
key_doc.to_mongo_dict()
and excluding specific keys to build the$set
operation 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
KeyDocument
to convert instances into MongoDB-compatible dictionaries.Bug Fixes
Refactor
KeyDocument
from a Pydantic model to a simpler dataclass structure.ExecutionStore
class, streamlining the code.