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

Handle internal exceptions in postflight #7242

Merged
merged 4 commits into from
Apr 10, 2023
Merged

Handle internal exceptions in postflight #7242

merged 4 commits into from
Apr 10, 2023

Conversation

stu-k
Copy link
Contributor

@stu-k stu-k commented Mar 29, 2023 •
edited
Loading

resolves #7118

Description

  • Handle all dbt thrown exceptions in the postflight decorator
  • Add DbtRunnerResult dataclass with the intent of dbtRunner.invoke not throwing exceptions
  • Handle all exceptions thrown from cli.invoke within dbtRunner.invoke
  • Add exception handling to dbt/cli/README.md
  • Change run_dbt method from tests/util.py to raise any exceptions returned from dbtRunner.invoke
  • Move cli exceptions into dbt/cli/exceptions.py

Checklist

@cla-bot cla-bot bot added the cla:yes label Mar 29, 2023
jtcohen6
jtcohen6 reviewed Mar 29, 2023
View reviewed changes
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Thanks for throwing up the draft PR so early @stu-k!

core/dbt/cli/requires.py Show resolved Hide resolved
@stu-k stu-k force-pushed the CT-2267/fail-fast-stacktrace branch 6 times, most recently from 79f30aa to d0567bc Compare April 5, 2023 15:45
@stu-k stu-k marked this pull request as ready for review April 5, 2023 16:06
@stu-k stu-k requested review from a team as code owners April 5, 2023 16:06
@stu-k stu-k requested review from McKnight-42, nathaniel-may, a team, ChenyuLInx, aranke and iknox-fa and removed request for a team, nathaniel-may and McKnight-42 April 5, 2023 16:06
@stu-k stu-k force-pushed the CT-2267/fail-fast-stacktrace branch from d0567bc to 7bf96bd Compare April 5, 2023 18:30
@stu-k stu-k requested a review from jtcohen6 April 5, 2023 18:31
@stu-k stu-k force-pushed the CT-2267/fail-fast-stacktrace branch from 7bf96bd to 5d256d0 Compare April 5, 2023 18:39
ChenyuLInx
ChenyuLInx reviewed Apr 5, 2023
View reviewed changes
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

The changes look good and through. We should figure out a bit about the impact of the interface update before merge.

core/dbt/cli/example.py
@@ -17,4 +17,4 @@
# initialize the runner with pre-loaded profile and project, you can also pass in a preloaded manifest
dbt = dbtRunner(profile=profile, project=project)
# run the command, this will use the pre-loaded profile and project instead of loading
res, success = dbt.invoke(cli_args)
res = dbt.invoke(cli_args)
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually changed our previous interface a bit, two other places someone should update once this is merged

I probably will do the first one and CC @racheldaniel for the second one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

I think we should maintain the previous interface if we can.

Copy link
Contributor

@jtcohen6 jtcohen6 Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As discussed during quick huddle last week - I'm okay with changing the interface here. It's only being used in a few places, only for v1.5+, and we can handle those changes. Unless we think the previous interface (returning & unpacking a tuple) is strictly better, more important that we get this right going forward.

We may just need a bit of conditional logic in those call sites, since this change will roll out in HEAD before we include it in a new v1.5 prerelease.

Copy link
Member

Choose a reason for hiding this comment

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

My bad, I missed that we're returning a data class now.
Aligned with the approach outlined here.

aranke
aranke reviewed Apr 5, 2023
View reviewed changes
core/dbt/cli/README.md Show resolved Hide resolved
core/dbt/cli/example.py
@@ -17,4 +17,4 @@
# initialize the runner with pre-loaded profile and project, you can also pass in a preloaded manifest
dbt = dbtRunner(profile=profile, project=project)
# run the command, this will use the pre-loaded profile and project instead of loading
res, success = dbt.invoke(cli_args)
res = dbt.invoke(cli_args)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should maintain the previous interface if we can.

core/dbt/cli/exceptions.py Show resolved Hide resolved
core/dbt/cli/exceptions.py Show resolved Hide resolved
core/dbt/cli/flags.py Show resolved Hide resolved
core/dbt/cli/main.py Outdated Show resolved Hide resolved
core/dbt/cli/main.py Outdated Show resolved Hide resolved
core/dbt/cli/requires.py Show resolved Hide resolved
core/dbt/cli/requires.py Outdated Show resolved Hide resolved
tests/unit/test_dbt_runner.py Outdated Show resolved Hide resolved
@stu-k
Addressing PR comments
aa186f4 
* Add code example to cli/README.md

* Remove optional from DbtRunnerResult's result attribute to add None
  for better comments on which task returns what type

* Use exc.exit_code for an int comparison when handling a click Exit
  exception

* Split apart exception cases when handling in postflight

* Remove unnecessary test file
aranke
aranke reviewed Apr 5, 2023
View reviewed changes
Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but I'll wait for Chenyu to provide the final approval.

iknox-fa
iknox-fa approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@iknox-fa iknox-fa left a comment

Choose a reason for hiding this comment

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

LGTM!

jtcohen6
jtcohen6 reviewed Apr 7, 2023
View reviewed changes
core/dbt/cli/main.py Outdated Show resolved Hide resolved
jtcohen6
jtcohen6 reviewed Apr 7, 2023
View reviewed changes
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This PR is awesome!! Really appreciative of the thorough README :)

@jtcohen6 jtcohen6 mentioned this pull request Apr 10, 2023
Closed
@stu-k stu-k merged commit 2afb4cc into main Apr 10, 2023
@stu-k stu-k deleted the CT-2267/fail-fast-stacktrace branch April 10, 2023 22:25
This was referenced Apr 11, 2023
Merged
Merged
emmyoop pushed a commit that referenced this pull request Apr 11, 2023
@stu-k @emmyoop
Handle internal exceptions in postflight (#7242)
8cf4fab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iknox-fa iknox-fa iknox-fa approved these changes

@aranke aranke aranke left review comments

@ChenyuLInx ChenyuLInx ChenyuLInx left review comments

@jtcohen6 jtcohen6 jtcohen6 left review comments

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-2267] [Bug] Unexpected stacktrace for dbt --fail-fast run
5 participants
@stu-k @jtcohen6 @ChenyuLInx @aranke @iknox-fa