Skip to content

Add back depends_on for seeds - only macros, never nodes#6851

Merged
jtcohen6 merged 9 commits intomainfrom
jerco/fix-6806
Feb 9, 2023
Merged

Add back depends_on for seeds - only macros, never nodes#6851
jtcohen6 merged 9 commits intomainfrom
jerco/fix-6806

Conversation

@jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Feb 3, 2023

resolves #6806

Description

  • Seeds can have pre/post hooks that call macros, and we need to track these in the seed's depends_on
  • If those macros try to take a dependency on another node, this is an unsupported anti-pattern — raise an explicit error

I'm not thrilled with this approach, which requires a bit of mucking with magic methods, but it does keep the changes fairly self-contained.

If we do move forward with this approach, we'll need to:

Checklist

@jtcohen6 jtcohen6 requested review from a team as code owners February 3, 2023 12:57
@jtcohen6 jtcohen6 requested review from gshank and mikealfare February 3, 2023 12:57
@cla-bot cla-bot bot added the cla:yes label Feb 3, 2023
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.

I think the approach is fine. We do already use getattr in a couple of other places and it's the only real place to put a user friendly error message if somebody tries to use the missing and prohibited methods.

@joeinnyc
Copy link

joeinnyc commented Feb 6, 2023

+1 on this please

@gshank
Copy link
Contributor

gshank commented Feb 6, 2023

It occurred to me that instead of using setattr, we could provide "property" methods for ref/sources/metrics that throws an error. Either way is fine with me.

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Feb 7, 2023

It occurred to me that instead of using setattr, we could provide "property" methods for ref/sources/metrics that throws an error.

@gshank I like that much better! Significantly less magic.

@jtcohen6 jtcohen6 merged commit 298bf8a into main Feb 9, 2023
@jtcohen6 jtcohen6 deleted the jerco/fix-6806 branch February 9, 2023 09:56
github-actions bot pushed a commit that referenced this pull request Feb 9, 2023
* Extend functional tests for seeds w hooks

* Add MacroDependsOn to seeds, raise exception for other deps

* Add changelog entry

* Fix unit tests

* Update upgrade_seed_content

* Cleanup

* Regen manifest v8 schema. Fix tests

* Be less magical

* PR feedback

(cherry picked from commit 298bf8a)
jtcohen6 added a commit that referenced this pull request Feb 9, 2023
…, never `nodes` (#6920)

* Add back `depends_on` for seeds - only `macros`, never `nodes` (#6851)

* Extend functional tests for seeds w hooks

* Add MacroDependsOn to seeds, raise exception for other deps

* Add changelog entry

* Fix unit tests

* Update upgrade_seed_content

* Cleanup

* Regen manifest v8 schema. Fix tests

* Be less magical

* PR feedback

(cherry picked from commit 298bf8a)

* Update manifest v8 for v1.4.x

* Cleanup

---------

Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
@sfc-gh-pkommini
Copy link

Hi Team, Which release would the changes in this PR make it into?

@jtcohen6
Copy link
Contributor Author

@sfc-gh-pkommini This has been backported for inclusion in the next v1.4.x patch release. There are a few more in-progress bug fixes we're hoping to include; at this point, we'll probably put out a release candidate next week.

@smitsrr
Copy link

smitsrr commented Feb 20, 2023

Second - looking for an update as we're blocked from upgrading until this bug is included in a release.

@jtcohen6
Copy link
Contributor Author

@sfc-gh-pkommini @smitsrr This fix has been included in a release candidate of v1.4.2 (next patch). Could you try it out and confirm that it fixes the issue for you?

pip install dbt-core==1.4.2rc1 dbt-<youradapter>

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.

[CT-1984] [Regression] In v1.4, seeds with hooks calling macros raise an AttributeError

6 participants