Prefect: Explore transforming State into a class instead of a String + Data attribute

[jlowin]

Instead of the current State class that has a string state and Any data attribute, as well as a bunch of is_finished() methods that check the state string against a list of valid strings, consider building a hierarchy of State classes:

# Base class

class State:
    # class attribute for example; set in __init__
    data = None

# -------------------------------------------
# Finished classes - Success, Failed, Skipped
class Finished(State):
    pass

class Success(Finished):
    pass

class Failed(Finished):
    # class attribute for example; set in __init__
    message = None

class Skipped(Finished):
    pass

# -------------------------------------------
# Pending classes - Retry and Scheduled (and Pending itself)
class Pending(State):
    pass

class Retry(Pending):
    # class attribute for example; set in __init__
    retry_time = None

class Scheduled(Pending):
    # class attribute for example; set in __init__
    scheduled_time = None

Then checking and working with states is easier than the current system of checking the string attribute and hoping the data attribute matches an expected schema:

s = Success(100)
f = Failed('Division by zero error')

assert isinstance(s, Finished)
assert isinstance(f, Finished)

r = Retry(datetime(2018, 12, 31))
assert isinstance(r, Pending)
assert isinstance(r.retry_time datetime)

[jlowin]

The more I think about this the more I like it, because it would make state processing no longer dependent on matching strings against other strings or possibly lists of strings, but rather based on a hierarchical system of inheritance.

For example, as long as something subclasses Finished, we know our system will handle it. So (hypothetically) we could introduce a new Finished state - say, UserCanceled or ManualShutdown or Timeout - and not have to build new logic for it. isinstance(state, Finished) would be enough. We could still use class methods (state.is_finished()) as we do currently.

[jlowin]

@cicdw Now that you're taggable, just adding you to this idea :)

[cicdw]

Overall I really like this idea; my only hesitation is that complex inheritance hierarchies can be confusing to read and maintain, but in this case it feels like it could be worthwhile.

[cicdw]

We could also consider mixins which would allow for (possibly multiple) state-like properties to be inherited from, and that might make reasoning about states slightly less rigid and more trait-driven.

[jlowin]

Good points. My thoughts to the first -- I don't think we would encourage (nor even expect) rampant custom states. I think it would be helpful even with our existing set of states (and one or two more we've discussed but not implemented yet, like #44). But even if it got out of hand... I still think it would be preferable to the equivalent number of states using pure string matching!

And to your second point -- it's an interesting idea. After considering my answer to the previous point, I think we will end up with a well-defined set of hierarchical states, so I'm leaning towards a class hierarchy over mixins. For example, we have three major "types" of mutually exclusive states: Pending, Running, and Finished; the sub-states in each of those groups are themselves pretty well defined and don't have too much overlap (otherwise they would probably be combined).

Just my $0.02 after reading your thoughts -- but let's continue to kick this around