-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feature/defer to prod #2656
Feature/defer to prod #2656
Conversation
This is really exciting. Pulling on two of the threads you mention above:
This is the expected behavior IMO, since it ensures that
To clarify my understanding: Ephemerals are passthroughs, and so any of their references to non-ephemeral, unselected models will be deferred. To that end, it doesn't matter whether the ephemeral models themselves are included or excluded from the set of selected nodes. |
Yeah. I played around with a bit and deferring to ephemeral models really violates my expectations about |
e7c624e
to
f58e338
Compare
I've rebased onto the newly-merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I played around with our internal analytics project, and everything felt intuitive, including ephemeral model behavior. Just two small questions from me.
core/dbt/task/run.py
Outdated
def _get_state_path(self) -> Path: | ||
if self.args.state is not None: | ||
return self.args.state | ||
elif ARTIFACT_STATE_PATH is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Glad to see that ARTIFACT_STATE_PATH
and --state
are mutually substitutable, to support Cloud deployment.
I think the conclusion of this conversation was to add a separate environment variable for deferral as well. Let me know if you disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that! It's set up differently because it's a boolean rather than a value, but it's defined in dbt/flags.py
. It being set/unset informs the default value for the --defer
/--no-defer
argument to change.
I'll unify this logic to all go in flags
+ main
.
- Actually set deferred=True - add a test for that - combine/unify environment vars vs cli vars behavior between defer/state better - make the state path start with DBT_
d13a25b
to
2ed9764
Compare
resolves #2527
Description
Adds
--defer
and--state
arguments, they behave as described in #2527I had to update hologram and make some small tweaks to support parsing the manifest from disk. The
compiled
field is now mandatory for compiled nodes, and while it may exists for a short time in aFalse
state, users should never experience a situation wherecompiled=False
.The code as it is replaces all unselected nodes with entries from a previous run. I'm not sure this is as intuitive as we might want!
For example, if you have seeds you ref, and you run
dbt run --defer --state ./state
, the seeds will still point at the current run's information, because the implicitfqn:*
selector did select seeds! I think we can make some tweaks to node selection to fix this if we want, but for now I left it as-is.Ephemeral nodes are never deferred.
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.