Fix full-refresh and vars for retry #9328
Conversation
| @@ -28,6 +32,7 @@ | |||
| "defer_state", | |||
| "deprecated_state", | |||
| "target_path", | |||
| "warn_error", | |||
There was a problem hiding this comment.
@graciegoheen how do we feel about warn_error being a thing that's being carried over?
Current behavior:
dbt run --warn-errorfailed 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?
There was a problem hiding this comment.
@peterallenwebb I think we talked about this became a behavior change in my version. But in the latest version this is actually not.
There was a problem hiding this comment.
- 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.
There was a problem hiding this comment.
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.
core/dbt/task/retry.py
Outdated
| k: v | ||
| for k, v in args.__dict__.items() | ||
| if k in IGNORE_PARENT_FLAGS | ||
| or click_context.get_parameter_source(k) == ParameterSource.COMMANDLINE |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Thanks for calling it out. Updated.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
A different Datadog query shows that --log-format, --debug, and --profiles-dir are also specified sometimes in production.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think it would be slightly clearer if we called it ALLOW_CLI_OVERRIDE_FLAGS, but upon further review this looks okay.
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if self.args.warn_error: | ||
| RETRYABLE_STATUSES.add(NodeStatus.Warn) | ||
|
|
||
| self.previous_state = PreviousState( |
There was a problem hiding this comment.
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.)
|
|
||
| RETRYABLE_STATUSES = {NodeStatus.Error, NodeStatus.Fail, NodeStatus.Skipped, NodeStatus.RuntimeErr} | ||
| OVERRIDE_PARENT_FLAGS = { | ||
| IGNORE_PARENT_FLAGS = { |
There was a problem hiding this comment.
Updating the name to better reflect what this list is.
|
The backport to 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.latestThen, create a pull request where the |
Co-authored-by: Peter Allen Webb <peter.webb@dbtlabs.com> (cherry picked from commit 1e4286a)
|
Backport for 1.7 -> #9391 |
resolves #9327 and #9112
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.