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

Regression: a profile should not be required to run dbt deps #2231

Closed
drewbanin opened this issue Mar 23, 2020 · 9 comments
Closed

Regression: a profile should not be required to run dbt deps #2231

drewbanin opened this issue Mar 23, 2020 · 9 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@drewbanin
Copy link
Contributor

Describe the bug

This is a regression in 0.16.0. Previous releases of dbt did not require a valid profile to exist in order to download deps stated in a packages.yml file.

TBD what the exact use case is here, but this is a regression, and we should either reinstate the previous behavior or recommend an alternative approach.

Steps To Reproduce

# dbt_project.ym;

name: debug
version: 1.0.0
$ dbt deps
Running with dbt=0.16.0
Encountered an error while reading the project:
  ERROR: Runtime Error
  dbt cannot run because no profile was specified for this dbt project.
  To specify a profile for this project, add a line like the this to
  your dbt_project.yml file:

  profile: [profile name]

Expected behavior

dbt deps does not strictly need a profile to operate, and dbt should be able to invoke the task without a valid profile.

Additional context

I do not think it is unreasonable to require that a valid profile exists (even with bad/incorrect data) to run dbt deps. I imagine the use-case has something to do with building a Docker image, or similar. Let's weigh the tradeoffs of fixing this regression with our goals of making compilation and task execution consistent and interrogable when considering what to do with this bug report.

@drewbanin drewbanin added the bug Something isn't working label Mar 23, 2020
@drewbanin drewbanin added this to the 0.16.1 milestone Mar 23, 2020
@Raalsky
Copy link
Contributor

Raalsky commented Mar 23, 2020

+1 Today I've bumped my dbt version to 0.16.0 and our CI pipeline was failed due to lookup for profile.yml 😢 . Definitely a regression. I could handle a PR.

@Raalsky
Copy link
Contributor

Raalsky commented Mar 23, 2020

I think it's due to dbt Contexts (b8febdd)
Prior to it DepsTask derives from ProjectOnlyTask changed to ConfiguredTask.
Currently ConfiguredTask always requests for profile configuration I think.

@drewbanin
Copy link
Contributor Author

Hey @Raalsky - #b8febdd is definitely the relevant change.

If you able and interested, it would be awesome if you could send through a PR! I just made a new dev branch - dev/0.16.1. Let us know if there's anything we can help out with in the implementation!

@beckjake
Copy link
Contributor

I'm pretty sure we did this for for the following chain of reasons:

  • we want target to be available when rendering configuration files, including most dbt_project.yml fields and packages.yml
  • for target to be available, we need to have a valid profile
  • deps downloads dependencies recursively
  • part of that involves (for non-hubsite dependencies) parsing the project file to find the dependencies' dependencies, etc
  • so we have to have a valid profile to run dbt deps

We can revisit any of those for sure, or see about offering some sort of workaround (no profile -> target becomes an object where accessing any attribute raises an error?)

@Raalsky
Copy link
Contributor

Raalsky commented Mar 25, 2020

@beckjake Thank you. I was looking for such a list. 👍

We can revisit any of those for sure, or see about offering some sort of workaround (no profile -> target becomes an object where accessing any attribute raises an error?)

I'm looking for something like a lazy profile load (I hope so 😉). But I think it could be too complicated for short PR. I think it's ok as hotfix if we fill something like default empty profile and raising only on attribute requests as you suggested?

@drewbanin
Copy link
Contributor Author

We want to prioritize consistency around how dbt compiles projects. This entails having consistent and well-documented compilation contexts for jinja strings, and therefore, consistent inputs into dbt's compilation. In thinking more about this issue, I'm increasingly hesitant to let dbt deps be invoked without a well-formulated profile. These extra code paths are a ripe source for bugs and user-error to pop up.

Here's a quick example: you can use {{ target.name }} pretty much everywhere jinja is accepted in dbt. If we make dbt deps run without a profile, then target won't be defined in the packages.yml compilation context. So, this is sort of diametrically opposed to the direction we're marching in with regards to how dbt compiles projects, and it makes me want to think really hard about how we should proceed here.

Just practically speaking, my instincts are that the target variable doesn't have a ton of merit in the compilation of the packages.yml file, but the point is kind of that my instincts should take a back seat to user comprehensibility anymore :)

@Raalsky - can you speak a little more about your requirements here? I do think providing a dummy profile should work well, but I want to make sure I understand your needs (and the needs of everyone else who is interested in this issue!) before making a final call

@Raalsky
Copy link
Contributor

Raalsky commented Mar 26, 2020

Just to be clear. The dbt context that was released in 0.16.0 it's really a big thing (and really good thing! 🚀). And I'm really existed about your work. All I think that is worth noticing is about the data privacy. I think we should treat the profile data as something really private and using it only when it's needed. As a user during my first impressions with dbt it’s not clear that something like dependency installation wants access to my connection data (profile) during request to an external service (if we actually use the registry or something). Of course, I know there could be the cases when we need/want to access the context in dbt_project.yml etc. but in general it's not clear.

It’s something similar to Android app installer with requesting permission popups. For ex. mail client calling me during installation that it need acccess to my microphone it's something confusing. I could not know that it's VoiceToText capable. But a request it's okay when I actually just pressed a record button.

What I would like to see (in future) is loading the profile actually only when I really need it or be more clear with how we treat private data.

A really really abstract vision but Python-capable (:stuck_out_tongue_winking_eye: ):

def lookup_for_your_profile_data_or_raise():
    print('I NEED YOUR DATA NOW!')
    return 'My Private Data'

class A(object):
    def __init__(self):
        self.data = None
        self._initialized = False

    def _initialize(self):
        self.data = lookup_for_your_profile_data_or_raise()


class Profile(A):
    def __init__(self):
        super().__init__()

    def __getattribute__(self, name):
        if not super().__getattribute__('_initialized'):
            super()._initialize()
        return super().__getattribute__(name)

# dbt deps without hacky target.name etc.
without_profile = Profile()
# OUTPUT: <NOTHING>

# dbt deps with hacky target.name etc.
with_profile = Profile()
with_profile.data
# OUTPUT: 'My Private Data'

The “empty profile” is just something to be backward compatible and I definitely don’t enforce it 😉! . I’m really excited about the direction to be more consistent in the codebase and during access through Jinja contexts. To be clear once more: It’s fine for me to break this today but looking for solutions that treat privacy data more carefully.

May the Force be with you!

@drewbanin
Copy link
Contributor Author

Thanks @Raalsky - I buy it!

Let's go ahead and pursue this approach of a "phony" profile. If you try to get any attributes of the profile, dbt should raise. This issue can be closed when dbt deps can run without a profile: declared in the dbt_project.yml file, or when the stated profile does not exist (ie. dbt shouldn't check for the profile).

Let's ship this for 0.16.1

@drewbanin drewbanin assigned beckjake and unassigned drewbanin Apr 1, 2020
beckjake added a commit that referenced this issue Apr 2, 2020
…required

Do not require a profile in "dbt deps" (#2231)
@drewbanin
Copy link
Contributor Author

closed by #2290

@jtcohen6 jtcohen6 mentioned this issue Jul 9, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants