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

Implement no_retries_on_test_failure on DbtTestLocalOperator #1563

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

okayhooni
Copy link

@okayhooni okayhooni commented Feb 25, 2025

Description

Added no_retries_on_test_failure parameter to DbtTestLocalOperator with a default value of True.
This parameter raises AirflowFailException, ensuring that failed DBT tests are not retried unnecessarily, even if a common retries value of 1 or more is set in DbtDag/DbtTaskGroup.

Unlike the on_warning_callback parameter, it does not seem necessary to propagate no_retries_on_test_failure through the converter and airflow/graph modules. Instead, similar to the callback parameter in AbstractDbtLocalBase, it can be passed via operator_args when needed.

Related Issue(s)

closes #1537

Breaking Change?

Nope.

Checklist

  • [] I have made corresponding changes to the documentation (if required) -> Not needed in this case.
  • I have added tests that prove my fix is effective or that my feature works

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Feb 25, 2025
Copy link

netlify bot commented Feb 25, 2025

Deploy Preview for sunny-pastelito-5ecb04 ready!

Name Link
🔨 Latest commit 47ed26c
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/67c89cd2eca63800083915ce
😎 Deploy Preview https://deploy-preview-1563--sunny-pastelito-5ecb04.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dosubot dosubot bot added dbt:test Primarily related to dbt test command or functionality execution:local Related to Local execution environment labels Feb 25, 2025
@okayhooni okayhooni changed the title implement no_retries_on_test_failure on DbtTestLocalOperator Implement no_retries_on_test_failure on DbtTestLocalOperator Feb 25, 2025
Copy link
Contributor

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

LGTM

@pankajastro pankajastro requested a review from tatiana February 26, 2025 08:32
@tatiana tatiana added this to the Cosmos 1.10.0 milestone Feb 27, 2025
@tatiana tatiana changed the title Implement no_retries_on_test_failure on DbtTestLocalOperator Implement no_retries_on_test_failure on DbtTestLocalOperator Feb 28, 2025
@pankajastro pankajastro force-pushed the feat/no_retries_on_test_failure branch from 440201c to 3307086 Compare March 3, 2025 08:21
@pankajastro pankajastro force-pushed the feat/no_retries_on_test_failure branch from 3307086 to 47ed26c Compare March 5, 2025 18:49
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.

Hi @okayhooni, thanks a lot for the report and your contribution!

Apologies for the delayed review—I was out last week and have been catching up this week.

After discussing this internally with @tatiana, we feel it would be better to unify this configuration with our existing interface for similar scenarios. For reference, in PR #1492, we added support for overriding the profile per dbt node. Similarly, we think it would be more consistent to allow this configuration to be overridden at the test level for specific nodes and at the dbt_project.yml level for global application.

For example, similar to this configuration, we could support something like:

tests:
  jaffle_shop:
      +meta:
        cosmos:
          retries: 0

Another point to consider is that test failures may also occur due to connectivity issues, such as being unable to connect to the datasource. In such cases, it might make sense to retry the task. If we can determine the actual reason for the failure—whether it was due to a failed dbt check or a connectivity issue—then we could apply this configuration accordingly (perhaps by analyzing the logs).

Additionally, we might not want to disable retries by default, as it maybe considered breaking change for existing versions. Right now, tasks may retry for various reasons beyond dbt test failures, such as network interruptions.

Looking forward to your thoughts on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dbt:test Primarily related to dbt test command or functionality execution:local Related to Local execution environment size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] raise AirflowFailException in order to avoid retrying dbt test tasks on failure
4 participants