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

[Fix] raise execution errors for runnable tasks #8237

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

MichelleArk
Copy link
Contributor

@MichelleArk MichelleArk commented Jul 27, 2023

resolves #8166
docs dbt-labs/docs.getdbt.com/# N/A

Problem

Root cause is this return statement within a finally clause: https://github.com/dbt-labs/dbt-core/blob/main/core/dbt/task/runnable.py#L386

From: https://docs.python.org/3/tutorial/errors.html#defining-clean-up-actions

If the finally clause executes a break, continue or return statement, exceptions are not re-raised.

The initial motivation for this change was to ensure that the current node as well as any other nodes involved in the command had their run result metadata returned for writing to run_results.json. https://github.com/dbt-labs/dbt-core/pull/8066/files#r1260138324

Solution

  • move the return statement out of the finally block, this is typically discouraged for reasons such as this issue.
  • return node_results on fail-fast to ensure the initial fail-fast issue remains resolved (test coverage is here as well under tests/functional/fail_fast)

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

🎩

❯ dbt show --inline 'slect asdlkjfsld;j'
16:28:51  Running with dbt=1.7.0-a1
16:28:51  target not specified in profile 'postgres', using 'default'
16:28:51  Registered adapter: postgres=1.7.0-a1
16:28:51  Found 10 models, 3 seeds, 20 tests, 0 sources, 0 exposures, 0 metrics, 353 macros, 0 groups, 0 semantic models
16:28:51  
16:28:52  Concurrency: 1 threads (target='default')
16:28:52  
16:28:52  Encountered an error:
Runtime Error
  Database Error in sql_operation inline_query (from remote system.sql)
    syntax error at or near "slect"
    LINE 2: slect asdlkjfsld;j

@cla-bot cla-bot bot added the cla:yes label Jul 27, 2023
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #8237 (72da643) into main (fe9c875) will increase coverage by 0.03%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8237      +/-   ##
==========================================
+ Coverage   86.23%   86.26%   +0.03%     
==========================================
  Files         174      174              
  Lines       25511    25510       -1     
==========================================
+ Hits        21999    22007       +8     
+ Misses       3512     3503       -9     
Files Changed Coverage Δ
core/dbt/task/runnable.py 92.92% <100.00%> (ø)

... and 2 files with indirect coverage changes

@@ -208,9 +207,8 @@ def models(self):

def test_missing_dependency(self, project):
# dbt should raise a runtime exception
res = run_dbt(["compile"], expect_pass=False)
assert len(res) == 1
assert res[0].status == RunStatus.Error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverts the changes made here: https://github.com/dbt-labs/dbt-core/pull/8066/files#diff-4f5f78d9b3b6f6eda81916de152b5c17b9d1ef14e40b7a2031c4d10f601d6474L210-L211

On closer inspection, I'm not sure why this test was modified, it is a change in behaviour for non fail-fast functionality.

@MichelleArk MichelleArk marked this pull request as ready for review July 28, 2023 16:24
@MichelleArk MichelleArk requested a review from a team as a code owner July 28, 2023 16:24
@MichelleArk MichelleArk requested a review from ChenyuLInx July 28, 2023 16:24
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.

One question regarding pool them it should be good to go

pool.join()
return self.node_results

pool.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, do we want to always do pool.close() , pool.join() before join? If yes, based on the document that finally will be executed before break, return, continue, should we consider just write it as

try:
  xxxx
  return  self.node_results
except:
  xxxx
finally:
  pool.close()
  pool.join()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That may be a very sound thing to do, but I think the safest path forward to ensure the regression (and any others that may be lingering) are fixed is to go back to the state before this change: https://github.com/dbt-labs/dbt-core/pull/8066/files#diff-f01af78b7eaa1bacc301afa75eea4c7cdbe03064b3913bc2ce3f6bf01d0bc125R383-R386, where there was no finally

@MichelleArk
Copy link
Contributor Author

Additional 🎩 for expected fail fast behaviour:

❯ dbt run --select +downstream --fail-fast --project-dir ~/src/jaffle_shop
17:16:34  Running with dbt=1.7.0-a1
17:16:34  target not specified in profile 'postgres', using 'default'
17:16:34  Registered adapter: postgres=1.7.0-a1
17:16:34  Unable to do partial parsing because of a version mismatch
17:16:34  Found 10 models, 3 seeds, 20 tests, 0 sources, 0 exposures, 0 metrics, 353 macros, 0 groups, 0 semantic models
17:16:34  
17:16:35  Concurrency: 1 threads (target='default')
17:16:35  
17:16:35  1 of 2 START sql table model test_arky.upstream ................................ [RUN]
17:16:35  1 of 2 ERROR creating sql table model test_arky.upstream ....................... [ERROR in 0.05s]
17:16:35  CANCEL query model.jaffle_shop.upstream ........................................ [CANCEL]
17:16:35  
17:16:35  Database Error in model upstream (models/fail_fast/upstream.sql)
17:16:35    division by zero
17:16:35    compiled Code at target/run/jaffle_shop/models/fail_fast/upstream.sql
17:16:35  
17:16:35  Finished running 2 table models in 0 hours 0 minutes and 0.62 seconds (0.62s).
17:16:35  
17:16:35  Completed with 2 errors and 0 warnings:
17:16:35  
17:16:35  Database Error in model upstream (models/fail_fast/upstream.sql)
17:16:35    division by zero
17:16:35    compiled Code at target/run/jaffle_shop/models/fail_fast/upstream.sql
17:16:35  
17:16:35  Skipping due to fail_fast
17:16:35  
17:16:35  Done. PASS=0 WARN=0 ERROR=1 SKIP=1 TOTAL=2

Looking at run results, the skipped nodes are there as expected :)

 "results": [
        {
            "status": "error",
            "timing": [
                {
                    "name": "compile",
                    "started_at": "2023-07-28T17:16:35.229905Z",
                    "completed_at": "2023-07-28T17:16:35.231937Z"
                },
                {
                    "name": "execute",
                    "started_at": "2023-07-28T17:16:35.232805Z",
                    "completed_at": "2023-07-28T17:16:35.275653Z"
                }
            ],
            "thread_id": "Thread-1 (worker)",
            "execution_time": 0.05124211311340332,
            "adapter_response": {},
            "message": "Database Error in model upstream (models/fail_fast/upstream.sql)\n  division by zero\n  compiled Code at target/run/jaffle_shop/models/fail_fast/upstream.sql",
            "failures": null,
            "unique_id": "model.jaffle_shop.upstream"
        },
        {
            "status": "skipped",
            "timing": [],
            "thread_id": "MainThread",
            "execution_time": 0.0,
            "adapter_response": {},
            "message": "Skipping due to fail_fast",
            "failures": null,
            "unique_id": "model.jaffle_shop.downstream"
        }
    ],

@MichelleArk MichelleArk merged commit 410506f into main Jul 28, 2023
@MichelleArk MichelleArk deleted the 8166/reraise-execution-errors branch July 28, 2023 17:18
github-actions bot pushed a commit that referenced this pull request Jul 28, 2023
@aranke aranke mentioned this pull request Jul 12, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2852] [v1.6 pre-regression] dbt show swallows database errors
2 participants