refactor: sqlite3 proof of concept #2740
Conversation
dosubot
bot
added
the
size:XL
| _CONFIG = """ | ||
| PRAGMA foreign_keys = ON; | ||
| PRAGMA journal_mode = WAL; | ||
| PRAGMA synchronous = OFF; | ||
| PRAGMA cache_size = -32000; | ||
| PRAGMA busy_timeout = 10000; | ||
| """ |
There was a problem hiding this comment.
note to self: need to figure out how this configuration is settable in something like sql alchemy
There was a problem hiding this comment.
| ) | ||
| from phoenix.core.model_schema_adapter import create_model_from_datasets | ||
| from phoenix.core.traces import Traces | ||
| from phoenix.datasets.dataset import EMPTY_DATASET, Dataset | ||
| from phoenix.datasets.fixtures import FIXTURES, get_datasets | ||
| from phoenix.db.database import SqliteDatabase |
There was a problem hiding this comment.
How about just phoenix.database?
| _INIT_DB = """ | ||
| BEGIN; | ||
| CREATE TABLE projects ( | ||
| rowid INTEGER PRIMARY KEY, |
There was a problem hiding this comment.
Do index names usually not include underscores? Looks kind of weird.
There was a problem hiding this comment.
rowid is from sqlite by default (https://www.sqlite.org/rowidtable.html), but i had to spell it out otherwise the foreign key constraint doesn't work. We can also name it something else, but I just left it as is.
There was a problem hiding this comment.
I think it's a super nit but it might make sense to have this not be sqlite flavor. It's not a big deal but this is something that an ORM abstraction could abstract away. Let's see if we can.
src/phoenix/db/database.py
Outdated
| elif start_time: | ||
| cur = cur.execute(query + " AND ? <= spans.start_time;", (project_name, start_time)) | ||
| elif start_time: | ||
| cur = cur.execute(query + " AND spans.start_time < ?;", (project_name, stop_time)) |
| ); | ||
| CREATE INDEX idx_trace_start_time ON traces(start_time); | ||
| CREATE TABLE spans ( | ||
| rowid INTEGER PRIMARY KEY, |
There was a problem hiding this comment.
| rowid INTEGER PRIMARY KEY, | |
| id INTEGER PRIMARY KEY, |
There was a problem hiding this comment.
I think when we move to sql alchamy we consider moving this to a simpler convention, though I get the confusing nature of the foreign keys....
There was a problem hiding this comment.
I'll revise the names and incorporate the other feedbacks in the sqlalchemy edition of the schema
| ); | ||
| INSERT INTO projects(name) VALUES('default'); | ||
| CREATE TABLE traces ( | ||
| rowid INTEGER PRIMARY KEY, |
There was a problem hiding this comment.
| rowid INTEGER PRIMARY KEY, | |
| id INTEGER PRIMARY KEY, |
| _INIT_DB = """ | ||
| BEGIN; | ||
| CREATE TABLE projects ( | ||
| rowid INTEGER PRIMARY KEY, |
There was a problem hiding this comment.
I think it's a super nit but it might make sense to have this not be sqlite flavor. It's not a big deal but this is something that an ORM abstraction could abstract away. Let's see if we can.
| cumulative_error_count = int(span.status_code is SpanStatusCode.ERROR) | ||
| cumulative_llm_token_count_prompt = cast( | ||
| int, span.attributes.get(SpanAttributes.LLM_TOKEN_COUNT_PROMPT, 0) | ||
| ) | ||
| cumulative_llm_token_count_completion = cast( | ||
| int, span.attributes.get(SpanAttributes.LLM_TOKEN_COUNT_COMPLETION, 0) | ||
| ) |
There was a problem hiding this comment.
Let's think about keeping the DB layer clean
| cumulative_error_count=span[11], | ||
| cumulative_llm_token_count_prompt=span[12], | ||
| cumulative_llm_token_count_completion=span[13], | ||
| cumulative_llm_token_count_total=span[12] + span[13], |
There was a problem hiding this comment.
discussed that maybe dynamic "computed columns" might belong separate to the span.
|
Merging for now, but I'll revise the names and incorporate the other feedbacks in the sqlalchemy edition of the schema. |
No description provided.