-
Notifications
You must be signed in to change notification settings - Fork 204
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
fix: cartesian products in get_evaluations #3081
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.
do you think it's important to have a test for this one?
we don't have a set up for server integration tests in general |
connectable, | ||
index_col=["span_id", "document_position"], | ||
) | ||
).set_index(["span_id", "document_position"]) |
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.
Reason to prefer this to index_col
?
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.
It didn't work. Don't know why
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.
Weird
.join_from(models.Span, models.Trace) | ||
.join_from(models.Trace, models.Project) | ||
.where(models.Project.name == project_name) | ||
.where(models.DocumentAnnotation.annotator_kind == "LLM"), |
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.
Good catch.
.join_from(models.Span, models.Trace) | ||
.join_from(models.Trace, models.Project) | ||
.where(models.Project.name == project_name) | ||
.where(models.DocumentAnnotation.annotator_kind == "LLM"), |
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.
So strange to me that sqlalchemy doesn't complain here.
I think we can seed data use SQLAlchemy and test the query at whatever level you find appropriate, something like here: https://github.com/Arize-ai/phoenix/blob/sql/tests/db/test_models.py, is this setup insufficient to catch this? |
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.
great catches, are we still doing cross joins elsewhere?
yea, that would work |
No description provided.