Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add snowflake sqlalchemy implementation #1216

Merged
merged 20 commits into from
Jun 21, 2024
Merged

Add snowflake sqlalchemy implementation #1216

merged 20 commits into from
Jun 21, 2024

Conversation

sfc-gh-chu
Copy link
Contributor

@sfc-gh-chu sfc-gh-chu commented Jun 14, 2024 •
edited
Loading

Items to add to release announcement:

  • Heading: delete this list if this PR does not introduce any changes that need announcing.
  • Support Snowflake Tables with snowflake-sqlalchemy
  • Major speedups to the dashboard: A SQL query was inadvertently being made for every record due to how SQLAlchemy handles lazy loading by default, which was drastically bringing down performance, especially on remote DBs. This PR fixes that behavior to make a single joined query to bring down network cost and query overhead.
    • (5k records) Dashboard leaderboard page load time: 4x speedup (15.78s -> 3.85s)
    • (5k records) Dashboard evaluation page load time: 11x speedup (31s -> 2.80s)
    • (5k records) Dashboard single record load time: N/A -> 4.03s
    • (675 records) Querying records and feedback: 32x speedup (60s -> 1.9s)

Other details that are good to know but need not be announced:

  • There should be something here at least.
  • Requires PRPR snowflake-sqlalchemy 1.6.x installation

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 14, 2024
sfc-gh-dhuang
sfc-gh-dhuang approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-dhuang sfc-gh-dhuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prob wanna make sure you run yapf formatter

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 17, 2024
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jun 17, 2024
sfc-gh-pmardziel
sfc-gh-pmardziel reviewed Jun 18, 2024
View reviewed changes
trulens_eval/trulens_eval/database/sqlalchemy.py
@@ -51,6 +52,10 @@
logger = logging.getLogger(__name__)


class SnowflakeImpl(DefaultImpl):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how or where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When TruLens loads a Snowflake Table, this is required for SQLAlchemy to interpret snowflake DB urls

@sfc-gh-jreini
Copy link
Contributor

Can you add docs for this? You can get form factor and language from:

https://github.com/truera/trulens/blob/garett/snowflake-test/docs/trulens_eval/tracking/logging/where_to_log/log_in_snowflake.md

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jun 20, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jun 21, 2024
sfc-gh-jreini
sfc-gh-jreini requested changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-jreini sfc-gh-jreini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include log_in_snowflake.md into mkdocs.yml

docs/trulens_eval/tracking/logging/where_to_log/log_in_snowflake.md Outdated Show resolved Hide resolved
sfc-gh-dkurokawa
sfc-gh-dkurokawa approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-dkurokawa sfc-gh-dkurokawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM given your offline explanations to me, with the caveat, you may want to hold out for someone who actually understands this stuff better than the person you just taught haha. That said, I'll leave it to you if you want to submit it now.

docs/trulens_eval/tracking/logging/where_to_log/log_in_snowflake.md Outdated Show resolved Hide resolved
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jun 21, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jun 21, 2024
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jun 21, 2024
sfc-gh-jreini
sfc-gh-jreini approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-jreini sfc-gh-jreini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sfc-gh-jreini sfc-gh-jreini merged commit 98cb4a0 into main Jun 21, 2024
9 checks passed
sfc-gh-chu added a commit that referenced this pull request Jun 21, 2024
@sfc-gh-chu @sfc-gh-pmardziel
Add snowflake sqlalchemy implementation (#1216)
3074912 
* add snowflake table implementation for sqlachemy

* fmt

* fmt

* joined load

* bug

* joined 2 deep

* add logging in snowflake doc

* fmt

* fmt?

* remove unneeded

* rm trulens source install instructions

* remove legacy streamlit cache clear

* fmt

* add snowflake doc to mkdocs

---------

Co-authored-by: Piotr Mardziel <piotr.mardziel@snowflake.com>
@sfc-gh-chu sfc-gh-chu mentioned this pull request Jun 21, 2024
Merged
sfc-gh-chu added a commit that referenced this pull request Jun 21, 2024
@sfc-gh-chu @sfc-gh-pmardziel
Add snowflake sqlalchemy implementation (#1216) (#1235)
39dc01b 
* add snowflake table implementation for sqlachemy

* fmt

* fmt

* joined load

* bug

* joined 2 deep

* add logging in snowflake doc

* fmt

* fmt?

* remove unneeded

* rm trulens source install instructions

* remove legacy streamlit cache clear

* fmt

* add snowflake doc to mkdocs

---------

Co-authored-by: Piotr Mardziel <piotr.mardziel@snowflake.com>
@sfc-gh-jreini sfc-gh-jreini mentioned this pull request Jun 21, 2024
Merged
sfc-gh-dhuang pushed a commit that referenced this pull request Jun 28, 2024
@sfc-gh-chu @sfc-gh-pmardziel
Add snowflake sqlalchemy implementation (#1216)
dca680b 
* add snowflake table implementation for sqlachemy

* fmt

* fmt

* joined load

* bug

* joined 2 deep

* add logging in snowflake doc

* fmt

* fmt?

* remove unneeded

* rm trulens source install instructions

* remove legacy streamlit cache clear

* fmt

* add snowflake doc to mkdocs

---------

Co-authored-by: Piotr Mardziel <piotr.mardziel@snowflake.com>
sfc-gh-dhuang pushed a commit that referenced this pull request Jul 1, 2024
@sfc-gh-chu @sfc-gh-pmardziel @sfc-gh-dhuang
Add snowflake sqlalchemy implementation (#1216)
a0b5ee3 
* add snowflake table implementation for sqlachemy

* fmt

* fmt

* joined load

* bug

* joined 2 deep

* add logging in snowflake doc

* fmt

* fmt?

* remove unneeded

* rm trulens source install instructions

* remove legacy streamlit cache clear

* fmt

* add snowflake doc to mkdocs

---------

Co-authored-by: Piotr Mardziel <piotr.mardziel@snowflake.com>
sfc-gh-chu added a commit that referenced this pull request Sep 25, 2024
@sfc-gh-chu @sfc-gh-pmardziel
Add snowflake sqlalchemy implementation (#1216)
ccf806b 
* add snowflake table implementation for sqlachemy

* fmt

* fmt

* joined load

* bug

* joined 2 deep

* add logging in snowflake doc

* fmt

* fmt?

* remove unneeded

* rm trulens source install instructions

* remove legacy streamlit cache clear

* fmt

* add snowflake doc to mkdocs

---------

Co-authored-by: Piotr Mardziel <piotr.mardziel@snowflake.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-pmardziel sfc-gh-pmardziel sfc-gh-pmardziel left review comments

@sfc-gh-dkurokawa sfc-gh-dkurokawa sfc-gh-dkurokawa approved these changes

@sfc-gh-jreini sfc-gh-jreini sfc-gh-jreini approved these changes

@sfc-gh-dhuang sfc-gh-dhuang Awaiting requested review from sfc-gh-dhuang

@sfc-gh-pdharmana sfc-gh-pdharmana Awaiting requested review from sfc-gh-pdharmana

Assignees
No one assigned
Labels
lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sfc-gh-chu @sfc-gh-jreini @sfc-gh-dkurokawa @sfc-gh-dhuang @sfc-gh-pdharmana @sfc-gh-pmardziel