-
Notifications
You must be signed in to change notification settings - Fork 177
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
[SNOW-1569937] App Versioning #1332
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
What is the effect of app name on non-snowflake databases? |
@@ -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, |
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.
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?
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.
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.
Thoughts on generalizing this while being backwards compatible and not deprecating:
|
@@ -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 |
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.
did you mean to add this?
@@ -337,6 +337,18 @@ def insert_record( | |||
|
|||
return _rec.record_id | |||
|
|||
def get_app_version( |
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.
out of curiosity, is there a reason you moved this up?
|
||
Revision ID: 2 | ||
Revises: 1 | ||
Create Date: 2023-08-10 23:11:37.405982 |
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.
Can you update this date.
if prefix is None: | ||
raise RuntimeError("trulens.table_prefix is not set") | ||
|
||
# TODO: need to prepend prefix to all table names in the upgrades |
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.
get rid of TODO.
Description
Tru()
takes an app_name parameter which sets the schema nameapp_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
not work as expected)