-
Notifications
You must be signed in to change notification settings - Fork 285
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: allow retries for insertions #4026
Conversation
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.
I think I'm having trouble grokking the difference between insertables and precursors/ parcels.
time_cutoff = time.time() + 1 | ||
while not fut.done() and time.time() < time_cutoff: | ||
time.sleep(0.01) | ||
if fut.done(): | ||
return fut.result() | ||
raise TimeoutError |
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.
Nice, is it possible to make the timeout shorter to prevent any individual test from taking too long?
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.
at 0.1
, the next order of magnitude, i have run into some flakiness, so i think 1
is probably fine for now
src/phoenix/db/insertion/helpers.py
Outdated
for column in entity.__table__.c: | ||
if column.name in ["created_at", "updated_at"]: | ||
continue | ||
k = "metadata_" if column.name == "metadata" else column.name |
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.
Does it work to use column.key
instead?
from phoenix.db import models
column = models.Experiment.metadata_
print(f"{column.key=}")
print(f"{column.name=}")
Prints:
column.key='metadata_'
column.name='metadata'
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.
column.key
is actually still "metadata"
return select( | ||
existing_traces.c.id, | ||
existing_traces.c.trace_id, | ||
table.id, | ||
table.name, | ||
table.updated_at, | ||
).outerjoin_from( |
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.
This strikes me as a good place to use with_only_columns
to simplify the return type of the function. Same for trace and doc annotations.
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.
after reading the docs, i'm still not clear how it can be used
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.
A lot of the complexity of this PR is understanding the types and terminology (Received
and Postponed
as a generic types, Precursor
vs. Insertable
, parcel
as an instance of Received
). It would make sense to me if we had stateful classes for the entities to be inserted (e.g., TraceAnnotation
, SpanAnnotation
) that had a state
attribute (RECEIVED
, POSTPONED
), an add_foreign_key
method, and a boolean ready_for_insertion
attribute. Sort of feels like the types are being used to make everything immutable and avoid stateful objects, but at the cost of introducing a lot of concepts and terminology.
Consider the following.
|
resolves #3828