-
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
[backfill daemon run retries 3/n] retries of runs in completed backfills should not be considered part of the backfill #25900
Conversation
389f553
to
46e48b3
Compare
aabdbaa
to
039026a
Compare
46e48b3
to
eade146
Compare
039026a
to
9f7eaa6
Compare
eade146
to
7408d71
Compare
9b3b9ca
to
bc18437
Compare
python_modules/dagster/dagster_tests/daemon_tests/test_backfill.py
Outdated
Show resolved
Hide resolved
7408d71
to
bcf74ed
Compare
bc18437
to
a9538d8
Compare
bcf74ed
to
9e9139b
Compare
a9538d8
to
0c88250
Compare
9e9139b
to
cb3ce07
Compare
0c88250
to
c06ac7f
Compare
cb3ce07
to
72f301f
Compare
c06ac7f
to
55249f2
Compare
72f301f
to
b42f503
Compare
55249f2
to
11ed14f
Compare
b42f503
to
eb2dd11
Compare
11ed14f
to
111b62b
Compare
eb2dd11
to
2fa09c5
Compare
5062420
to
4944d77
Compare
2fa09c5
to
ad1c316
Compare
4944d77
to
b5db7ae
Compare
ad1c316
to
de67c1d
Compare
b5db7ae
to
d7ac069
Compare
de67c1d
to
6cb78c1
Compare
a89be73
to
f6c77ca
Compare
b135d13
to
fb46cc2
Compare
) | ||
parent_run_tags = {} | ||
if use_parent_run_tags: | ||
parent_run_tags = { |
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 applies to the old code too but maybe this should be parent_run_tags_to_include?
parent_run_tags = { | ||
key: val | ||
for key, val in parent_run.tags.items() | ||
if key not in TAGS_TO_OMIT_ON_RETRY and key not in TAGS_TO_MAYBE_OMIT_ON_RETRY |
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.
given that this is the sole place that TAGS_TO_OMIT_ON_RETRY and TAGS_TO_MAYBE_OMIT_ON_RETRY are used I think we could keep it as one list of tags vs. splitting them out into two lists? should it all just be TAGS_TO_MAYBE_OMIT_ON_RETRY?
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.
and then just not have a condition to add back the tags that are in the TAGS_TO_OMIT_ON_RETRY list? that sounds good to me
58ed06d
to
eea3924
Compare
fb46cc2
to
22ec8e7
Compare
eea3924
to
5126053
Compare
22ec8e7
to
9873da8
Compare
5126053
to
4ce8b5a
Compare
9873da8
to
56d380e
Compare
4ce8b5a
to
d6f6797
Compare
56d380e
to
2b9202d
Compare
2b9202d
to
ef43e6b
Compare
…lls should not be considered part of the backfill (#25900) ## Summary & Motivation If a run is retried after a backfill is complete, that run is given the backfill tag, but has no affect on the backfill itself. This can cause confusion. Imagine the scenario where a single asset-partition failed in a backfill. The backfill is complete and a user retries the failed asset and the retry succeeds. That retried run will show up in the list of runs for the backfill, but the status in the overview tab for partition will still be failed since the status is locked when the backfill completes. We should be more strict about when run retries are considered part of the backfill. We decided in dagster-io/internal#12460 that retries that are launched while the backfill is in progress will be part of the backfill, but that retries that are launched after the backfill is complete should not be considered part of the backfill. To make this change we need to remove the backfill tag from retried runs if the backfill is not in progress. ## How I Tested These Changes new unit tests manually launched a retry of a run that was launched by a backfill after the backfill was complete. no backfill tags were added <img width="1037" alt="Screenshot 2024-12-02 at 1 55 09 PM" src="https://github.com/user-attachments/assets/5bb8ae12-4c61-4fd4-8255-1d245ae43318"> ## Changelog Manual retries of runs launched by backfills are no longer considered part of the backfill if the backfill is complete when the retry is launched.
…lls should not be considered part of the backfill (dagster-io#25900) ## Summary & Motivation If a run is retried after a backfill is complete, that run is given the backfill tag, but has no affect on the backfill itself. This can cause confusion. Imagine the scenario where a single asset-partition failed in a backfill. The backfill is complete and a user retries the failed asset and the retry succeeds. That retried run will show up in the list of runs for the backfill, but the status in the overview tab for partition will still be failed since the status is locked when the backfill completes. We should be more strict about when run retries are considered part of the backfill. We decided in https://github.com/dagster-io/internal/discussions/12460 that retries that are launched while the backfill is in progress will be part of the backfill, but that retries that are launched after the backfill is complete should not be considered part of the backfill. To make this change we need to remove the backfill tag from retried runs if the backfill is not in progress. ## How I Tested These Changes new unit tests manually launched a retry of a run that was launched by a backfill after the backfill was complete. no backfill tags were added <img width="1037" alt="Screenshot 2024-12-02 at 1 55 09 PM" src="https://github.com/user-attachments/assets/5bb8ae12-4c61-4fd4-8255-1d245ae43318"> ## Changelog Manual retries of runs launched by backfills are no longer considered part of the backfill if the backfill is complete when the retry is launched.
Summary & Motivation
If a run is retried after a backfill is complete, that run is given the backfill tag, but has no affect on the backfill itself. This can cause confusion. Imagine the scenario where a single asset-partition failed in a backfill. The backfill is complete and a user retries the failed asset and the retry succeeds. That retried run will show up in the list of runs for the backfill, but the status in the overview tab for partition will still be failed since the status is locked when the backfill completes.
We should be more strict about when run retries are considered part of the backfill. We decided in https://github.com/dagster-io/internal/discussions/12460 that retries that are launched while the backfill is in progress will be part of the backfill, but that retries that are launched after the backfill is complete should not be considered part of the backfill.
To make this change we need to remove the backfill tag from retried runs if the backfill is not in progress.
How I Tested These Changes
new unit tests
manually launched a retry of a run that was launched by a backfill after the backfill was complete. no backfill tags were added
Changelog
Manual retries of runs launched by backfills are no longer considered part of the backfill if the backfill is complete when the retry is launched.