Page MenuHomeElementl

expose retry attempts on SoildExecutionContext
ClosedPublic

Authored by alangenfeld on Jun 4 2021, 2:53 PM.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

python_modules/dagster/dagster/core/execution/context/compute.py
232–237

what exactly should i expose here?

retry_number
attempt_number
previous_attempts
previous_attempt_count

this is the user facing API so last chance to get it right

python_modules/dagster/dagster/core/execution/context/compute.py
232–237

how about retry_count?

imo at minimum the word "retry" should be in the name

see how retry_attempt_number feels

owen requested changes to this revision.Jun 7 2021, 4:32 PM
owen added inline comments.
python_modules/dagster/dagster/core/execution/context/compute.py
232–237

If we're being super-pedantic, I don't think the word "attempt" should be in the name, because it makes the property name longer while potentially introducing extra confusion.

Every time you run the solid (including the first time, which is not a retry) can be considered an "attempt", so if it was just "attempt_number", then I'd expect that to be 1-indexed instead of 0-indexed. On the other hand, a "retry attempt" is not really a thing (you don't attempt to retry, because the retry will happen regardless of if the solid succeeds or fails, you're attempting to run the solid successfully).

I also agree w/ yuhan that "retry" should be in the name, so I think my favorite option is retry_number, although retry_count seems ok to me as well.

This revision now requires changes to proceed.Jun 7 2021, 4:32 PM
This revision is now accepted and ready to land.Jun 8 2021, 4:50 PM