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

Use dbt ls as the default parser when profile_config is provided #1101

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

pankajastro
Copy link
Contributor

@pankajastro pankajastro commented Jul 17, 2024

Description

The load_via_custom_parser method is not very effective
when it comes to filtering. Currently, if the ExecutionMode
is not Local, we always use load_via_custom_parser to parse
the dbt project. However, I believe it's beneficial to utilize DBT LS
whenever it's available. This PR remove the check if the
execution mode is ExecutionMode.LOCAL before load_via_dbt_ls

Related Issue(s)

Related: #943

Breaking Change?

I believe no :)

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

Copy link

netlify bot commented Jul 17, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit f403292
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/669a23e040512d0008b407a8

cosmos/dbt/graph.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.47%. Comparing base (b25d5ea) to head (f403292).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1101      +/-   ##
==========================================
- Coverage   96.50%   96.47%   -0.04%     
==========================================
  Files          64       64              
  Lines        3288     3288              
==========================================
- Hits         3173     3172       -1     
- Misses        115      116       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pankajastro pankajastro marked this pull request as ready for review July 18, 2024 13:47
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. area:parsing Related to parsing DAG/DBT improvement, issues, or fixes parsing:dbt_ls Issues, questions, or features related to dbt_ls parsing labels Jul 18, 2024
@pankajkoti pankajkoti changed the title Use DBT LS as the default parser when profile_config is provided Use dbt ls as the default parser when profile_config is provided Jul 18, 2024
Copy link
Contributor

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

LGTM, let's wait for @tatiana's review.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 18, 2024
Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

@pankajastro Looks great - but, please, let's add a test to cover the use-case that encouraged this change - so we make sure it continues working in the future

@tatiana tatiana added this to the Cosmos 1.6.0 milestone Jul 18, 2024
@tatiana tatiana merged commit e61f3a3 into main Jul 19, 2024
63 of 64 checks passed
@tatiana tatiana deleted the dbt_ls_as_default_parser branch July 19, 2024 09:54
@pankajkoti pankajkoti mentioned this pull request Jul 31, 2024
pankajkoti added a commit that referenced this pull request Aug 20, 2024
New Features

* Add support for loading manifest from cloud stores using Airflow
Object Storage by @pankajkoti in #1109
* Cache ``package-lock.yml`` file by @pankajastro in #1086
* Support persisting the ``LoadMode.VIRTUALENV`` directory by @tatiana
in #1079
* Add support to store and fetch ``dbt ls`` cache in remote stores by
@pankajkoti in #1147
* Add default source nodes rendering by @arojasb3 in #1107
* Add Teradata ``ProfileMapping`` by @sc250072 in #1077

Enhancements

* Add ``DatabricksOauthProfileMapping`` profile by @CorsettiS in #1091
* Use ``dbt ls`` as the default parser when ``profile_config`` is
provided by @pankajastro in #1101
* Add task owner to dbt operators by @wornjs in #1082
* Extend Cosmos custom selector to support + when using paths and tags
by @mvictoria in #1150
* Simplify logging by @dwreeves in #1108

Bug fixes

* Fix Teradata ``ProfileMapping`` target invalid issue by @sc250072 in
#1088
* Fix empty tag in case of custom parser by @pankajastro in #1100
* Fix ``dbt deps`` of ``LoadMode.DBT_LS`` should use
``ProjectConfig.dbt_vars`` by @tatiana in #1114
* Fix import handling by lazy loading hooks introduced in PR #1109 by
@dwreeves in #1132
* Fix Airflow 2.10 regression and add Airflow 2.10 in test matrix by
@pankajastro in #1162

Docs

* Fix typo in azure-container-instance docs by @pankajastro in #1106
* Use Airflow trademark as it has been registered by @pankajastro in
#1105

Others

* Run some example DAGs in Kubernetes execution mode in CI by
@pankajastro in #1127
* Install requirements.txt by default during dev env spin up by
@@CorsettiS in #1099
* Remove ``DbtGraph.current_version`` dead code by @tatiana in #1111
* Disable test for Airflow-2.5 and Python-3.11 combination in CI by
@pankajastro in #1124
* Pre-commit hook updates in #1074, #1113, #1125, #1144, #1154,  #1167

---------

Co-authored-by: Pankaj Koti <[email protected]>
Co-authored-by: Pankaj Singh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:parsing Related to parsing DAG/DBT improvement, issues, or fixes lgtm This PR has been approved by a maintainer parsing:dbt_ls Issues, questions, or features related to dbt_ls parsing size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants