[dagster-dbt] Make default backfill_policy=None always for @dbt_asset

[smackesey]

Summary & Motivation

@dbt_assets currently sets a default backfill policy of BackfillPolicy.single_run() if there is a TimeWindowPartitionsDefinition. This is non-standard behavior since all other assets have a default backfill policy of None.

With the recent fix of job backfills to respect backfill policies, this has caused problems for some users since all assets in a job need to have the same backfill policy, so users who have a mix of dbt and regular assets in the same job without setting a backfill policy are now seeing failures.

This PR removes the non-standard default, which is a breaking change (but it should be noted backfill policies are experimental).

How I Tested These Changes

Add test to make sure a job with @dbt_asset and @asset can be created.

[smackesey]

• [dagster-dbt] Make default backfill_policy=None always for @dbt_asset #22280 👈 (View in Graphite)
• master

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

[smackesey]

This whole test is no longer needed as there is no special backfill policy logic in @dbt_assets anymore.

[smackesey]

I confirmed this test was failing before the changes.

[schrockn]

This is a breaking change but I think the original behavior is so counterintuitive that we should consider it a bug, unless I am missing something obvious. Want @rexledesma to weigh in ofc since he has more context.

[rexledesma]

Adding @sryza here for visibility.

The non-standard default for backfill_policy in @dbt_assets was added in #17822.

The context was that the initial introduction of backfill_policy was providing a broken experience for folks. Before, they could do a single-run backfill through the UI. Afterwards, they had to manually specify BackfillPolicy.single_run() to get that behavior.

This change would remove this bandaid for users.

---

Part of the problem was, after the introduction of backfill_policy, the UI did not clearly indicate that the user now had to take action in code to accomplish a single-run backfill. Do we have that guidance in the UI now?

Furthermore, when defining a mismatch of backfill_policy's from separate assets combined in a job, do we have an appropriate error message for the user to unify the backfill_policy of these definitions?

[smackesey]

Part of the problem was, after the introduction of backfill_policy, the UI did not clearly indicate that the user now had to take action in code to accomplish a single-run backfill.

That makes sense, but is there something special about dbt and TimeWindowPartitionsDefinition in particular? Because it seems like one could make the same argument for any partitioned asset, but all other partitioned assets default to backfill_policy=None AFAICT.

Do we have that guidance in the UI now?

The UI has some text notifying the user that backfill policies will be used, but I don't think it provides specific guidance like: "if you want a single-run backfill, set BackfillPolicy.single_run(). Perhaps we should be more explicit? Curious what @sryza thinks.

Furthermore, when defining a mismatch of backfill_policy's from separate assets combined in a job, do we have an appropriate error message for the user to unify the backfill_policy of these definitions?

Yes: https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/definitions/unresolved_asset_job_definition.py#L186-L193

[rexledesma]

That makes sense, but is there something special about dbt and TimeWindowPartitionsDefinition in particular?

dagster-dbt is a pretty common entrypoint for folks into Dagster. We were just making it ergonomic for these folks, since things are coming out of the box for them. For dbt, a single-run backfill is a sensible default, considering that could accomplished by running dbt with --full-refresh.

Yes: https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/definitions/unresolved_asset_job_definition.py#L186-L193

The current message describes the definition error, but does not prescribe an explicit remedy. Like which definitions should I modify to ensure that the backfill policies are the same?

I would love to have an error message that mentions the names of the underlying node definitions violating the constraint and the corresponding backfill policies.

[smackesey]

@schrockn Since @rexledesma laid out the reasoning for the current behavior, can you weigh in as to whether to proceed with this PR?

Also I've improved the error message for multiple backfill policies in an asset job here: #22433

[rexledesma]

Some discussion is happening in some issues: #22254, #22356.

[smackesey]

@rexledesma I'm not familiar enough with dagster-dbt to know this without digging-- will partitioned assets created with @dbt_assets work "by default" with partition ranges (i.e. single-run backfill)? Do users have to set anything else (like a special IO manager?)

Asking because there are users using partitioned dbt assets (see here) who seem to be not using the single-run-backfill functionality, because they've been backfilling using jobs.

[rexledesma]

@smackesey They don't need a custom IO manager. Instead, they'll need to appropriately shuffle in the partition information to the dbt invocation in the body of their execution function for things to work properly. https://docs.dagster.io/integrations/dbt/reference#building-incremental-models-using-partitions

[rexledesma]

Not sure if we need this anymore -- did we end up fixing this behavior in the core framework?

[schrockn]

q mgmt