fix to sql query bug in dashboard #1531
Conversation
dosubot
bot
added
the
size:M
![ellipsis-dev[bot]](https://github.com/avatars/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 3027a36 in 37 seconds
More details
- Looked at
243lines of code in4files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_MwCPiyqiCgayWe4f
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -778,6 +778,8 @@ def get_records_and_feedback( | |||
|
|
|||
| stmt = stmt.limit(limit).offset(offset) | |||
|
|
|||
| print("stmt=", stmt) | |||
There was a problem hiding this comment.
Debugging print statement should be removed or replaced with proper logging before merging.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fe5bbb9 in 12 seconds
More details
- Looked at
33lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/core/trulens/core/database/connector/base.py:369
- Draft comment:
The PR description mentions disablingrelationshiporder_byin ORM classes, but the changes in the diff only include docstring updates. Ensure that the intended code changes are included in the PR. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_xqW2m3ngX3FvaHCl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a0cb8a2 in 9 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/core/trulens/core/database/sqlalchemy.py:779
- Draft comment:
The removal of theprintstatement for debugging SQL statements is a good practice for production code. However, the PR description mentioned adding a print statement for debugging, which seems contradictory. Please ensure the PR description accurately reflects the changes made. - Reason this comment was not posted:
Confidence changes required:50%
The print statement for debugging SQL statements was removed, which is a good practice for production code. However, the PR description mentioned adding a print statement for debugging, which seems contradictory. This might be a mistake in the PR description or the code change.
Workflow ID: wflow_ni5W0qgBmuDybmHx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
dosubot
bot
added
size:L
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4bc5548 in 15 seconds
More details
- Looked at
30lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/core/trulens/core/database/sqlalchemy.py:765
- Draft comment:
The PR description mentions adding a print statement for debugging SQL statements, but this change is not present in the diff. Ensure that the intended debugging print statement is included in the commit. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_EujvLPMap0YBFeBm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
sfc-gh-pmardziel
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3d8e4d8 in 12 seconds
More details
- Looked at
170lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/core/trulens/core/database/sqlalchemy.py:761
- Draft comment:
The PR description mentions adding a print statement for debugging SQL statements, but no such addition is present in the diff. Ensure that the intended debugging statement is included. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions adding a print statement for debugging SQL statements, but I don't see any such addition in the diff. This might be an oversight or a missed change.
Workflow ID: wflow_iH5R6EuWKxuMebjd
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 28f555c in 11 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/core/trulens/core/database/orm.py:404
- Draft comment:
Theorder_byin thebackrefforAppDefinition.recordsshould be removed as per the PR description to prevent SQLAlchemy query generation issues. This applies to otherorder_byin backrefs as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_j5L0gyI6JtZH0wbN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Confirmed this resolves the dashboard bug |
|
|
||
| # Couldn't figure out how to avoid using prefix in ForeignKey here | ||
| # and in the other classes below. | ||
| app_id = Column( |
There was a problem hiding this comment.
does this require a migration?
There was a problem hiding this comment.
I don't think the schema has changed so I don't think so.
Description
Changed what
order_byapplies to in the orm relationships to prevent an incorrect SQL bug.Added limit and offset to
get_leaderboard. The bug was occurring only in the dashboard because it was usingget_records_and_feedbackwith a limit whereas the notebooks did not use a limit inside theirget_leaderboard.Other details good to know for developers
Please include any other details of this change useful for TruLens developers.
Type of change
not work as expected)
Important
Fix SQL query bug in dashboard by disabling
order_byin ORM relationships and addinglimitandoffsettoget_leaderboard.order_byin ORM relationships inorm.pyto prevent SQLAlchemy query generation issues withjoinedloadandlimit.limitandoffsetparameters toget_leaderboardinbase.pyandsession.pyto ensure consistent query behavior.Record,FeedbackResult, andGroundTruthclasses inorm.pyto removeorder_byin relationships.printstatement for debugging SQL statements insqlalchemy.py.This description was created by
for 28f555c. It will automatically update as commits are pushed.