Fix full-refresh and vars for retry #9328

ChenyuLInx · 2024-01-04T00:34:25Z

This patch restructures retry so that it resolves all flags and config first, then does the parsing and execution.
It also changes to only load the last run results since that's the only thing needed for retry.

ChenyuLInx · 2024-01-04T00:38:18Z

core/dbt/task/retry.py

@@ -28,6 +32,7 @@
    "defer_state",
    "deprecated_state",
    "target_path",
+    "warn_error",


@graciegoheen how do we feel about warn_error being a thing that's being carried over?
Current behavior:

dbt run --warn-error failed because of some warning seen as error

'dbt retry' will not treat those warnings as error

'dbt retry --warn-error' will be the way to still treat warnings as error.
This is the previous behavior we have. Do we think this is good? or do we want to change it?

@peterallenwebb I think we talked about this became a behavior change in my version. But in the latest version this is actually not.

dbt run --warn-error failed because of some warning seen as error

'dbt retry' will not treat those warnings as error

This is surprising to me. It ties back to the desire to have a consistent policy here. I would assume that all flags passed to the original command are pulled through to dbt retry. Is this a specific exception for --warn-error? cc: @jtcohen6 do you remember if this was intentional or not?

Ultimately, if we're worried about breaking something in a backport, we could address this for 1.8.

So warn-error is a flag, so if we don't ignore it in retry there's actually no way to specify no-warn-error in retry.

dbt run --warn-error failed because of some warning seen as error
'dbt retry' will not treat those warnings as error

Just to be super clear this is the current behavior of retry.

ChenyuLInx · 2024-01-04T00:40:41Z

core/dbt/task/retry.py

+            k: v
+            for k, v in args.__dict__.items()
+            if k in IGNORE_PARENT_FLAGS
+            or click_context.get_parameter_source(k) == ParameterSource.COMMANDLINE


This is a key change, any parameter from the command line would overwrite the previous set parameter.
If it is not from command line and not in the IGNORE_PARENT_FLAGS, we will reuse things from previous invocation

@ChenyuLInx Doesn't this mean that any parameter supplied on the command line would be used? That seems different than what we discussed with Grace, which was that only a limited set of parameters could be overriden when using retry.

Thanks for calling it out. Updated.

The code now looks better to me, but I think it also means we need to add some more parameters to the ALLOW_OVERRIDE_ARGS list. In particular the --target and --profile flags are overridden in production usage of dbt, as you can see using this Datadog query.

A different Datadog query shows that --log-format, --debug, and --profiles-dir are also specified sometimes in production.

So we actually have two list now, and they behave slightly different.
There's a IGNORE_PARENT_FLAGS whose behavior is: no matter what the last command set, we actually don't try to reuse them, but just use the new ones,
then there is the ALLOW_OVERRIDE_ARGS, which is : use whatever the command set, unless user specify something new in the retry command.

Is the name not obvious enough? should I change to different name?

I think it would be slightly clearer if we called it ALLOW_CLI_OVERRIDE_FLAGS, but upon further review this looks okay.

codecov · 2024-01-04T00:43:27Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (e42b7ca) 86.64% compared to head (8b862be) 86.60%.
Report is 1 commits behind head on main.

❗ Current head 8b862be differs from pull request most recent head 736d35f. Consider uploading reports for the commit 736d35f to get more accurate results

Files	Patch %	Lines
core/dbt/contracts/state.py	66.66%	3 Missing ⚠️
core/dbt/parser/manifest.py	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9328      +/-   ##
==========================================
- Coverage   86.64%   86.60%   -0.04%     
==========================================
  Files         221      224       +3     
  Lines       26997    27016      +19     
==========================================
+ Hits        23391    23398       +7     
- Misses       3606     3618      +12

Flag	Coverage Δ
integration	`83.49% <92.30%> (-0.09%)`	⬇️
unit	`65.22% <38.46%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ChenyuLInx · 2024-01-04T00:53:18Z

core/dbt/task/retry.py

-        if self.args.warn_error:
-            RETRYABLE_STATUSES.add(NodeStatus.Warn)
-
-        self.previous_state = PreviousState(


We do not need the full previous state here, just run results.
Loading the whole state could be memory intensive for large projects(manifest can get large.)

ChenyuLInx · 2024-01-04T00:54:08Z

core/dbt/task/retry.py


 RETRYABLE_STATUSES = {NodeStatus.Error, NodeStatus.Fail, NodeStatus.Skipped, NodeStatus.RuntimeErr}
-OVERRIDE_PARENT_FLAGS = {
+IGNORE_PARENT_FLAGS = {


Updating the name to better reflect what this list is.

github-actions · 2024-01-10T17:06:36Z

The backport to 1.7.latest failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.7.latest 1.7.latest
# Navigate to the new working tree
cd .worktrees/backport-1.7.latest
# Create a new branch
git switch --create backport-9328-to-1.7.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1e4286a62dddd49e251443e51529dda5ab1ff012
# Push it to GitHub
git push --set-upstream origin backport-9328-to-1.7.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.7.latest

Then, create a pull request where the base branch is 1.7.latest and the compare/head branch is backport-9328-to-1.7.latest.

Co-authored-by: Peter Allen Webb <peter.webb@dbtlabs.com> (cherry picked from commit 1e4286a)

graciegoheen · 2024-01-16T23:15:02Z

Backport for 1.7 -> #9391

* Fix full-refresh and vars for retry (#9328) Co-authored-by: Peter Allen Webb <peter.webb@dbtlabs.com> (cherry picked from commit 1e4286a) * pr feedback * Update requires.py

peterallenwebb and others added 10 commits December 13, 2023 17:46

Fix the way vars interact with retry.

c118a5e

Fix persistence of full_refresh flag during retry.

b398902

Add changelong entry

62f30d3

Fix path construction while looking up previous state.

6477c45

Add full-refresh parameter to retry.

fc7acbe

scrappy

c9cdd2f

Update unit test to match the behavior we now think is correct.

0fcf0d7

Merge branch 'main' into cl/retry-fix-alter-method

ef54150

restructure retry

75df690

properly combine vars

b018326

ChenyuLInx requested a review from a team as a code owner January 4, 2024 00:34

ChenyuLInx requested a review from gshank January 4, 2024 00:34

cla-bot bot added the cla:yes label Jan 4, 2024

ChenyuLInx commented Jan 4, 2024

View reviewed changes

ChenyuLInx requested a review from peterallenwebb January 4, 2024 00:40

nits

5215d65

ChenyuLInx commented Jan 4, 2024

View reviewed changes

ChenyuLInx added 3 commits January 3, 2024 16:54

Update Fixes-20231213-220449.yaml

492ad69

Merge branch 'main' into cl/retry-fix-alter-method

1795f12

pr_feedback

8b862be

peterallenwebb approved these changes Jan 5, 2024

View reviewed changes

pr feedback

736d35f

ChenyuLInx merged commit 1e4286a into main Jan 10, 2024
58 of 68 checks passed

ChenyuLInx added the backport 1.7.latest label Jan 10, 2024

ChenyuLInx added a commit that referenced this pull request Jan 16, 2024

Fix full-refresh and vars for retry (#9328)

28a0bdb

Co-authored-by: Peter Allen Webb <peter.webb@dbtlabs.com> (cherry picked from commit 1e4286a)

graciegoheen mentioned this pull request Jan 16, 2024

Fix the way vars and --full-refresh interact with retry. #9285

Closed

5 tasks

ChenyuLInx added a commit that referenced this pull request Jan 17, 2024

Backport 9328 to 1.7.latest (#9391)

6e33183

* Fix full-refresh and vars for retry (#9328) Co-authored-by: Peter Allen Webb <peter.webb@dbtlabs.com> (cherry picked from commit 1e4286a) * pr feedback * Update requires.py

dbeatty10 mentioned this pull request Feb 16, 2024

[Feature] Add target_path to global_flags #9575

Open

3 tasks

tlento mentioned this pull request Feb 26, 2024

Update dbt-semantic-interfaces dependency to compatible range #9671

Merged

ChenyuLInx deleted the cl/retry-fix-alter-method branch May 2, 2024 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix full-refresh and vars for retry #9328

Fix full-refresh and vars for retry #9328

ChenyuLInx commented Jan 4, 2024

ChenyuLInx Jan 4, 2024

ChenyuLInx Jan 4, 2024

graciegoheen Jan 5, 2024 •

edited

Loading

ChenyuLInx Jan 5, 2024

ChenyuLInx Jan 4, 2024

peterallenwebb Jan 4, 2024

ChenyuLInx Jan 4, 2024

peterallenwebb Jan 4, 2024

peterallenwebb Jan 4, 2024

ChenyuLInx Jan 4, 2024

peterallenwebb Jan 5, 2024

ChenyuLInx Jan 5, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading

ChenyuLInx Jan 4, 2024

ChenyuLInx Jan 4, 2024

github-actions bot commented Jan 10, 2024

graciegoheen commented Jan 16, 2024 •

edited

Loading

Fix full-refresh and vars for retry #9328

Fix full-refresh and vars for retry #9328

Conversation

ChenyuLInx commented Jan 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graciegoheen Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2024

graciegoheen commented Jan 16, 2024 • edited Loading

graciegoheen Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 4, 2024 •

edited

Loading

graciegoheen commented Jan 16, 2024 •

edited

Loading