-
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
Rework adapter.dispatch(), remove packages arg #3383
Comments
I think I 90% understand where this is coming from, and I'm sure I'll get the remaining 10% as I brood on it. Thanks @jtcohen6 your strong written communication skills! I'm also pretty certain I've seen the missing shim macro described in #3362 while debugging I'm also onboard with the new default search spacePerhaps explicit search path would be a happy path for users new to shimming to develop a set of shims for a package. e.g. a user who wants to make a proposed change to tsql-utils could put this in their current project then put whatever config:
- macro_namespace: dbt-utils
search_order: ['dbt_utils', 'my_project', 'tsql-utils'] before it seems that macros in the user's current project were available to the package being shimmed, right? Maybe this was never the case and I would just override the main macro rather than trying to put a shim into the implication for dispatched adaptersthis question that might be more about with this proposal, would all projects, using the config:
- macro_namespace: dbt-sqlserver
search_order: ['spark_utils', 'dbt_utils'] |
Thanks for the quick read + feedback @swanderz!
I sure hope not! That's an ugly error. I'm not positive what's causing it, but I very much hope that this change will simplify the experience of building and testing shim (and shim-able) packages.
That's right. You can just as easily shim As far as adapter inheritance in v0.20.0: This is built-in, and not something a user needs to specify in their So in general, I think a config like this makes a lot of sense: dispatch:
- macro_namespace: dbt_utils
search_order: ['my_project', 'tsql_utils', 'dbt_utils'] That is, when dispatching
The first one it finds is the one it will use. To put the same logic in English:
Question: Should we name |
We've run into some issues (#3362) with statically analyzing macros, which is required to resolve v0.19.1 bugs, continue our performance work, and enable compelling features in the future.
The big blocker is that
adapter.dispatch()
is not-quite statically analyzable today, since thepackages
argument can accept a Jinja macro. We're working on a fix that will be backwards compatible for how we know this is used in the wild today, by packages such as dbt-utils, fivetran-utils, and dbt-expectations.Namely, so long as the user sets a variable named
<package_name>_dispatch_list
, dbt will setpackages
to the equivalent ofvar('<package_name>_dispatch_list', []) + ['<package_name']
. All open source packages I know of that lean on dispatch have followed this convention for the_get_namespaces()
macro that they currently pass to thepackages
argument of dispatched macros.In the meantime, we also want to change
dispatch()
to:Proposal
The Jinja macro
adapter.dispatch()
should only take one argument,macro_name
(a string literal). Package maintainers shouldn't need to think about what kind of vars/macros to pass into thepackages
arg at all.For users, instead of setting a magic variable name (e.g.
dbt_utils_dispatch_list
), we should support a new config specification indbt_project.yml
:When dbt goes to dispatch a macro, it uses the
search_order
corresponding to themacro_namespace
(package) of the dispatching macro. This accomplishes the same functionality as the currentpackages
+ vars setup, but with much less confusion, and it centralizes all macro "routing" logic in the project file. (If a dispatching macro does not have a "route" set in the projectdispatch
config, it just searches within its own project—the same as current behavior whenpackages
isNone
.)This should also make the syntax for
dispatch
a little less onerous:Whether the python
dispatch
argument still takes two arguments, withsearch_order
passed in aspackages
, is an implementation detail. But in v0.20.0, we should start raising a deprecation warning if the user passes a value to thepackages
argument of theadapter.dispatch()
Jinja macro.And in a future version of dbt, once everyone has had a chance to upgrade to this new way of dispatching, we should remove the Jinja AST-parsing logic that #3363 is adding for backwards compatibility.
The text was updated successfully, but these errors were encountered: