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.

Sign up for GitHub

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

Jump to bottom

Fix converting dbt projects that have ProjectConfig.dbt_project_path set as strings #601

Closed
tatiana opened this issue Oct 13, 2023 · 2 comments · Fixed by #605
Closed

Fix converting dbt projects that have ProjectConfig.dbt_project_path set as strings #601

tatiana opened this issue Oct 13, 2023 · 2 comments · Fixed by #605
Assignees
@MrBones757
Labels
bug Something isn't working good first issue Good for newcomers priority:high High priority issues are blocking or critical issues without a workaround and large impact
Milestone
1.3.0

Comments

@tatiana
Copy link
Collaborator

tatiana commented Oct 13, 2023 •
edited
Loading

Our documentation has examples of configuring a project using a project path that is a string:

from cosmos.config import ProjectConfig

config = ProjectConfig(
    dbt_project_path="/path/to/dbt/project",
    models_relative_path="models",
    seeds_relative_path="data",
    snapshots_relative_path="snapshots",
    manifest_path="/path/to/manifests",
)

https://astronomer.github.io/astronomer-cosmos/configuration/project-config.html

However, if users attempt to use this, they will face an error:

Broken DAG: [/home/airflow/Sites/data-airflow/dags/high_freq_core_dbt.py] Traceback (most recent call last):
  File "/home/airflow/python-venvs/data-pipelines/lib/python3.10/site-packages/cosmos/airflow/task_group.py", line 26, in __init__
    DbtToAirflowConverter.__init__(self, *args, **specific_kwargs(**kwargs))
  File "/home/airflow/python-venvs/data-pipelines/lib/python3.10/site-packages/cosmos/converter.py", line 109, in __init__
    dbt_root_path = project_config.dbt_project_path.parent
AttributeError: 'str' object has no attribute 'parent'

Because of:
https://github.com/astronomer/astronomer-cosmos/blob/main/cosmos/converter.py#L109-L110

This issue was originally reported in the #airflow-dbt slack channel by Sai:
https://apache-airflow.slack.com/archives/C059CC42E9W/p1697234106298499?thread_ts=1697209691.193499&cid=C059CC42E9W

Acceptance criteria
As part of the fix, we should improve the converter tests by adding unit tests similar to:
https://github.com/astronomer/astronomer-cosmos/blob/main/tests/test_converter.py#L49-L69

We should validate at least the two following use-cases:

  1. ProjectConfig.dbt_project_path as string
  2. No ProjectConfig.dbt_project_path, to have a broader test of the improvement Make the arg dbt_project_path in the ProjectConfig optional #581
@tatiana tatiana added bug Something isn't working good first issue Good for newcomers priority:high High priority issues are blocking or critical issues without a workaround and large impact labels Oct 13, 2023
@tatiana tatiana added this to the 1.3.0 milestone Oct 13, 2023
This was referenced Oct 16, 2023
Closed
Merged
@MrBones757
Copy link
Contributor

I should have some time to take a look at this tomorrow!

@tatiana
Copy link
Collaborator Author

tatiana commented Oct 16, 2023

Thanks a lot, @MrBones757 ! I assigned it to you for now - please, let us know if the circumstances change

@MrBones757 MrBones757 mentioned this issue Oct 17, 2023
Merged
2 tasks
@tatiana tatiana mentioned this issue Oct 23, 2023
Closed
2 tasks
tatiana pushed a commit that referenced this issue Oct 25, 2023
@MrBones757
Resolve errors occurring when dbt_project_path is str and partial sup…
2f8d0e2 
…port dbt_project_path=None (#605)

As part of the changes made #581, some downstream logic was missed
relating to the handling of a None and String-based project dir. This MR
attempts to remedy this issue by adding down steam support for the project
dir being None (including generation of exceptions and guarding), as
well as some property reference changes in the converter.

Closes: #601 

Co-authored-by: tabmra <[email protected]>
tatiana pushed a commit that referenced this issue Oct 25, 2023
@MrBones757 @tatiana
Resolve errors occurring when dbt_project_path is str and partial sup…
b055cdd 
…port dbt_project_path=None (#605)

As part of the changes made #581, some downstream logic was missed
relating to the handling of a None and String-based project dir. This MR
attempts to remedy this issue by adding down steam support for the project
dir being None (including generation of exceptions and guarding), as
well as some property reference changes in the converter.

Closes: #601

Co-authored-by: tabmra <[email protected]>
(cherry picked from commit 2f8d0e2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MrBones757 MrBones757

Labels
bug Something isn't working good first issue Good for newcomers priority:high High priority issues are blocking or critical issues without a workaround and large impact
Projects
None yet
Milestone
1.3.0
Development

Successfully merging a pull request may close this issue.

Resolve errors occurring when dbt_project_path is str and partial support dbt_project_path=None
2 participants
@tatiana @MrBones757