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

[CT-2463] Add “exclude_target” to packages config #7434

Open
3 tasks done
dbrtly opened this issue Apr 22, 2023 · 11 comments
Open
3 tasks done

[CT-2463] Add “exclude_target” to packages config #7434

dbrtly opened this issue Apr 22, 2023 · 11 comments
Labels
deps dbt's package manager enhancement New feature or request help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors

Comments

@dbrtly
Copy link
Contributor

dbrtly commented Apr 22, 2023

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature

`
packages:

Describe alternatives you've considered

Edit dbt_packages.yml in build pipeline

Who will this benefit?

Open source community.

Are you interested in contributing this feature?

Yes point to the file/s with description and I’ll do it

Anything else?

No response

@dbrtly dbrtly added enhancement New feature or request triage labels Apr 22, 2023
@github-actions github-actions bot changed the title Add “exclude_target” to packages config [CT-2463] Add “exclude_target” to packages config Apr 22, 2023
@jtcohen6
Copy link
Contributor

Thanks for opening @dbrtly!

We've gotten this issue in the past : #4837

I've come around to this, it does feel like there are use cases for it.

Which of these do you prefer?

    exclude_targets: [“prod”]
    exclude: "{{ target.name == 'prod' }}"

The latter opens up the possibility of doing conditional configuration with env vars. It could also be enabled: false|true, instead of exclude: true|false, for consistency with other dbt resource configuration.


I think this new attribute would want to be on the base Package class:

@dataclass
class Package(Replaceable, HyphenatedDbtClassMixin):
pass

And then the exclusion logic would happen within the deps logic. I think we'd want it to be within resolve_packages, to accomplish the "nolist" behavior proposed in #4837 (comment). E.g. setting

packages:
  - package: dbt-labs/snowplow
    exclude: true

Should also have the effect of preventing other packages from installing it as a transitive dependency. If those packages need models/macros from the snowplow package, and you haven't disabled them - you're on your own. What do you think?

@jtcohen6 jtcohen6 added help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors deps dbt's package manager and removed triage labels Apr 22, 2023
@dbrtly
Copy link
Contributor Author

dbrtly commented Apr 22, 2023

exclude: "{{ target.name == 'prod' }}"

This is the one.

@dbrtly
Copy link
Contributor Author

dbrtly commented Apr 22, 2023

I see the primary purpose as enabling packages in non-prod but not bloating prod.

What if another package rely on a target-excluded package, dbt compile would throw an error message?

Is that easy to add?

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 22, 2023

I see the primary purpose as enabling packages in non-prod but not bloating prod.

Ah! Workflow tooling - codegen, audit_helper? That makes sense to me!

dbt compile would throw an error message?

Yeah, if an installed package has enabled resources that call macros from / depend on models from an excluded package, it would raise a parsing/compilation error.

The user in #4837 was talking specifically about the fivetran/ad_reporting package, which is really like a "package of packages." The maintainers of that package already ask users to set variables enabling the inputs they need (linkedin, pinterest), and disable the ones they don't (tiktok, snapchat). But it still installs all the packages every time. So it would be nice if they made it easy to actually exclude those packages/resources (avoid bloat) — rather than installing & including them, just to end up disabling them.

@dbrtly
Copy link
Contributor Author

dbrtly commented Apr 22, 2023

Just realised this is potentially a double negative

exclude: "{{ target.name == 'prod' }}"

This would be easier to grok:

enable: "{{ target.name == 'prod' }}"

With default True

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 22, 2023

I'm with you, this is easier to grok:

    enabled: True  # default, implied -- I want to install this package
    enabled: False # I do *not* want to install this package

So e.g.:

packages:
  - package: dbt-labs/codegen
    version: 0.9.0
    enabled: "{{ target.name != 'prod' }}"

@dbrtly
Copy link
Contributor Author

dbrtly commented Apr 22, 2023

@nickperrott
Copy link

Hiya,
This sounds like exactly what I need.

A few words on my use case:
We are working on a SAAS dw product where the core (sellable) portion of the system is in a package that is referenced from customer specific dbt projects.

So far, so good. The issue I have, is that I am working heavily on implementation for our reference customer. The best setup/structure for development that I have found so far, is to use a git submodule to bring in our core code and a local package definition in packages.xml. This allows me to work on both core and customer specific changes without having to juggle git repos etc... too much.

It works well, but complicates our CI/CD as we currently need to have specific branches for each stack (test, prod etc..) that have their own distinct packages.yml in each branch that specifies the git repo as the source of core (rather than the local package) and the specific tagged revision from the core to use.

Ideally, I would like this to be handled somewhat automatically based upon target, which sounds exactly like what this new feature being discussed would allow.

Thanks for picking this up!

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 28, 2023

Looks like a solid start: dbrtly@22bdf38

Shall I add tests here? https://github.com/dbt-labs/dbt-core/blob/ca231489082fd9f1feef55e8e454068444d34178/test/unit/test_deps.py

& that looks like the right spot!

Worth verifying that it will accomplish this behavior described above:

[Setting enabled: False explicitly for a package] should also have the effect of preventing other packages from installing it as a transitive dependency.

@Maayan-s
Copy link

Adding another use case here from a package developer perspective -
The current state of using enabled for package models causes issues, so I believe the suggested solution is needed.

Example:
We have models that are not supported in Databricks, so we added this to their config block:
enabled = target.type != 'snowflake' and target.type != 'spark' | as_bool()

We have users that don't want Elementary to run in dev, so they configure in their dbt_project.yml:

models:
  elementary:
     enabled: "{{ target.name != 'dev' }}"

In production, their configuration overrides the model configuration (as the user config > package config, which makes sense). This causes failures on their runs.

@dbrtly
Copy link
Contributor Author

dbrtly commented Jul 9, 2023

I took a break from this for a while because I couldn't get the tests to run and it got overwhelming. Trying again.

I recloned, created a virtual environment as per contributing and ran make dev.
Without making changes on main, I can't get the tests to run successfully. I have python 3.11.1 as default on my machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps dbt's package manager enhancement New feature or request help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors
Projects
None yet
Development

No branches or pull requests

4 participants