Page MenuHomePhabricator

[retry] initial in_process implementation
ClosedPublic

Authored by alangenfeld on Feb 18 2020, 7:24 PM.

Details

Summary

basic implementation of retries

  • throw a RequestRetry to get retried
  • step sequence is STARTED, UP_FOR_RETRYxN, RESTARTEDxN, SUCCESS/FAILURE
  • currently handled solely in in_process engine, diff above in stack allows handling in parent "orchestrator"
Test Plan

added test case

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alangenfeld created this revision.Feb 18 2020, 7:24 PM

step_restart event

alangenfeld retitled this revision from retry prototype V0 to [WIP] retry prototype.Feb 21 2020, 10:15 PM
alangenfeld edited the test plan for this revision. (Show Details)

with limits

alangenfeld retitled this revision from [WIP] retry prototype to [RFC] retry prototype.Feb 24 2020, 11:52 PM
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld added reviewers: prha, max, nate, schrockn.
alangenfeld added inline comments.
python_modules/dagster/dagster/core/definitions/events.py
329

better name?

python_modules/dagster/dagster/core/events/__init__.py
36

better name?

schrockn requested changes to this revision.Feb 25 2020, 1:28 AM

direction looks pretty good. req'ing to discuss naming!

python_modules/dagster/dagster/core/definitions/events.py
329

InstigateRetry
InitiateRetry
ExecuteRetry
TriggerRetry

I would avoid the use of the word "Request" since it might be confused with "Request" as a noun

python_modules/dagster/dagster/core/events/__init__.py
36

STEP_SET_TO_RETRY or something akin to that

37

STEP_RETRYING?

This revision now requires changes to proceed.Feb 25 2020, 1:28 AM
nate added inline comments.Feb 25 2020, 3:28 PM
python_modules/dagster/dagster/core/events/__init__.py
36

STEP_UP_FOR_RETRY?

schrockn added inline comments.Feb 25 2020, 4:55 PM
python_modules/dagster/dagster/core/events/__init__.py
36

๐Ÿ‘๐Ÿป

alangenfeld edited the summary of this revision. (Show Details)

renames & better tests

RetryRequested

schrockn requested changes to this revision.Feb 25 2020, 11:59 PM
schrockn added inline comments.
python_modules/dagster/dagster/core/engine/engine_inprocess.py
563

CAPS?

python_modules/dagster/dagster/core/errors.py
162 โ†—(On Diff #9994)

this seems like too "low level" for this

python_modules/dagster/dagster_tests/core_tests/execution_tests/test_retries.py
64โ€“92

might want to think of structuring these in a way so that other executors can invoke these as an acceptance test. but don't need to do that here

This revision now requires changes to proceed.Feb 25 2020, 11:59 PM

rebase & fix casing

alangenfeld added inline comments.Feb 27 2020, 9:40 PM
python_modules/dagster/dagster/core/errors.py
162 โ†—(On Diff #9994)

ya this doesn't feel right but I wasnt happy with the marker class version I tried before either. Thoughts on what a better way to do this would be?

schrockn added inline comments.Feb 28 2020, 10:11 PM
python_modules/dagster/dagster/core/engine/engine_multiprocess.py
171 โ†—(On Diff #10105)

rm?

schrockn requested changes to this revision.Feb 28 2020, 10:19 PM
schrockn added inline comments.
python_modules/dagster/dagster/core/errors.py
162 โ†—(On Diff #10105)

req'ing changes based on in-person conversation

general comments:

  • Failure and RetryRequested should probably be uniformly treated in some dimensions (they are both control flow exceptions thrown by user)
  • If this is the place, allowed control flow exceptions should be a parameter to the error boundary because it is used in many contexts (e.g. type checks, hydration configs, etc)
This revision now requires changes to proceed.Feb 28 2020, 10:19 PM
alangenfeld updated this revision to Diff 10183.Mar 2 2020, 9:58 PM

rebase on top of D2174

alangenfeld retitled this revision from [RFC] retry prototype to [retry] initial in_process implementation.Mar 2 2020, 9:59 PM
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld edited the summary of this revision. (Show Details)

rebase

schrockn accepted this revision.Mar 2 2020, 11:47 PM

looks great. excited to get this going

python_modules/dagster/dagster/core/engine/engine_inprocess.py
249

assuming you will fix numbering on rebase

708

cool

python_modules/dagster/dagster/core/errors.py
167 โ†—(On Diff #10190)

yay

This revision is now accepted and ready to land.Mar 2 2020, 11:47 PM
This revision was automatically updated to reflect the committed changes.