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

Convert cli commands to use dbt executable path #83

Merged
merged 12 commits into from
Jan 12, 2023
Merged

Conversation

chrishronek
Copy link
Contributor

@chrishronek chrishronek commented Jan 11, 2023

Currently, we are sourcing the venv here. Instead, we should reference the dbt executable so that implementing the dbt-ol wrapper is easier.

Before:

dbt_path = [bash_path, "-c", f"source {self.python_venv} && dbt {self.base_cmd} "]

After:

{self.python_venv}/bin/dbt {self.base_cmd}

@chrishronek chrishronek added enhancement New feature or request invalid This doesn't seem right labels Jan 11, 2023
@chrishronek chrishronek self-assigned this Jan 11, 2023
@chrishronek chrishronek linked an issue Jan 11, 2023 that may be closed by this pull request
@chrishronek chrishronek marked this pull request as ready for review January 11, 2023 03:12
@chrishronek chrishronek requested a review from a team as a code owner January 11, 2023 03:12
@chrishronek
Copy link
Contributor Author

@jlaneve & @iancmoritz, after merging this, we should probably tag a new minor release - this is going to break some shit

@jlaneve
Copy link
Collaborator

jlaneve commented Jan 11, 2023

hmm I was thinking this would look more along the lines of:

DbtTaskGroup(
  ...,
  dbt_executable="/usr/local/airflow/dbt_venv/bin/dbt"
)

This would abstract away the venv, lineage, etc management from our operators and let the user handle it, so you could do something like:

DbtTaskGroup(
  ...,
  dbt_executable="/usr/local/airflow/dbt_venv/bin/dbt-ol"
)

And I believe we could also default it to just dbt (since, after pip installing it, it'll be in the Docker image's PATH).

@pohek321 what do you think?

@chrishronek
Copy link
Contributor Author

chrishronek commented Jan 11, 2023

hmm I was thinking this would look more along the lines of:

DbtTaskGroup(
  ...,
  dbt_executable="/usr/local/airflow/dbt_venv/bin/dbt"
)

This would abstract away the venv, lineage, etc management from our operators and let the user handle it, so you could do something like:

DbtTaskGroup(
  ...,
  dbt_executable="/usr/local/airflow/dbt_venv/bin/dbt-ol"
)

And I believe we could also default it to just dbt (since, after pip installing it, it'll be in the Docker image's PATH).

@pohek321 what do you think?

@jlaneve Ahhh, yeah that makes way more sense - that's a pretty easy change anyway. I'll get it done.

@chrishronek
Copy link
Contributor Author

Did some successful testing:

dbt-ol

Running dbt-ol (with venv)

Dockerfile:

FROM quay.io/astronomer/astro-runtime:7.1.0

# install python virtualenv to run dbt
WORKDIR /usr/local/airflow
COPY dbt-requirements.txt ./
RUN python -m virtualenv dbt_venv && source dbt_venv/bin/activate && \
    pip install --no-cache-dir -r dbt-requirements.txt && deactivate

dbt-requirements:

openlineage-dbt

Operator:

DbtDepsOperator(
    ...
    python_venv='/usr/local/airflow/dbt_venv/bin/dbt-ol'
)

Result:

['/usr/local/airflow/dbt_venv/bin/dbt-ol', 'deps', '--project-dir', '/usr/local/airflow/dbt/jaffle_shop', '--profile', '***_profile']

Running dbt-ol (no venv)

requirements.txt

openlineage-dbt

Operator:

DbtDepsOperator(
    ...
)

Result:

['/usr/local/bin/dbt-ol', 'deps', '--project-dir', '/usr/local/airflow/dbt/jaffle_shop', '--profile', '***_profile']

dbt

Running dbt (with venv)

Dockerfile:

FROM quay.io/astronomer/astro-runtime:7.1.0

# install python virtualenv to run dbt
WORKDIR /usr/local/airflow
COPY dbt-requirements.txt ./
RUN python -m virtualenv dbt_venv && source dbt_venv/bin/activate && \
    pip install --no-cache-dir -r dbt-requirements.txt && deactivate

dbt-requirements:

dbt-core

Operator:

DbtDepsOperator(
    ...
    python_venv='/usr/local/airflow/dbt_venv/bin/dbt'
)

Result:

['/usr/local/airflow/dbt_venv/bin/dbt', 'deps', '--project-dir', '/usr/local/airflow/dbt/jaffle_shop', '--profile', '***_profile']

Running dbt (no venv)

requirements.txt

dbt-core

Operator:

DbtDepsOperator(
    ...
)

Result:

['/usr/local/bin/dbt', 'deps', '--project-dir', '/usr/local/airflow/dbt/jaffle_shop', '--profile', '***_profile']

@chrishronek
Copy link
Contributor Author

@jlaneve sorry for the long comment ☝🏻

@jlaneve
Copy link
Collaborator

jlaneve commented Jan 11, 2023

nice! that looks great, can we rename python_venv to something like dbt_executable_path?

@chrishronek
Copy link
Contributor Author

nice! that looks great, can we rename python_venv to something like dbt_executable_path?

@jlaneve just committed it up -- thoughts?

Copy link
Collaborator

@jlaneve jlaneve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apologies for the big review here, but tried to be overly verbose to turn this into a checklist 😬

overall looking really good! just a handful of minor things

cosmos/providers/dbt/core/operators.py Outdated Show resolved Hide resolved
cosmos/providers/dbt/core/operators.py Outdated Show resolved Hide resolved
cosmos/providers/dbt/core/operators.py Show resolved Hide resolved
cosmos/providers/dbt/core/operators.py Outdated Show resolved Hide resolved
cosmos/providers/dbt/core/operators.py Outdated Show resolved Hide resolved
examples/dags/extract_dag.py Outdated Show resolved Hide resolved
examples/dags/extract_dag.py Outdated Show resolved Hide resolved
examples/dags/extract_dag.py Outdated Show resolved Hide resolved
examples/dags/jaffle_shop.py Outdated Show resolved Hide resolved
examples/dags/mrr-playbook.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jlaneve jlaneve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great!

@chrishronek chrishronek merged commit ec493bf into main Jan 12, 2023
@chrishronek chrishronek deleted the dbt-executable branch January 12, 2023 15:14
chrishronek added a commit that referenced this pull request Jan 12, 2023
@jlaneve @iancmoritz I forgot to bump the minor version. We probably
should do this after implementing
[this](#83) as it
broke some stuff.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify venv parameter to use dbt executable path instead of venv
2 participants