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

Fix task run count when a task is configured with retry_delay_seconds #15424

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Fix task run count when a task is configured with retry_delay_seconds #15424

merged 2 commits into from
Sep 19, 2024

Conversation

desertaxle
Copy link
Member

@desertaxle desertaxle commented Sep 18, 2024 •
edited
Loading

This PR moves the increment of a task run's run_count closer to the user code execution to ensure it's correctly counted.

Closes #15422

@github-actions github-actions bot added the bug Something isn't working label Sep 18, 2024
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Sep 18, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #15424 will not alter performance

Comparing fix-run-count-on-task-retries (4e29b9b) with main (5cc3b66)

Summary

✅ 3 untouched benchmarks

@desertaxle desertaxle marked this pull request as ready for review September 18, 2024 20:10
zzstoatzz
zzstoatzz approved these changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

lgtm!

@desertaxle desertaxle merged commit a09aacc into main Sep 19, 2024
33 checks passed
@desertaxle desertaxle deleted the fix-run-count-on-task-retries branch September 19, 2024 13:23
cicdw
cicdw reviewed Sep 19, 2024
View reviewed changes
Copy link
Member

@cicdw cicdw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@desertaxle apparently my review submission didn't go through last night - want to make sure you see this comment in case it's important

src/prefect/task_engine.py
@@ -364,7 +364,6 @@ def begin_run(self):
new_state = Running()

self.task_run.start_time = new_state.timestamp
self.task_run.run_count += 1
Copy link
Member

Choose a reason for hiding this comment

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

I might be over thinking it but isn't this line still relevant? I guess it depends on whether "run count" is how many runs have entered Running vs. how many runs have exited Running -- I think it's always been entered Running which means this should still increment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, I was interpreting it as the number of times the user code has been run. I'll submit a PR to revert back to the previous behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cicdw Update is in #15436

desertaxle added a commit that referenced this pull request Sep 19, 2024
@desertaxle
Revert "Fix task run count when a task is configured with `retry_dela…
857727d 
…y_seconds` (#15424)"

This reverts commit a09aacc.
@desertaxle desertaxle mentioned this pull request Sep 19, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cicdw cicdw cicdw left review comments

@zzstoatzz zzstoatzz zzstoatzz approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Retried task runs do not accumulate their executions to the task run's run_count
3 participants
@desertaxle @cicdw @zzstoatzz