Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Abstract manifest generation from tasks #6565
Abstract manifest generation from tasks #6565
Changes from 2 commits
a0a4d5b
8615b39
c4ac379
2237873
0f1ecad
e5fea9a
592033e
b673061
d42de06
0945988
c675dd9
4a62ada
22eb4a7
a7e778b
b6d7ea2
fcf49a9
28efb2a
c1a1fe8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The changes in this PR mean that:
dbt parse
will always write the manifest (= overwritemanifest.json
). IMO that's a good change! I'd opened an issue for it recently: [CT-1759]dbt parse
should (over)writemanifest.json
by default #6534. So we can also remove--write-manifest
as a parameter for this command, and inparams.py
.dbt parse --compile
, which allows us to output detailed performance timing that includes the DAG construction step (= resolving ephemeral model references + linking nodes/edges).For the second point: I'd be happy with kicking that out of scope for this PR, and opening a tech debt ticket to track it. There's a bigger idea here: "Abstract manifest compilation / graph generation from tasks." Idea being, move
compile_manifest()
out from task initialization, into either a conditional step within@requires.manifest
, or as its own@requires.graph
step. Then, tasks would accept the graph as an argument.Considerations there:
RuntimeConfig
, as an inputGraph
build
task produces + uses a differentgraph
from all other tasks, with extra test edgesgraph
onctx.obj
, and passed it into each task explicitly, we would unlock the ability for programmatic invocations to cache + reuse thegraph
between invocations. That could make a difference for performance in very large projects. (With the important caveat that the external application would be responsible for distinguishing between standard andbuild
-specific graphs.)Finally, a separate proposal, out of scope for this PR but highly relevant: The
parse
task should return theManifest
here, instead ofNone
(#6547). That could be as simple as changing this last line to:And then:
There's a bit more refinement we should do on that proposal first, to make sure it's an idea we're happy with.
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.
Thank you for the context. I had spoken with Gerda about the function of the existing
ParseTask
class, and she had said I could take out the tracking. Adding it back in shouldn't be difficult, and could be done conditionally.For your proposal of returning the manifest from the task directly, I think that's something we need to discuss on the exec team. I believe we should have a standard output for each of these functions, perhaps a dict with a
result
ormessage
or whatever key should contain result, in case we want to add meta information to that result object later.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 is fine with me too. Gerda consolidated all the events in this task, anyway, to just
ParseCmdOut
(in themain
branch). We'll just want to remove that event now that it's no longer being called anywhere. For simplicity, let's track that in a separate issue, rather than try to do branch merging shenanigans here.The point I was making above was around,
dbt parse --compile
would also conditionally include the step around manifest compilation / graph generation, which can be very slow (as we've seen in recent reports/issues from folks with large projects). Let's track that in the new issue, "Abstract manifest compilation / graph generation from tasks."Agreed! Let's keep discussing in the separate linked issue. The biggest considerations are:
results
relatively consistent for all commands (although different tasks already return differently typed result objects)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.
We might want to call these changes out here:
https://docs.getdbt.com/guides/migration/versions/upgrading-to-v1.5
Did the proposed "Abstract manifest compilation / graph generation from tasks." issue ever get created?
In the meantime, I'm going to delete both of these from
params.py
as part of #6546 since they don't appear to do anything:dbt-core/core/dbt/cli/params.py
Lines 44 to 49 in 3f76f82
dbt-core/core/dbt/cli/params.py
Lines 491 to 496 in 3f76f82
This file was deleted.