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

HOTFIX setup.py doesn't include materializations folder #45

Merged
merged 8 commits into from
Mar 19, 2021

Conversation

alieus
Copy link
Contributor

@alieus alieus commented Mar 18, 2021

HOTFIX setup.py doesn't include materializations folder

@dataders
Copy link
Contributor

dataders commented Mar 19, 2021

Issue

setup()'s package_data param actually does need the below line because:

  • dbt-msft adapters have extra folders for the seed and snapshot materializations as compared to the standard adapters, and
  • distutils.core.setup does not glob as intended...
"include/sqlserver/macros/**/**/*.sql",

This escaped our integration testing bc in in tox, we're not building a distribution and instead opting in install in development mode, something we've already remedied in dbt-sqlserver with dbt-msft/dbt-sqlserver#105. cc: @JCZuurmond

Supporting Evidence

@alieus and I were able to confirm this by:

  • confirming that the tarball does not contain seed.sql (tar tzf dist/dbt-synapse-0.19.0.tar.gz)
  • looking at the dbt package installation after running pip install dbt-synapse

TL;DR

this is because distutils.core.setup does not interpret/glob ** the same as linux does, per this SO answer (excerpt below).

Double Asterisk (**)

Double Asterisk (**) matches zero or more characters across multiple segments. It is used for globbing files that are in nested directories.

Example: Tests/**/*.js

Here, the file selecting will be restricted to the Tests directory. The glob will match the files such as Tests/HelloWorld.js, Tests/UI/HelloWorld.js, Tests/UI/Feature1/HelloWorld.js.

does not glob in Python as I thought it would what I thought it would... what that /**/*.sql is does not recursively include these dirs

  • include/synapse/macros/materializations/snapshot
  • include/synapse/macros/materializations/seed

image

Discussion elsewhere

this Slack thread documents the issue.

@chaerinlee1 and I are seeing one of two issues come up sometimes which makes me think there’s a bug where it’s trying to use dbt-sqlserver’s macros instead of dbt-synapse's when calling dbt seed

('The SQL contains 0 parameter markers, but 800 parameters were supplied', 'HY000')

This also explains an internal thread (below) about why when I installed the package in development mode (pip install -e .) that the seed macro would be correctly loaded into dbt/include.

i know you're working on other stuff but i wanted to share this craziness from yesterday, I've been trying to figure out the weird dbt-synapse error we were seeing with dbt seed. here's my story....

  1. I couldn't get dbt seed working for ml mart w/ dbt-synapse v0.19.0

in a conda env called dbtenv
2. i decide to install dbt-synapse in dev mode so i can add log statements to the seed macros to make sure the right adapter macros were being called so i
1. created a new conda env, dbtdev
2. from the dbt-synapse adapter repo, called pip install -e.
3. so then i call dbt seed from mart ml and dbtdev conda env. it works! wtf?!, then
4. i switch to original dbtenv and call dbtenv to verify the original error.... and it works!!!!

how is this posssible? all i did was make a new conda env, then the error is resolved in the original conda env?!? wtf

@dataders dataders requested a review from jtcohen6 March 19, 2021 06:06
@jtcohen6
Copy link
Contributor

FYI we had the same problem in dbt-spark v0.19.0, and it escaped our notice in CI for the same reason. Here's the resolution we decided on: dbt-labs/dbt-spark#151

@JCZuurmond
Copy link
Contributor

The approach @jtcohen6 shows looks fine to me

@dataders dataders merged commit 269706b into master Mar 19, 2021
@sdebruyn sdebruyn deleted the alieus-hotfix-setup.py branch June 4, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants