z3z1ma / dbt-osmosis

Provides automated YAML management, a dbt server, streamlit workbench, and git-integrated dbt model output diff tools
https://z3z1ma.github.io/dbt-osmosis/
Apache License 2.0
422 stars 45 forks source link

Using dbt query_comment breaks project registration #31

Closed dh-racheldean closed 1 year ago

dh-racheldean commented 1 year ago

Symptoms: My dbt installation uses the query-comment config to add labels to BigQuery queries. In our dbt_project.yml we have

query-comment:
  comment: "{{ query_comment(node) }}"
  job-label: true

When trying to run a dbt-osmosis server serve...., the server throws a 500 Could not connect to Database error.

Diagnosis: Trailing things through the code, I reached parse_project in osmosis.py. This inits the DB adapter, loads the manifest, and importantly calls save_macros_to_adapter. BUT... before we get to the save macros, the adapter setter fn calls _verify_connection which tries to query the DB before the adapter is fully initialised...

Now the adapter errors in this chunk in the dbt-bigquery connection

if (
            hasattr(self.profile, "query_comment")
            and self.profile.query_comment
            and self.profile.query_comment.job_label
        ):
            query_comment = self.query_header.comment.query_comment. 
            labels = self._labels_from_query_comment(query_comment)

failing with 'NoneType' object has no attribute 'comment' on line 6 of the snippet. Now this code looks ugly - it's checking the contents of self.profile, then trying to access self.query-header - clearly a massive assumption is made here...

So - we have a conflict. dbt-osmosis is trying to run a query without ensuring the adapter is completely configured - and dbt-bigquery is doing a really wonky test. I'm happy to PR a deferred verify process in dbt-osmosis, or if you consider this to be more of a dbt-bigquery bug, I'll raise it with them.

z3z1ma commented 1 year ago

This looks to be resolved in latest versions. I use bigquery myself and tested this. I think the migration to the vendored dbt-core-interface smoothed out adapter management.