-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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/improve rpc compile performance (#1824) #1830
Feature/improve rpc compile performance (#1824) #1830
Conversation
Create new helper function dbt.perf_utils.get_full_manifest Update task.runnable accordingly Update RPC server accordingly
initial refactoring of adapter factory stuff Move HasCredentials protocol into connection contract and use that in the base connection
RemoteCallableResult -> RPCResult RemoteCallable -> RemoteMethod - move some things from RPCTask -> RemoteMethod - recursive_subclasses classmethod things in core/dbt/rpc now are all based on RemoteMethods, not RPCTasks
The _sql tasks now compile any ref'ed CTE chains at RPC call time Give RPC tasks their own folder - task/rpc_server -> task/rpc/server - task/remote -> task/rpc/{project_commands,sql_commands,base} Linker enhancements: - Expose subset graph building so multiple methods can use it - Expose a way for the linker to provide an interable of the ephemeral ancestors of a node - it's guaranteed to be ordered (so nested CTEs behave)
Some circular import cleanups remove is_type function, just compare to resource_type Add type checking for dbt deps
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 really great! Empirically, it takes the time to hup the rpc server from ~26 seconds all the way down to ~1 second for a typical-looking dbt project :D
While playing around with this PR, I did notice that dbt is logging ephemeral
models as "running" in the poll
method logs, which probably isn't exactly right. I don't think that change was introduced in this PR though, so I'll create a new issue to address that.
Were the changes around deps.py
all related to mypy? Those look non-trivial to me...
core/dbt/contracts/graph/manifest.py
Outdated
@@ -415,6 +387,7 @@ def patch_nodes(self, patches): | |||
'not found or is disabled').format(patch.name) | |||
) | |||
|
|||
# TODO: why is this here? |
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.
👁 👁
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 really don't know. I'm going to remove it and push and if the tests pass, we'll call it good to go?
It's actually mostly type annotations! I did rename some things for clarity. |
Improve RPC startup/sighup performance by only parsing, not compiling
Fixes #1824
On a local demo case, server ready time improved from 24s to 14s, but it's a weird test project so don't get too excited. I'm interested to see real-world data...
expect
method that does a node lookup and raises an internal error if it's missingunique_id
not being in nodes is an unrecoverable internal logic errorI've rebased extensively to try to keep the various commits sensible and digestible chunks of the PR, though tests may be out of whack in some places. That may make it easier to review...