Skip to content
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

Merged
merged 18 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Under the Hood-20230110-115725.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Under the Hood
body: Abstract manifest generation
time: 2023-01-10T11:57:25.193965-06:00
custom:
Author: stu-k
Issue: "6357"
42 changes: 32 additions & 10 deletions core/dbt/cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@

import click
from dbt.cli import requires, params as p
from dbt.config import RuntimeConfig
from dbt.config.project import Project
from dbt.config.profile import Profile
from dbt.contracts.graph.manifest import Manifest
from dbt.task.clean import CleanTask
from dbt.parser.manifest import write_manifest
from dbt.task.compile import CompileTask
from dbt.task.deps import DepsTask
from dbt.task.run import RunTask
Expand Down Expand Up @@ -156,7 +156,6 @@ def docs(ctx, **kwargs):
@p.exclude
@p.profile
@p.profiles_dir
@p.project_dir
@p.select
@p.selector
@p.state
Expand Down Expand Up @@ -208,11 +207,15 @@ def docs_serve(ctx, **kwargs):
@p.vars
@p.version_check
@requires.preflight
@requires.profile
@requires.project
@requires.runtime_config
@requires.manifest
def compile(ctx, **kwargs):
"""Generates executable SQL from source, model, test, and analysis files. Compiled SQL files are written to the
target/ directory."""
config = RuntimeConfig.from_parts(ctx.obj["project"], ctx.obj["profile"], ctx.obj["flags"])
task = CompileTask(ctx.obj["flags"], config)
task = CompileTask(ctx.obj["flags"], ctx.obj["runtime_config"])
_set_manifest_on_task(task, ctx)

results = task.run()
success = task.interpret_results(results)
Expand Down Expand Up @@ -250,7 +253,6 @@ def debug(ctx, **kwargs):
def deps(ctx, **kwargs):
"""Pull the most recent version of the dependencies listed in packages.yml"""
task = DepsTask(ctx.obj["flags"], ctx.obj["project"])

results = task.run()
success = task.interpret_results(results)
return results, success
Expand Down Expand Up @@ -309,9 +311,13 @@ def list(ctx, **kwargs):
@p.version_check
@p.write_manifest
@requires.preflight
@requires.profile
@requires.project
@requires.runtime_config
@requires.manifest(write_perf_info=True)
def parse(ctx, **kwargs):
"""Parses the project and provides information on performance"""
click.echo(f"`{inspect.stack()[0][3]}` called\n flags: {ctx.obj['flags']}")
_write_manifest(ctx)
return None, True
Copy link
Contributor

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 (= overwrite manifest.json). IMO that's a good change! I'd opened an issue for it recently: [CT-1759] dbt parse should (over)write manifest.json by default #6534. So we can also remove --write-manifest as a parameter for this command, and in params.py.
  • We no longer have the option of 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:

  • Manifest compilation / graph generation does require the adapter, and therefore RuntimeConfig, as an input
  • It mutates the manifest and returns a Graph
  • The build task produces + uses a different graph from all other tasks, with extra test edges
  • If we stored the graph on ctx.obj, and passed it into each task explicitly, we would unlock the ability for programmatic invocations to cache + reuse the graph 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 and build-specific graphs.)

Finally, a separate proposal, out of scope for this PR but highly relevant: The parse task should return the Manifest here, instead of None (#6547). That could be as simple as changing this last line to:

def parse(ctx, **kwargs) -> Tuple[Manifest, bool]:
    """Parses the project and provides information on performance"""
    # manifest generation and writing happens in @requires.manifest
    return ctx.obj["manifest"], True

And then:

from dbt.cli.main import dbtRunner
dbt = dbtRunner()
manifest, _ = dbt.invoke(['parse'])

There's a bit more refinement we should do on that proposal first, to make sure it's an idea we're happy with.

Copy link
Contributor Author

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 or message or whatever key should contain result, in case we want to add meta information to that result object later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This is fine with me too. Gerda consolidated all the events in this task, anyway, to just ParseCmdOut (in the main 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."


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 or message or whatever key should contain result, in case we want to add meta information to that result object later.

Agreed! Let's keep discussing in the separate linked issue. The biggest considerations are:

  • How to keep the results relatively consistent for all commands (although different tasks already return differently typed result objects)
  • Should we expose the full manifest, as part of our public API? Just a part of it? Should we call this "experimental" functionality (liable to change in future versions)? We have the power to decide these things!

Copy link
Contributor

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:

compile_parse = click.option(
"--compile/--no-compile",
envvar=None,
help="TODO: No help text currently available",
default=True,
)

write_manifest = click.option(
"--write-manifest/--no-write-manifest",
envvar=None,
help="TODO: No help text currently available",
default=True,
)



Expand All @@ -336,10 +342,12 @@ def parse(ctx, **kwargs):
@requires.preflight
@requires.profile
@requires.project
@requires.runtime_config
@requires.manifest
def run(ctx, **kwargs):
"""Compile SQL and execute against the current target database."""
config = RuntimeConfig.from_parts(ctx.obj["project"], ctx.obj["profile"], ctx.obj["flags"])
task = RunTask(ctx.obj["flags"], config)
task = RunTask(ctx.obj["flags"], ctx.obj["runtime_config"])
_set_manifest_on_task(task, ctx)

results = task.run()
success = task.interpret_results(results)
Expand Down Expand Up @@ -455,16 +463,30 @@ def freshness(ctx, **kwargs):
@p.vars
@p.version_check
@requires.preflight
@requires.profile
@requires.project
@requires.runtime_config
@requires.manifest
def test(ctx, **kwargs):
"""Runs tests on data in deployed models. Run this after `dbt run`"""
config = RuntimeConfig.from_parts(ctx.obj["project"], ctx.obj["profile"], ctx.obj["flags"])
task = TestTask(ctx.obj["flags"], config)
task = TestTask(ctx.obj["flags"], ctx.obj["runtime_config"])
_set_manifest_on_task(task, ctx)

results = task.run()
success = task.interpret_results(results)
return results, success


def _set_manifest_on_task(task, ctx):
_write_manifest(ctx)
task.set_manifest(ctx.obj["manifest"])


def _write_manifest(ctx):
if ctx.obj["flags"].write_json:
write_manifest(ctx.obj["manifest"], ctx.obj["runtime_config"].target_path)


# Support running as a module
if __name__ == "__main__":
cli_runner()
65 changes: 64 additions & 1 deletion core/dbt/cli/requires.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
from dbt.adapters.factory import adapter_management
from dbt.adapters.factory import adapter_management, register_adapter
from dbt.cli.flags import Flags
from dbt.config import RuntimeConfig
from dbt.config.runtime import load_project, load_profile
from dbt.events.functions import setup_event_logger
from dbt.exceptions import DbtProjectError
from dbt.parser.manifest import ManifestLoader
from dbt.profiler import profiler
from dbt.tracking import initialize_from_flags, track_run

Expand Down Expand Up @@ -85,3 +87,64 @@ def wrapper(*args, **kwargs):
return func(*args, **kwargs)

return update_wrapper(wrapper, func)


def runtime_config(func):
"""A decorator used by click command functions for generating a runtime
config given a profile and project.
"""

def wrapper(*args, **kwargs):
ctx = args[0]
assert isinstance(ctx, Context)

req_strs = ["profile", "project"]
reqs = [ctx.obj.get(req_str) for req_str in req_strs]

if None in reqs:
raise DbtProjectError("profile and project required for runtime_config")

ctx.obj["runtime_config"] = RuntimeConfig.from_parts(
ctx.obj["project"],
ctx.obj["profile"],
ctx.obj["flags"],
)

return func(*args, **kwargs)

return update_wrapper(wrapper, func)


def manifest(*args0, write_perf_info=False):
stu-k marked this conversation as resolved.
Show resolved Hide resolved
"""A decorator used by click command functions for generating a manifest given a profile, project,
and runtime config. This also registers the adaper from the runtime config.
"""

def outer_wrapper(func):
def wrapper(*args, **kwargs):
ctx = args[0]
assert isinstance(ctx, Context)

req_strs = ["profile", "project", "runtime_config"]
reqs = [ctx.obj.get(dep) for dep in req_strs]

if None in reqs:
raise DbtProjectError("profile, project, and runtime_config required for manifest")

runtime_config = ctx.obj["runtime_config"]
register_adapter(runtime_config)
manifest = ManifestLoader.get_full_manifest(
runtime_config, write_perf_info=write_perf_info
)

ctx.obj["manifest"] = manifest

return func(*args, **kwargs)

return update_wrapper(wrapper, func)

# if there are no args, the decorator was used without params @decorator
# otherwise, the decorator was called with params @decorator(arg)
if len(args0) == 0:
return outer_wrapper
return outer_wrapper(args0[0])
Comment on lines +152 to +156
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better more pythonic way to have a decorator accept args?

Binary file modified core/dbt/docs/build/doctrees/environment.pickle
Binary file not shown.
Binary file modified core/dbt/docs/build/doctrees/index.doctree
Binary file not shown.
2 changes: 1 addition & 1 deletion core/dbt/docs/build/html/.buildinfo
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Sphinx build info version 1
# This file hashes the configuration used when building these files. When it is not found, a full rebuild will be done.
config: e27d6c1c419f2f0af393858cdf674109
config: 0d25ef12a43286020bcd8b805064f01c
tags: 645f666f9bcd5a90fca523b33c5a78b7
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/*
* _sphinx_javascript_frameworks_compat.js
* ~~~~~~~~~~
*
* Compatability shim for jQuery and underscores.js.
*
* WILL BE REMOVED IN Sphinx 6.0
* xref RemovedInSphinx60Warning
*
*/

/**
* select a different prefix for underscore
*/
$u = _.noConflict();


/**
* small helper function to urldecode strings
*
* See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent#Decoding_query_parameters_from_a_URL
*/
jQuery.urldecode = function(x) {
if (!x) {
return x
}
return decodeURIComponent(x.replace(/\+/g, ' '));
};

/**
* small helper function to urlencode strings
*/
jQuery.urlencode = encodeURIComponent;

/**
* This function returns the parsed url parameters of the
* current request. Multiple values per key are supported,
* it will always return arrays of strings for the value parts.
*/
jQuery.getQueryParameters = function(s) {
if (typeof s === 'undefined')
s = document.location.search;
var parts = s.substr(s.indexOf('?') + 1).split('&');
var result = {};
for (var i = 0; i < parts.length; i++) {
var tmp = parts[i].split('=', 2);
var key = jQuery.urldecode(tmp[0]);
var value = jQuery.urldecode(tmp[1]);
if (key in result)
result[key].push(value);
else
result[key] = [value];
}
return result;
};

/**
* highlight a given string on a jquery object by wrapping it in
* span elements with the given class name.
*/
jQuery.fn.highlightText = function(text, className) {
function highlight(node, addItems) {
if (node.nodeType === 3) {
var val = node.nodeValue;
var pos = val.toLowerCase().indexOf(text);
if (pos >= 0 &&
!jQuery(node.parentNode).hasClass(className) &&
!jQuery(node.parentNode).hasClass("nohighlight")) {
var span;
var isInSVG = jQuery(node).closest("body, svg, foreignObject").is("svg");
if (isInSVG) {
span = document.createElementNS("http://www.w3.org/2000/svg", "tspan");
} else {
span = document.createElement("span");
span.className = className;
}
span.appendChild(document.createTextNode(val.substr(pos, text.length)));
node.parentNode.insertBefore(span, node.parentNode.insertBefore(
document.createTextNode(val.substr(pos + text.length)),
node.nextSibling));
node.nodeValue = val.substr(0, pos);
if (isInSVG) {
var rect = document.createElementNS("http://www.w3.org/2000/svg", "rect");
var bbox = node.parentElement.getBBox();
rect.x.baseVal.value = bbox.x;
rect.y.baseVal.value = bbox.y;
rect.width.baseVal.value = bbox.width;
rect.height.baseVal.value = bbox.height;
rect.setAttribute('class', className);
addItems.push({
"parent": node.parentNode,
"target": rect});
}
}
}
else if (!jQuery(node).is("button, select, textarea")) {
jQuery.each(node.childNodes, function() {
highlight(this, addItems);
});
}
}
var addItems = [];
var result = this.each(function() {
highlight(this, addItems);
});
for (var i = 0; i < addItems.length; ++i) {
jQuery(addItems[i].parent).before(addItems[i].target);
}
return result;
};

/*
* backward compatibility for jQuery.browser
* This will be supported until firefox bug is fixed.
*/
if (!jQuery.browser) {
jQuery.uaMatch = function(ua) {
ua = ua.toLowerCase();

var match = /(chrome)[ \/]([\w.]+)/.exec(ua) ||
/(webkit)[ \/]([\w.]+)/.exec(ua) ||
/(opera)(?:.*version|)[ \/]([\w.]+)/.exec(ua) ||
/(msie) ([\w.]+)/.exec(ua) ||
ua.indexOf("compatible") < 0 && /(mozilla)(?:.*? rv:([\w.]+)|)/.exec(ua) ||
[];

return {
browser: match[ 1 ] || "",
version: match[ 2 ] || "0"
};
};
jQuery.browser = {};
jQuery.browser[jQuery.uaMatch(navigator.userAgent).browser] = true;
}
5 changes: 1 addition & 4 deletions core/dbt/docs/build/html/_static/basic.css
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*
* Sphinx stylesheet -- basic theme.
*
* :copyright: Copyright 2007-2023 by the Sphinx team, see AUTHORS.
* :copyright: Copyright 2007-2022 by the Sphinx team, see AUTHORS.
* :license: BSD, see LICENSE for details.
*
*/
Expand Down Expand Up @@ -324,15 +324,13 @@ aside.sidebar {
p.sidebar-title {
font-weight: bold;
}

nav.contents,
aside.topic,
div.admonition, div.topic, blockquote {
clear: left;
}

/* -- topics ---------------------------------------------------------------- */

nav.contents,
aside.topic,
div.topic {
Expand Down Expand Up @@ -608,7 +606,6 @@ ol.simple p,
ul.simple p {
margin-bottom: 0;
}

aside.footnote > span,
div.citation > span {
float: left;
Expand Down
2 changes: 1 addition & 1 deletion core/dbt/docs/build/html/_static/doctools.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*
* Base JavaScript utilities for all Sphinx HTML documentation.
*
* :copyright: Copyright 2007-2023 by the Sphinx team, see AUTHORS.
* :copyright: Copyright 2007-2022 by the Sphinx team, see AUTHORS.
* :license: BSD, see LICENSE for details.
*
*/
Expand Down
Loading