Skip to content

A new asset freshness API #28828

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

Merged
merged 18 commits into from
Apr 10, 2025
Merged

Conversation

anuthebananu
Copy link
Contributor

@anuthebananu anuthebananu commented Mar 28, 2025

Summary & Motivation

Introduces a new API for defining freshness as a top-level attribute on an asset. This API will replace freshness checks and freshness policies as the way to model asset freshness in Dagster.

For more details on the motivation behind needing a new API, please refer to the internal product review.

The implementation here is slightly different than the desired end state, which is to reclaim the deprecated FreshnessPolicy construct. Instead we're writing the new freshness policy to the asset spec metadata. This will unblock internal testing and give us time to implement a longer-term migration plan for FreshnessPolicy.

Auxiliary Changes

  • Relocate SerializableTimeDelta from dagster._core.definitions.declarative_automation.util to dagster_shared.serdes.utils

How I Tested These Changes

  • new + existing unit tests for backward compatibility

Changelog

Insert changelog entry or delete this section.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@anuthebananu anuthebananu changed the title wip - new freshness thing, everything working except from graph assets [WIP] a new asset freshness API Mar 28, 2025
@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch 3 times, most recently from c87a0f4 to 45ee60e Compare April 3, 2025 14:43
@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch 3 times, most recently from 2ed1a73 to 29d5a43 Compare April 8, 2025 18:34
@anuthebananu anuthebananu changed the title [WIP] a new asset freshness API A new asset freshness API Apr 8, 2025
@anuthebananu anuthebananu marked this pull request as ready for review April 8, 2025 18:39
@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch from 29d5a43 to c43e303 Compare April 8, 2025 18:48
@anuthebananu anuthebananu requested a review from benpankow April 8, 2025 18:53
@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch 2 times, most recently from d26e982 to 18c9cfe Compare April 8, 2025 19:14
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

not all the comments here are blocking, but the main thing is that I really don't think we can add a new non-hidden argument to AssetSpec that we plan to remove in the near future, especially due to the number of APIs we end up needing to thread this ephemeral parameter through.

Copy link
Contributor

@dpeng817 dpeng817 left a comment

Choose a reason for hiding this comment

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

As discussed offline, let's move forward with the asset metadata storage approach for now

@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch from c56de48 to 985651a Compare April 9, 2025 19:46
@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch from 6bab510 to cf7f5bb Compare April 9, 2025 21:15
@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch from 9b6754f to 066d781 Compare April 9, 2025 21:42
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

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

Great work -- this feels way cleaner / safer. Had two extremely tiny changes, and also we should have a test that includes the @asset decorator.

Approving as this is definitely in a mergeable state right now, just would prefer those changes to be made first.

Copy link
Contributor Author

knew i was forgetting something lol

@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch 2 times, most recently from 3e9bd60 to 6d652e4 Compare April 9, 2025 22:26
Copy link

github-actions bot commented Apr 9, 2025

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-nx96xrjr7-elementl.vercel.app
https://anurag-oper-1768-freshness-sla-python-api.core-storybook.dagster-docs.io

Built with commit 1c20726.
This pull request is being automatically deployed with vercel-action

@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch from 69eb992 to d2b2743 Compare April 9, 2025 23:22
Copy link
Contributor Author

I've ignored a couple of pyright errors, mainly stemming from the fact that we have a serdes class TimeWindowFreshnessPolicy that inherits from an abstract class InternalFreshnessPolicy, which we use as the type for all freshness policies.

Pyright somehow cannot recognize that it is not possible to instantiate InternalFreshnessPolicy, so it complains when it sees code like:

policy = InternalFreshnessPolicy.time_window(...) # actually a `TimeWindowFreshnessPolicy`
serialize_value(policy)

because InternalFreshnessPolicy is not whitelisted for serdes. (at this time I believe we cannot whitelist abstract classes).

To me these feel safe enough to ignore for now to get this merged, if desired we can come up with a better solution in the future.

@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch from ac60857 to 5d3a983 Compare April 10, 2025 00:33
@anuthebananu anuthebananu force-pushed the anurag/oper-1768-freshness-sla-python-api branch from 5d3a983 to 1c20726 Compare April 10, 2025 01:01
Copy link
Contributor Author

anuthebananu commented Apr 10, 2025

Merge activity

  • Apr 9, 10:03 PM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Apr 9, 10:03 PM EDT: A user merged this pull request with Graphite.

@anuthebananu anuthebananu merged commit 339b824 into master Apr 10, 2025
7 checks passed
@anuthebananu anuthebananu deleted the anurag/oper-1768-freshness-sla-python-api branch April 10, 2025 02:03
benpankow pushed a commit that referenced this pull request Apr 10, 2025
Introduces a new API for defining freshness as a top-level attribute on an asset. This API will replace freshness checks and freshness policies as the way to model asset freshness in Dagster.

For more details on the motivation behind needing a new API, please refer to the [internal product review](https://www.notion.so/dagster/New-Asset-Freshness-System-Product-Review-1c318b92e46280fe859ce86f5d78934a?d=1c718b92e462807ca2e1001c0a331dc0#1c718b92e46280c8973ef9358ec38367).

The implementation here is slightly different than the desired end state, which is to reclaim the deprecated `FreshnessPolicy` construct. Instead we're writing the new freshness policy to the asset spec metadata. This will unblock internal testing and give us time to implement a longer-term migration plan for `FreshnessPolicy`.

- Relocate `SerializableTimeDelta` from `dagster._core.definitions.declarative_automation.util` to `dagster_shared.serdes.utils`

- new + existing unit tests for backward compatibility

> Insert changelog entry or delete this section.
benpankow pushed a commit that referenced this pull request Apr 10, 2025
## Summary & Motivation
Fixes a Python 3.9 backcompat issue with unit tests introduced in
#28828.

## How I Tested These Changes
bk

## Changelog

> Insert changelog entry or delete this section.
benpankow pushed a commit that referenced this pull request Apr 10, 2025
## Summary & Motivation
Fixes a Python 3.9 backcompat issue with unit tests introduced in
#28828.

## How I Tested These Changes
bk

## Changelog

> Insert changelog entry or delete this section.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants