-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: Add versioning to the data point model #378
base: dev
Are you sure you want to change the base?
Changes from 11 commits
6fb3b4a
87bc5d8
15d8eff
52b91b4
f455ba9
f71485e
fe31bcd
7657b8e
b976f5b
2bfc657
d5243b4
667f973
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,26 +1,35 @@ | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
from datetime import datetime, timezone | ||||||||||||||||||||||||||||||||||||||
from typing import Optional | ||||||||||||||||||||||||||||||||||||||
from typing import Optional, Any, Dict | ||||||||||||||||||||||||||||||||||||||
from uuid import UUID, uuid4 | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
from pydantic import BaseModel, Field | ||||||||||||||||||||||||||||||||||||||
from typing_extensions import TypedDict | ||||||||||||||||||||||||||||||||||||||
import pickle | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
# Define metadata type | ||||||||||||||||||||||||||||||||||||||
class MetaData(TypedDict): | ||||||||||||||||||||||||||||||||||||||
index_fields: list[str] | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
# Updated DataPoint model with versioning and new fields | ||||||||||||||||||||||||||||||||||||||
class DataPoint(BaseModel): | ||||||||||||||||||||||||||||||||||||||
__tablename__ = "data_point" | ||||||||||||||||||||||||||||||||||||||
id: UUID = Field(default_factory = uuid4) | ||||||||||||||||||||||||||||||||||||||
updated_at: Optional[datetime] = datetime.now(timezone.utc) | ||||||||||||||||||||||||||||||||||||||
id: UUID = Field(default_factory=uuid4) | ||||||||||||||||||||||||||||||||||||||
created_at: int = Field(default_factory=lambda: int(datetime.now(timezone.utc).timestamp() * 1000)) | ||||||||||||||||||||||||||||||||||||||
updated_at: int = Field(default_factory=lambda: int(datetime.now(timezone.utc).timestamp() * 1000)) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+20
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify timestamp creation and add validation The timestamp creation could be simplified and should validate against negative values. Consider this improvement: - created_at: int = Field(default_factory=lambda: int(datetime.now(timezone.utc).timestamp() * 1000))
- updated_at: int = Field(default_factory=lambda: int(datetime.now(timezone.utc).timestamp() * 1000))
+ created_at: int = Field(
+ default_factory=lambda: int(datetime.now(timezone.utc).timestamp() * 1000),
+ ge=0
+ )
+ updated_at: int = Field(
+ default_factory=lambda: int(datetime.now(timezone.utc).timestamp() * 1000),
+ ge=0
+ ) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
version: int = 1 # Default version | ||||||||||||||||||||||||||||||||||||||
type: Optional[str] = "text" # "text", "file", "image", "video" | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same for type, doesn't belong here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Laslzo asked me for this one, due to retriever logic. In general I agree |
||||||||||||||||||||||||||||||||||||||
topological_rank: Optional[int] = 0 | ||||||||||||||||||||||||||||||||||||||
_metadata: Optional[MetaData] = { | ||||||||||||||||||||||||||||||||||||||
"index_fields": [], | ||||||||||||||||||||||||||||||||||||||
"type": "DataPoint" | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
# class Config: | ||||||||||||||||||||||||||||||||||||||
# underscore_attrs_are_private = True | ||||||||||||||||||||||||||||||||||||||
# Override the Pydantic configuration | ||||||||||||||||||||||||||||||||||||||
class Config: | ||||||||||||||||||||||||||||||||||||||
underscore_attrs_are_private = True | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@classmethod | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double |
||||||||||||||||||||||||||||||||||||||
def get_embeddable_data(self, data_point): | ||||||||||||||||||||||||||||||||||||||
|
@@ -30,16 +39,51 @@ def get_embeddable_data(self, data_point): | |||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
if isinstance(attribute, str): | ||||||||||||||||||||||||||||||||||||||
return attribute.strip() | ||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||
return attribute | ||||||||||||||||||||||||||||||||||||||
return attribute | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@classmethod | ||||||||||||||||||||||||||||||||||||||
def get_embeddable_properties(self, data_point): | ||||||||||||||||||||||||||||||||||||||
"""Retrieve all embeddable properties.""" | ||||||||||||||||||||||||||||||||||||||
if data_point._metadata and len(data_point._metadata["index_fields"]) > 0: | ||||||||||||||||||||||||||||||||||||||
return [getattr(data_point, field, None) for field in data_point._metadata["index_fields"]] | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
return [] | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@classmethod | ||||||||||||||||||||||||||||||||||||||
def get_embeddable_property_names(self, data_point): | ||||||||||||||||||||||||||||||||||||||
return data_point._metadata["index_fields"] or [] | ||||||||||||||||||||||||||||||||||||||
"""Retrieve names of embeddable properties.""" | ||||||||||||||||||||||||||||||||||||||
return data_point._metadata["index_fields"] or [] | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
def update_version(self): | ||||||||||||||||||||||||||||||||||||||
"""Update the version and updated_at timestamp.""" | ||||||||||||||||||||||||||||||||||||||
self.version += 1 | ||||||||||||||||||||||||||||||||||||||
self.updated_at = int(datetime.now(timezone.utc).timestamp() * 1000) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
# JSON Serialization | ||||||||||||||||||||||||||||||||||||||
def to_json(self) -> str: | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this serialization? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you can parallelize tasks, since you had issues with that. Pickle or json |
||||||||||||||||||||||||||||||||||||||
"""Serialize the instance to a JSON string.""" | ||||||||||||||||||||||||||||||||||||||
return self.json() | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@classmethod | ||||||||||||||||||||||||||||||||||||||
def from_json(self, json_str: str): | ||||||||||||||||||||||||||||||||||||||
"""Deserialize the instance from a JSON string.""" | ||||||||||||||||||||||||||||||||||||||
return self.model_validate_json(json_str) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
# Pickle Serialization | ||||||||||||||||||||||||||||||||||||||
def to_pickle(self) -> bytes: | ||||||||||||||||||||||||||||||||||||||
"""Serialize the instance to pickle-compatible bytes.""" | ||||||||||||||||||||||||||||||||||||||
return pickle.dumps(self.dict()) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@classmethod | ||||||||||||||||||||||||||||||||||||||
def from_pickle(self, pickled_data: bytes): | ||||||||||||||||||||||||||||||||||||||
"""Deserialize the instance from pickled bytes.""" | ||||||||||||||||||||||||||||||||||||||
data = pickle.loads(pickled_data) | ||||||||||||||||||||||||||||||||||||||
return self(**data) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+71
to
+80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Security concern: Replace pickle with a safer serialization method Using - def to_pickle(self) -> bytes:
- """Serialize the instance to pickle-compatible bytes."""
- return pickle.dumps(self.dict())
-
- @classmethod
- def from_pickle(self, pickled_data: bytes):
- """Deserialize the instance from pickled bytes."""
- data = pickle.loads(pickled_data)
- return self(**data)
+ def to_bytes(self) -> bytes:
+ """Serialize the instance to bytes using JSON."""
+ return self.json().encode('utf-8')
+
+ @classmethod
+ def from_bytes(cls, data: bytes):
+ """Deserialize the instance from JSON bytes."""
+ return cls.parse_raw(data) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
def to_dict(self, **kwargs) -> Dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||
"""Serialize model to a dictionary.""" | ||||||||||||||||||||||||||||||||||||||
return self.model_dump(**kwargs) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@classmethod | ||||||||||||||||||||||||||||||||||||||
def from_dict(cls, data: Dict[str, Any]) -> "DataPoint": | ||||||||||||||||||||||||||||||||||||||
"""Deserialize model from a dictionary.""" | ||||||||||||||||||||||||||||||||||||||
return cls.model_validate(data) |
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.
What is the difference between
datetime.now(timezone.utc)
and this one?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.
created_at is when the initial record was created, updated at is any change that happens