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

Add executed_with_root_target condition to handle partial runs / failures #25253

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

Resolves: #24389

To put it simply, this ensures the any_deps_updated condition does not "count" an update from a parent if the run that materialized that parent also planned to materialize the child.

This guards against a variety of situations where a run can be launched with the intention of updating many assets, but fail to do so for whatever reason (intentional skip, failure event). In those cases, the assumption is that the child should NOT be attempted again (even though the parent did successfully materialize).

This is a fairly niche implementation detail that users should not have to think about when crafting their own policies.

How I Tested These Changes

I added a parameterized test that failed before this change (for the skip and fail cases) and passes now.

Changelog

Fixed an issue with AutomationCondition.eager() that could cause it to attempt to launch a second attempt of an asset in cases where it was skipped or failed during a run where one of its parents successfully materialized.

Copy link
Contributor Author

OwenKephart commented Oct 14, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @OwenKephart and the rest of your teammates on Graphite Graphite

@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from 2e98c95 to dce5c4f Compare October 14, 2024 17:04
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from b9dbe0c to 4e4b437 Compare October 14, 2024 17:04
@OwenKephart OwenKephart requested a review from schrockn October 14, 2024 22:57
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from dce5c4f to 0059e10 Compare October 14, 2024 23:02
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from 4e4b437 to 2850300 Compare October 14, 2024 23:02
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from 0059e10 to 87bf450 Compare October 14, 2024 23:10
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from 2850300 to 840bb0e Compare October 14, 2024 23:10
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from 87bf450 to 2025cb6 Compare October 15, 2024 17:32
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from 840bb0e to 230df04 Compare October 15, 2024 17:33
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from 2025cb6 to 8e94ee3 Compare October 15, 2024 17:55
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from 230df04 to 0ddac7d Compare October 15, 2024 17:55
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from 8e94ee3 to f84471d Compare October 15, 2024 18:56
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from dfc460c to 528031d Compare October 15, 2024 21:09
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Are there performance implications of this given that it is in the eager condition and ends up invoking expensively_compute_partition_keys? Are we saved by latest_time_window here?

@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from b029615 to caca0a8 Compare October 16, 2024 23:36
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from 528031d to 5aa93dd Compare October 16, 2024 23:36
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from caca0a8 to da8082f Compare October 16, 2024 23:46
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from 5aa93dd to 7ad38c3 Compare October 16, 2024 23:46
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from da8082f to f8c18ab Compare October 17, 2024 00:20
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from 7ad38c3 to e802a7c Compare October 17, 2024 00:20
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from f8c18ab to e3fc146 Compare October 17, 2024 16:39
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from e802a7c to fbf6ea6 Compare October 17, 2024 16:39
Copy link
Contributor Author

We're actually saved by the fact that this is and'd together with the newly_updated condition. So we only check this on the exact tick that a parent updates, which pretty severely caps the number of total partitions we might be dealing with here

@OwenKephart OwenKephart requested a review from schrockn October 17, 2024 19:49
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from e3fc146 to 54b3fd3 Compare October 21, 2024 23:04
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from fbf6ea6 to 1620391 Compare October 21, 2024 23:05
@OwenKephart OwenKephart force-pushed the 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted branch from 54b3fd3 to a4fda1b Compare October 22, 2024 16:48
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from 1620391 to b54284c Compare October 22, 2024 16:48
Copy link
Contributor Author

OwenKephart commented Oct 22, 2024

Merge activity

  • Oct 22, 12:50 PM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Oct 22, 1:01 PM EDT: Graphite rebased this pull request as part of a merge.
  • Oct 22, 1:02 PM EDT: A user merged this pull request with Graphite.

@OwenKephart OwenKephart changed the base branch from 10-10-add_error_for_user_code_sensors_with_too_many_entities_targeted to graphite-base/25253 October 22, 2024 16:56
@OwenKephart OwenKephart changed the base branch from graphite-base/25253 to master October 22, 2024 16:58
@OwenKephart OwenKephart force-pushed the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch from b54284c to 0ed2237 Compare October 22, 2024 17:00
@OwenKephart OwenKephart merged commit 6a31810 into master Oct 22, 2024
1 check was pending
@OwenKephart OwenKephart deleted the 10-14-add_executed_with_root_target_condition_to_handle_partial_runs___failures branch October 22, 2024 17:02
Grzyblon pushed a commit to Grzyblon/dagster that referenced this pull request Oct 26, 2024
…ilures (dagster-io#25253)

## Summary & Motivation

Resolves: dagster-io#24389

To put it simply, this ensures the `any_deps_updated` condition does not "count" an update from a parent if the run that materialized that parent also planned to materialize the child.

This guards against a variety of situations where a run can be launched with the intention of updating many assets, but fail to do so for whatever reason (intentional skip, failure event). In those cases, the assumption is that the child should NOT be attempted again (even though the parent did successfully materialize).

This is a fairly niche implementation detail that users should not have to think about when crafting their own policies.

## How I Tested These Changes

I added a parameterized test that failed before this change (for the skip and fail cases) and passes now.

## Changelog

Fixed an issue with `AutomationCondition.eager()` that could cause it to attempt to launch a second attempt of an asset in cases where it was skipped or failed during a run where one of its parents successfully materialized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automation Condition can't handle assets with optional outputs correctly
2 participants