feat(python-sdk): add logger, datastore queue parity, and docs refresh - #6
Conversation
There was a problem hiding this comment.
Code Review
This pull request transitions the Python SDK from a core-only implementation to a full-featured port of the Convert Fullstack SDK. It introduces a multi-client logging system, visitor persistence with asynchronous queuing, and utilities for file-based data storage. Review feedback highlights a logic error in segment filtering that could lead to persisting non-reportable data, thread-safety concerns in the file-based data store, and opportunities for performance optimization through bulk updates. Additionally, it is recommended to remove camelCase method aliases to maintain PEP 8 compliance.
| stored_value = ( | ||
| object_deep_merge( | ||
| data, | ||
| {"segments": {**report_segments, **new_segments}}, | ||
| ) | ||
| if new_segments | ||
| else updated | ||
| ) |
There was a problem hiding this comment.
There's a potential bug here. If new_data contains only non-reportable segments, new_segments will be empty, and the else branch will be taken. This sets stored_value to updated, which includes the non-reportable segments. This seems to contradict the goal of persisting only reportable segments. The stored_value should always be constructed using only filtered, reportable segments.
stored_value = object_deep_merge(
data,
{"segments": {**report_segments, **new_segments}},
)| for key, value in queued_items.items(): | ||
| self.set(key, value) |
There was a problem hiding this comment.
Calling set in a loop for each item can be inefficient, causing repeated file reads and writes if using a file-based datastore like the DataStore utility. To optimize this, you can perform a bulk update if the underlying data_store supports it (e.g., with a set_many method).
| for key, value in queued_items.items(): | |
| self.set(key, value) | |
| if hasattr(self.data_store, "set_many") and callable( | |
| getattr(self.data_store, "set_many") | |
| ): | |
| try: | |
| self.data_store.set_many(queued_items) | |
| except Exception as error: | |
| if self._logger_manager: | |
| self._logger_manager.error( | |
| "DataStoreManager.release_queue() bulk set failed", | |
| {"error": str(error)}, | |
| ) | |
| else: | |
| for key, value in queued_items.items(): | |
| self.set(key, value) |
| class DataStore: | ||
| def __init__(self, file_path: str) -> None: | ||
| self._file_path = Path(file_path) | ||
| self._file_path.parent.mkdir(parents=True, exist_ok=True) | ||
| if not self._file_path.exists(): | ||
| self._file_path.write_text("{}", encoding="utf-8") | ||
|
|
||
| def _read(self) -> dict[str, Any]: | ||
| try: | ||
| return json.loads(self._file_path.read_text(encoding="utf-8")) | ||
| except Exception: | ||
| return {} | ||
|
|
||
| def get(self, key: str) -> Any: | ||
| return self._read().get(key) | ||
|
|
||
| def set(self, key: str, value: Any) -> None: | ||
| data = self._read() | ||
| data[key] = value | ||
| self._file_path.write_text(json.dumps(data), encoding="utf-8") | ||
|
|
||
| def delete(self, key: str) -> None: | ||
| data = self._read() | ||
| data.pop(key, None) | ||
| self._file_path.write_text(json.dumps(data), encoding="utf-8") |
There was a problem hiding this comment.
This DataStore implementation has a couple of areas for improvement regarding thread safety and performance.
- Thread Safety: The read-modify-write operations in
setanddeleteare not atomic. If multiple threads call these methods concurrently, it can lead to race conditions and data loss. - Performance:
DataStoreManagercallssetin a loop, which is inefficient as it reads and writes the entire file for each key.
I suggest adding a threading.Lock to ensure thread safety, a set_many method for efficient bulk updates, and using more specific exceptions in _read.
import json
import threading
from collections.abc import Mapping
from pathlib import Path
from typing import Any
class DataStore:
def __init__(self, file_path: str) -> None:
self._file_path = Path(file_path)
self._lock = threading.Lock()
self._file_path.parent.mkdir(parents=True, exist_ok=True)
if not self._file_path.exists():
with self._lock:
if not self._file_path.exists():
self._file_path.write_text("{}", encoding="utf-8")
def _read(self) -> dict[str, Any]:
try:
return json.loads(self._file_path.read_text(encoding="utf-8"))
except (json.JSONDecodeError, FileNotFoundError):
return {}
def get(self, key: str) -> Any:
with self._lock:
return self._read().get(key)
def set(self, key: str, value: Any) -> None:
with self._lock:
data = self._read()
data[key] = value
self._file_path.write_text(json.dumps(data), encoding="utf-8")
def set_many(self, items: Mapping[str, Any]) -> None:
with self._lock:
data = self._read()
data.update(items)
self._file_path.write_text(json.dumps(data), encoding="utf-8")
def delete(self, key: str) -> None:
with self._lock:
data = self._read()
data.pop(key, None)
self._file_path.write_text(json.dumps(data), encoding="utf-8")| def releaseQueue(self, reason: str | None = None) -> None: | ||
| self.release_queue(reason) |
There was a problem hiding this comment.
While I understand the goal of achieving parity with the JS SDK, adding camelCase aliases for Python methods goes against PEP 8, which recommends snake_case for functions and methods. This can be confusing for Python developers and harm maintainability. It's best to stick to idiomatic Python and remove these aliases.
| def addClient( | ||
| self, | ||
| client: Any | None = None, | ||
| level: LogLevel | str = LogLevel.TRACE, | ||
| methodMap: dict[str, str] | None = None, | ||
| ) -> None: | ||
| self.add_client(client, level, methodMap) |
There was a problem hiding this comment.
These camelCase aliases (addClient, and setClientLevel below) violate Python's PEP 8 style guide. For better readability and consistency within the Python ecosystem, I recommend removing them and using only the snake_case versions.
| except Exception as error: | ||
| print(error) |
There was a problem hiding this comment.
Catching a broad Exception can hide unexpected errors. It's better to catch more specific exceptions related to file I/O, such as IOError or OSError.
| except Exception as error: | |
| print(error) | |
| except (IOError, OSError) as error: | |
| print(error) |
Summary
This PR closes a few of the remaining parity gaps between the Python SDK and the JavaScript SDK by adding logger support, datastore queue behavior, helper utilities, and updated package/docs metadata.
What’s Included
Added logger support to the Python SDK runtime
LogManagerLogLevelandLogMethodenumsConvertSDKAdded helper utilities matching the JS SDK support layer
FileLoggerDataStoreReworked
DataStoreManagerto behave more like the JS SDKDATA_STORE_QUEUE_RELEASEDevents on releaseUpdated
DataManagerUpdated
Core.close()Refreshed public exports
Refreshed docs and metadata
pyproject.tomlTesting
Added and updated unit coverage for:
DataManagerVerification:
.venv/bin/pytest -q85 passedWhy This PR
The Python SDK already had the core evaluation flow and public SDK surface in place, but it was still missing part of the JS support/runtime layer. This PR brings the Python implementation closer to the JavaScript SDK in the areas of logging, queued datastore persistence, and developer-facing helper utilities.