-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[asset health] include freshness in asset health #29268
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
Conversation
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit c0342ff. |
3d71f5b
to
6f561f8
Compare
@record | ||
class FreshnessStateRecord: | ||
entity_key: AssetKey | ||
freshness_state: FreshnessState | ||
updated_at: datetime | ||
record_body: FreshnessStateRecordBody | ||
|
||
@staticmethod | ||
def from_db_row(db_row: Row): | ||
return FreshnessStateRecord( | ||
entity_key=check.not_none(AssetKey.from_db_string(db_row[0])), | ||
freshness_state=FreshnessState(db_row[3]), | ||
record_body=deserialize_value(db_row[4], FreshnessStateRecordBody), | ||
updated_at=db_row[5], | ||
) |
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.
The FreshnessStateRecord
class should have the @whitelist_for_serdes
decorator applied to ensure proper serialization/deserialization through Dagster's serialization system. This is consistent with other similar record classes in the codebase (like FreshnessStateRecordBody
). Without this decorator, there may be issues when storing and retrieving these records.
@whitelist_for_serdes # Add this decorator
@record
class FreshnessStateRecord:
# ...
@record | |
class FreshnessStateRecord: | |
entity_key: AssetKey | |
freshness_state: FreshnessState | |
updated_at: datetime | |
record_body: FreshnessStateRecordBody | |
@staticmethod | |
def from_db_row(db_row: Row): | |
return FreshnessStateRecord( | |
entity_key=check.not_none(AssetKey.from_db_string(db_row[0])), | |
freshness_state=FreshnessState(db_row[3]), | |
record_body=deserialize_value(db_row[4], FreshnessStateRecordBody), | |
updated_at=db_row[5], | |
) | |
@whitelist_for_serdes | |
@record | |
class FreshnessStateRecord: | |
entity_key: AssetKey | |
freshness_state: FreshnessState | |
updated_at: datetime | |
record_body: FreshnessStateRecordBody | |
@staticmethod | |
def from_db_row(db_row: Row): | |
return FreshnessStateRecord( | |
entity_key=check.not_none(AssetKey.from_db_string(db_row[0])), | |
freshness_state=FreshnessState(db_row[3]), | |
record_body=deserialize_value(db_row[4], FreshnessStateRecordBody), | |
updated_at=db_row[5], | |
) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
This is going to require some coordination with the freshness stack, since the daemon and graphql rely on types that are moved to OSS by this PR: 👀 #14963 [4/n] gql resolver to fetch freshness state for asset |
python_modules/dagster-graphql/dagster_graphql_tests/graphql/repo.py
Outdated
Show resolved
Hide resolved
@@ -3395,6 +3395,9 @@ def report_runless_asset_event( | |||
), | |||
) | |||
|
|||
def get_entity_freshness_state(self, entity_key: AssetKey) -> Optional[FreshnessStateRecord]: | |||
return 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.
does this need any warning or error message to signify that it's not implemented in OSS / is cloud only?
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.
since the method isn't marked public i think it's probably fine? in other similar cases we will usually return None
, []
, etc and not have a warning, but there's probably no harm in having a warning either
6f561f8
to
80c3a78
Compare
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.
🚀
Summary & Motivation
Includes the freshness status of each asset in asset health. Follows these rules:
HEALTHY
- the freshness policy is in a PASS-ing stateWARNING
- the freshness policy is in a WARN-ing stateDEGRADED
- the freshness policy is in a FAIL-ing stateUNKNOWN
- the freshness policy has never been evaluated or is in an UNKNOWN stateNOT_APPLICABLE
- the asset does not have a freshness policy defined.I need to access the freshness getter function from OSS, so i added an instance method for it. This could also be solved by moving the storage methods to a shared storage class (Run or EventLog probably). I don't have a preference and picked adding the Instance method for no specific reason. Similarly, I needed to move
FreshnessRecord
andFreshnessRecordBody
to OSS so that I could use them as type hintsHow I Tested These Changes
tests in https://github.com/dagster-io/internal/pull/15022