Change default backfill policy to BackfillPolicy.multi_run(1) for executable assets #22282
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
smackesey
force-pushed
the
sean/standardize-dagster-dbt-default-backfill-policy
branch
from
2a1e03c to
73a7421
Compare
smackesey
force-pushed
the
sean/default-backfill-policy
branch
from
a79895e to
73135c8
Compare
smackesey
force-pushed
the
sean/default-backfill-policy
branch
from
73135c8 to
0c0504d
Compare
smackesey
changed the base branch from
sean/standardize-dagster-dbt-default-backfill-policy
to
master
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 0c0504d. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 0c0504d. |
smackesey
changed the title
smackesey
force-pushed
the
sean/default-backfill-policy
branch
2 times, most recently
from
d377d14 to
3285b44
Compare
smackesey
changed the base branch from
master
to
sean/set-partition-key-for-range-l1
smackesey
force-pushed
the
sean/set-partition-key-for-range-l1
branch
from
c4ca5e6 to
695dac3
Compare
smackesey
force-pushed
the
sean/default-backfill-policy
branch
from
3285b44 to
ff91890
Compare
smackesey
force-pushed
the
sean/default-backfill-policy
branch
from
ff91890 to
caf4b19
Compare
| f"Cannot have {ASSET_PARTITION_RANGE_START_TAG} or" | ||
| f" {ASSET_PARTITION_RANGE_END_TAG} set along with" | ||
| f" {PARTITION_NAME_TAG}" | ||
| ) |
There was a problem hiding this comment.
this restriction no longer makes sense since runs targeting a single partition but launched as part of a backfill will have both of these set
There was a problem hiding this comment.
Would it be difficult to avoid setting the range tags in the case that there's a single partition targeted? Not necessarily a blocker if it's complicated, but seems like a safer and less invasive change than adding the range tags to every run from an asset backfill.
| if partition_range_start or partition_range_end: | ||
| if is_partitioned and partition_tag: | ||
| partition = partition_tag | ||
| partitions_subset = None |
There was a problem hiding this comment.
AssetMaterializationPlannedData accepts one non-null value for partition and partitions_subset. Prefer partition if we are targeting a single partition.
| @@ -37,53 +36,6 @@ | |||
| ) | |||
|
|
|||
|
|
|||
| def test_asset_backfill_not_all_asset_have_backfill_policy(): | |||
There was a problem hiding this comment.
We can delete this test because all assets now have a defined policy.
…erializable assets
smackesey
force-pushed
the
sean/default-backfill-policy
branch
from
caf4b19 to
d9d92e1
Compare
|
@sryza I made significant updates to this since you reviewed it so you'll want to take another look |
Summary & Motivation
Currently backfill policy defaults to
Nonefor assets. This null value flows all the way through our backfill machinery, and is interpreted the same way asBackfillPolicy.multi_run(1). This is weird because the backfill machinery doesn't recognize the equivalence of these two values.This PR makes it so that the backfill system never deals with null values. We do this by having
AssetsDefinitiondefault toBackfillPolicy.multi_run(1)(via a newDEFAULT_BACKFILL_POLICYconstant) for materializable assets. It didn't make sense to default to a non-null value for all assets because the backfill policy really is undefined for unexecutable assets.This is mostly a refactor-- the only intended behavior change from this PR is that if users specify a mix of
NoneandBackfillPolicy.multi_run(1)for backfill policies for their assets, we will no longer error (becauseNoneis now being converted for them).How I Tested These Changes
Existing test suite.