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

[SNOW-1569937] App Versioning #1332

Closed
wants to merge 17 commits into from
Closed

[SNOW-1569937] App Versioning #1332

wants to merge 17 commits into from

Conversation

sfc-gh-chu
Copy link
Contributor

@sfc-gh-chu sfc-gh-chu commented Aug 9, 2024 •
edited
Loading

Description

  • Fixes alembic DB work to support migrations to new app version schema
  • Tru() takes an app_name parameter which sets the schema name
  • Renames app_id > app_version

Other details good to know for developers

Please include any other details of this change useful for TruLens developers.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@sfc-gh-chu sfc-gh-chu marked this pull request as ready for review August 10, 2024 16:34
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. documentation Improvements or additions to documentation labels Aug 10, 2024
@sfc-gh-chu sfc-gh-chu changed the title App Versioning [SNOW-1569937] App Versioning Aug 12, 2024
@sfc-gh-pmardziel
Copy link
Contributor

What is the effect of app name on non-snowflake databases?

sfc-gh-pdharmana
sfc-gh-pdharmana reviewed Aug 13, 2024
View reviewed changes
src/core/trulens/core/app/base.py
@@ -1475,7 +1475,7 @@ def dummy_record(
calls.append(sample_call)

return mod_record_schema.Record(
app_id=self.app_id,
app_version=self.app_version,
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we support the following in OSS?
App 1 -> v0, v1, v2, v3
App 2 -> v0, v1
App 3 -> v1, v2.

From snowflake we support this by saying, create a different schema for the app, and every version is in tables. But I don't see how the same will work with open source. Should we introduce app-id and app-version as two different concepts instead of this?

Copy link
Contributor Author

@sfc-gh-chu sfc-gh-chu Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tru now supports an app_name argument, which in OSS, would define the database filename (app_1.sqlite instead of default.sqlite).

The thought is that in OSS, different apps are stored in different DBs. Only in Snowflake would that distinction be made at the schema-level however.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 13, 2024
@sfc-gh-pmardziel
Copy link
Contributor

sfc-gh-pmardziel commented Aug 15, 2024 •
edited
Loading

Thoughts on generalizing this while being backwards compatible and not deprecating:

  • Reinterpret "app_id" as a generic unique identifier not to be set by the user directly, only to link tables.
  • Create a new column app_idents (or some thing else), of type JSON, and store things like name, version, deployment or anything else one wants to store there. We could even use the current tags column for this, as long as we expose to the database.
  • Adjust methods to accept app_name, app_version and interpret that as app_idents={'name': app_name, 'version': app_version}. User can now query apps by either or both name and version, or anything else we decide to add to ident in the future.
  • Store both app_idents and unique app_id in the AppDefinition. This is so that users can also query the database by exact id that they would have not otherwise set themselves.
  • For snowflake, let the database interface decide whether it wants to send apps to different schemas based on app_idents['name'] . Other database interfaces can decide not to do this.
  • Accept idents in Tru so that all apps created in that workspace automatically inherit those idents.

sfc-gh-dkurokawa
sfc-gh-dkurokawa reviewed Aug 16, 2024
View reviewed changes
src/core/trulens/core/schema/record.py
@@ -68,6 +68,9 @@ class RecordAppCall(serial.SerialModel):
perf: Optional[mod_base_schema.Perf] = None
"""Timestamps tracking entrance and exit of the instrumented method."""

cost: Optional[mod_base_schema.Cost] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to add this?

src/core/trulens/core/database/sqlalchemy.py
@@ -337,6 +337,18 @@ def insert_record(

return _rec.record_id

def get_app_version(
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, is there a reason you moved this up?

src/core/trulens/core/database/migrations/versions/2_set_ff_id_not_null.py

Revision ID: 2
Revises: 1
Create Date: 2023-08-10 23:11:37.405982
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update this date.

src/core/trulens/core/database/migrations/versions/3_rename_app_id_to_version_tag.py
if prefix is None:
raise RuntimeError("trulens.table_prefix is not set")

# TODO: need to prepend prefix to all table names in the upgrades
Copy link
Contributor

Choose a reason for hiding this comment

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

get rid of TODO.

@sfc-gh-chu sfc-gh-chu marked this pull request as draft August 16, 2024 02:54
@sfc-gh-dhuang sfc-gh-dhuang self-requested a review August 19, 2024 15:52
@sfc-gh-chu sfc-gh-chu closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-dkurokawa sfc-gh-dkurokawa sfc-gh-dkurokawa left review comments

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

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

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

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

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

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