-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[daggy-u][dbt] Update dbt course to use DbtProject #23098
[daggy-u][dbt] Update dbt course to use DbtProject #23098
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @maximearmstrong and the rest of your teammates on Graphite |
Deploy preview for dagster-university ready! ✅ Preview Built with commit 66dd24f. |
68dbc27
to
07b7ca3
Compare
This is looking great. @maximearmstrong we will also need to update the "completed" |
|
||
```python | ||
dbt_project = DbtProject( | ||
project_dir=Path(__file__).joinpath("..", "..", "..", "analytics").resolve(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we did this in the previous lesson as well, but I'm not a fan of repeating ..
relative paths.
Would it be more intuitive if we used dagster home, or DagsterInstance.get().root_directory()
? This can merge before this change, but wanted to bring it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point, but I think we should avoid this for two main reasons:
- it is common to see a dbt project outside the Dagster project directory - we would need to resolve the path using
DagsterInstance.get().root_directory()
and more..
to indicate the dbt project location. - the current scaffold template uses
..
relative paths. If we want to change that in Dagster University, we should start by the scaffold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! A few things, but overall it's looking good.
Let me know when this is ready to go and I'll update the quiz and lesson title in the LMS.
docs/dagster-university/pages/dagster-dbt/lesson-3/3-representing-the-dbt-project-in-dagster.md
Outdated
Show resolved
Hide resolved
|
||
As you’ll frequently point your Dagster code to the `target/manifest.json` file and your dbt project in this course, it’ll be helpful to keep a reusable representation of the dbt project. This can be easily done using the `DbtProject` class. | ||
|
||
Create a new file in the `assets` directory called `dbt.py`. Open that file and add the following import at the top: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a new file in the `assets` directory called `dbt.py`. Open that file and add the following import at the top: | |
In the `assets` directory, create a new `dbt.py` file and add the following imports: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I see now:
In the
dagster_university
directory, create a newproject.py
file and add the following imports:
But later on in the lesson, dbt.py
is still used (ex: line 31).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch! Fixed in 66dd24f
docs/dagster-university/pages/dagster-dbt/lesson-3/3-representing-the-dbt-project-in-dagster.md
Outdated
Show resolved
Hide resolved
...dagster-university/pages/dagster-dbt/lesson-3/5-loading-dbt-models-into-dagster-as-assets.md
Outdated
Show resolved
Hide resolved
...dagster-university/pages/dagster-dbt/lesson-3/5-loading-dbt-models-into-dagster-as-assets.md
Outdated
Show resolved
Hide resolved
docs/dagster-university/pages/dagster-dbt/lesson-4/2-speeding-up-the-development-cycle.md
Outdated
Show resolved
Hide resolved
docs/dagster-university/pages/dagster-dbt/lesson-4/2-speeding-up-the-development-cycle.md
Outdated
Show resolved
Hide resolved
docs/dagster-university/pages/dagster-dbt/lesson-7/4-creating-the-manifest-during-deployment.md
Outdated
Show resolved
Hide resolved
docs/dagster-university/pages/dagster-dbt/lesson-7/5-preparing-for-a-successful-run.md
Outdated
Show resolved
Hide resolved
docs/dagster-university/pages/dagster-dbt/lesson-7/5-preparing-for-a-successful-run.md
Outdated
Show resolved
Hide resolved
bf706a6
to
54fd52f
Compare
@erinkcochran87 All comments were addressed in 54fd52f
Perfect. We need to update the final example then we are ready to go. Will let you know! |
54fd52f
to
305f9ff
Compare
After fully testing the updated final example here, I've updated this PR in commit 305f9ff. tl;dr: the cc @cmpadden @erinkcochran87 if you want to review the changes in this commit before we merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back to you!
## Summary & Motivation This PR updates the dbt course in Dagster University to use DbtProject. To do outside this PR: - update the knowledge check/quiz in Lesson 4. The `DAGSTER_DBT_PARSE_PROJECT_ON_LOAD` env var is no longer used. ## How I Tested These Changes
## Summary & Motivation This PR updates the dbt course in Dagster University to use DbtProject. To do outside this PR: - update the knowledge check/quiz in Lesson 4. The `DAGSTER_DBT_PARSE_PROJECT_ON_LOAD` env var is no longer used. ## How I Tested These Changes (cherry picked from commit b6156a4)
Summary & Motivation
This PR updates the dbt course in Dagster University to use DbtProject.
To do outside this PR:
DAGSTER_DBT_PARSE_PROJECT_ON_LOAD
env var is no longer used.How I Tested These Changes