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

Specify macro_namespace of global_project dispatch macros #3851

Merged
merged 4 commits into from
Sep 9, 2021

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Sep 2, 2021

resolves #3456

Fuller description in the issue. Big idea is, you can reimplement a built-in global macro (including your adapter's built-in version) with a version that's defined in a package.

# dbt_project.yml
dispatch:
  - macro_namespace: dbt
    search_order: ['my_root_project', 'my_cool_package', 'dbt']

I think this just works for existing functionality. I'll create some tests for the net-new functionality described above.

Following the conversation in #3830, I also added dispatching for generate_schema_name and generate_alias_name. Funny enough, generate_database_name was already dispatched.

This wasn't on our v1.0 list, but it feels quite 1.0-y, so I'd be excited to get it in before then.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Sep 2, 2021
@jtcohen6 jtcohen6 changed the title Specify macro_namespace of global_project dispatch macros [WIP] Specify macro_namespace of global_project dispatch macros Sep 2, 2021
@jtcohen6 jtcohen6 changed the title [WIP] Specify macro_namespace of global_project dispatch macros Specify macro_namespace of global_project dispatch macros Sep 3, 2021
@jtcohen6 jtcohen6 mentioned this pull request Sep 3, 2021
4 tasks
@jtcohen6 jtcohen6 marked this pull request as ready for review September 3, 2021 16:01
@jtcohen6 jtcohen6 requested review from gshank and leahwicz September 3, 2021 16:03
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

This feels like it ought to have always worked this way. LGTM!

@jpeak-intellify
Copy link

jpeak-intellify commented Sep 7, 2021

Steps to evaluate a feature branch

Really struggled to pip install just this branch to evaluate it which I have used these strategies before:
https://stackoverflow.com/questions/20101834/pip-install-from-git-repo-branch

Turns out I had to git clone the repo locally, checkout the branch then:

pip install --upgrade pip
pip install --upgrade setuptools wheel twine check-wheel-contents
cd scripts
. ./build-dist.sh

Then in my target toy dbt project:

python3 -m venv .venv
. ./.venv/bin/activate
pip install -U pip
cd path/to/dbt/dist
pip install *.whl
cd -

Current issue evaluating feature

So I finally got our dbt_bigcorp_util installed and it dispatched but,
that package has sub macros that could not resolve
and it's not obvious as a (novice) package developer how to handle this.

Error output:

λ dbt compile --no-version-check                             
Running with dbt=0.21.0-b1
* Deprecation Warning: The "packages" argument of adapter.dispatch() has been
deprecated. Use the "macro_namespace" argument instead.
Raised during dispatch for: except
For more information, see:
https://docs.getdbt.com/reference/dbt-jinja-functions/dispatch
Encountered an error:
Compilation Error in macro default__generate_database_name (macros/administration/generate_database_name.sql)
  'resolve_database_name' is undefined
  
  > in macro generate_database_name (macros/etc/get_custom_database.sql)
  > called by macro default__generate_database_name (macros/administration/generate_database_name.sql)

Does that make sense? I admit this is likely something trivial in the dbt ecosystem I am missing and nothing to do with the specifics of this PR.

Test strategy

My testing strategy to check our custom package is overriding the global macros for naming databases and schemas is to just run a dbt compile then check the output of target/manifest.json

@jpeak-intellify
Copy link

Following up, I got it working by name spacing calls to the internal macros with the name of the package.

Eg
default__generate_database_name now internally makes a call to dbt_bigcorp_utils.resolve_database_name instead of the bare resolve_database_name.

I thought it was something simple on my end.

This looks good to use immediately for our use case. 👍

@jtcohen6 jtcohen6 force-pushed the feat/allow-dispatch-global-macros branch from 343402a to 869f0fb Compare September 9, 2021 10:41
@jtcohen6 jtcohen6 merged commit 44e7390 into develop Sep 9, 2021
@jtcohen6 jtcohen6 deleted the feat/allow-dispatch-global-macros branch September 9, 2021 10:59
TeddyCr pushed a commit to TeddyCr/dbt that referenced this pull request Sep 9, 2021
)

* Specify macro_namespace of global_project dispatch macros

* Dispatch get_custom_alias, too

* Add integration tests

* Add changelog entry
@jpeak-intellify
Copy link

For what it is worth, rolled this out to 9 projects.
✅ Added 609 lines of code
💥 Removed 4052 lines of code
Also upgraded to v0.21 goodness in the process 💪

@jpeak-intellify
Copy link

Just following up here to save time explaining the context of the feature/bug I am experiencing before opening an issue where I have to repeat the context.

dbt_project.yml

dispatch:
  - macro_namespace: dbt
    search_order: ['dbt_sample', 'my_macro_package', 'dbt']

But because I refactored all of my macros into my_macro_package, my macros/ folder in my root dbt_sample project is empty.

So the above config gives the following error message:

Running with dbt=0.21.0
Encountered an error:
Compilation Error
  In dispatch: Could not find package 'dbt_sample'

The only workaround I can see is to change it to the following if the root project has no macros:
dbt_project.yml

dispatch:
  - macro_namespace: dbt
    search_order: ['my_macro_package', 'dbt']

I'm not a huge fan of having to change preference config to match the existence of filesystem content. Feels like an opportunity for automation instead of manually twizzling config to suit.

If it is already reported and being addressed, a nudge to the existing issue would be appreciated so I can follow.
Otherwise if it is non-trivial, I'll create the full issue ticket to track the flow of work.

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Nov 8, 2021

@jpeak-intellify Appreciate you putting the comment here! I think we made a semi-related change that will incidentally fix this (#4114), by catching the mid-dispatch Compilation Error instead of raising it here.

We've backported that fix for v0.21.1, which is currently available as a release candidate. Could you take that for a spin, and see if it resolves the issue?

pip install dbt-<adapter>==0.21.1rc1

(where <adapter> is whichever one of postgres/redshift/snowflake/bigquery/etc that you're using)

@jpeak-intellify
Copy link

Love your work. Yeah that nailed the issue. 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow dispatch & overrides of global macros via packages
3 participants