-
Notifications
You must be signed in to change notification settings - Fork 290
fix(local): load pydantic v1 pickled points under pydantic v2 (#481) #1400
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -15,3 +15,4 @@ local_cache/*/* | |
| docs/source/examples/local_cache/* | ||
| docs/source/examples/path/to/db/* | ||
| .venv | ||
| .venv/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| """Regression for #481: local persistence must load pydantic-v1 pickled points.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import io | ||
| import pickle | ||
| import tempfile | ||
|
|
||
| from qdrant_client.http import models | ||
| from qdrant_client.local.persistence import CollectionPersistence, load_point_compat | ||
|
|
||
|
|
||
| class _V1StylePoint: | ||
| """Pickle payload shaped like pydantic v1, with pydantic v2 setstate failure.""" | ||
|
|
||
| def __getstate__(self): | ||
| return { | ||
| "__dict__": {"id": 1, "vector": [1.0, 2.0, 3.0], "payload": {"a": 1}}, | ||
| "__fields_set__": {"id", "vector", "payload"}, | ||
| } | ||
|
|
||
| def __setstate__(self, state): | ||
| raise KeyError("__pydantic_fields_set__") | ||
|
|
||
|
|
||
| def _v1_style_point_blob() -> bytes: | ||
| return pickle.dumps(_V1StylePoint()) | ||
|
|
||
|
|
||
| def test_raw_pickle_matches_issue_481_failure_mode(): | ||
| blob = _v1_style_point_blob() | ||
| try: | ||
| pickle.loads(blob) | ||
| assert False, "expected KeyError" | ||
| except KeyError as exc: | ||
| assert exc.args == ("__pydantic_fields_set__",) | ||
|
|
||
|
|
||
| def test_load_point_compat_recovers_v1_state(monkeypatch): | ||
| import qdrant_client.local.persistence as persistence | ||
|
|
||
| blob = _v1_style_point_blob() | ||
|
|
||
| def _load_from_v1_style(data: bytes) -> models.PointStruct: | ||
| class _LegacyPoint: | ||
| def __setstate__(self, state): | ||
| payload = state.get("__dict__", state) | ||
| self.__dict__.update(payload if isinstance(payload, dict) else {}) | ||
|
|
||
| class _CompatUnpickler(pickle.Unpickler): | ||
| def find_class(self, module, name): | ||
| if name == "_V1StylePoint": | ||
| return _LegacyPoint | ||
| return super().find_class(module, name) | ||
|
|
||
| obj = _CompatUnpickler(io.BytesIO(data)).load() | ||
| data_dict = getattr(obj, "__dict__", {}) | ||
| return models.PointStruct.model_validate( | ||
| { | ||
| "id": data_dict["id"], | ||
| "vector": data_dict["vector"], | ||
| "payload": data_dict.get("payload"), | ||
| } | ||
| ) | ||
|
|
||
| monkeypatch.setattr(persistence, "_load_pydantic_v1_point", _load_from_v1_style) | ||
| point = load_point_compat(blob) | ||
| assert point.id == 1 | ||
| assert point.vector == [1.0, 2.0, 3.0] | ||
| assert point.payload == {"a": 1} | ||
|
|
||
|
|
||
| def test_collection_persistence_roundtrip_still_works(): | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| persistence = CollectionPersistence(tmpdir) | ||
|
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. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Close the SQLite connection before temporary-directory cleanup.
Proposed fix with tempfile.TemporaryDirectory() as tmpdir:
persistence = CollectionPersistence(tmpdir)
- point = models.PointStruct(id=7, vector=[0.1, 0.2], payload={"k": "v"})
- persistence.persist(point)
- assert list(persistence.load()) == [point]
+ try:
+ point = models.PointStruct(id=7, vector=[0.1, 0.2], payload={"k": "v"})
+ persistence.persist(point)
+ assert list(persistence.load()) == [point]
+ finally:
+ persistence.close()🤖 Prompt for AI Agents |
||
| point = models.PointStruct(id=7, vector=[0.1, 0.2], payload={"k": "v"}) | ||
| persistence.persist(point) | ||
| assert list(persistence.load()) == [point] | ||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the production legacy recovery implementation.
Line 66 replaces
_load_pydantic_v1_point, which is the code that maps legacyPointStructpickles. The fixture also uses_V1StylePoint, while production recovery only substitutes classes namedPointStructfrom a Qdrant module. This test can pass when the actual compatibility unpickler cannot recover a Pydantic-v1PointStructblob.Use an authentic Pydantic-v1
PointStructpickle fixture and callload_point_compatwithout monkeypatching the recovery helper.🤖 Prompt for AI Agents