-
Notifications
You must be signed in to change notification settings - Fork 68
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
Workflow init decorator #645
Changes from 11 commits
b677bda
0b33593
a68911e
3573c0b
5fda934
4f7a0e0
e9e2af4
a739d50
16395b4
84958a6
1f81d84
d8b5dff
0557b28
0c10888
95f0630
d87d7b1
fcf2d7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,10 +143,38 @@ def decorator(cls: ClassType) -> ClassType: | |
return decorator | ||
|
||
|
||
def init( | ||
init_fn: CallableType, | ||
) -> CallableType: | ||
"""Decorator for the workflow init method. | ||
|
||
This may be used on the __init__ method of the workflow class to specify | ||
that it accepts the same workflow input arguments as the ``@workflow.run`` | ||
method. It may not be used on any other method. | ||
|
||
If used, the workflow will be instantiated as | ||
``MyWorkflow(**workflow_input_args)``. If not used, the workflow will be | ||
instantiated as ``MyWorkflow()``. | ||
|
||
Note that the ``@workflow.run`` method is always called as | ||
``my_workflow.my_run_method(**workflow_input_args)``. Therefore, if you use | ||
the ``@workflow.init`` decorator, the parameter list of your __init__ and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think we should require/enforce the params are identical if this decorator is present and the run method is present There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. Yes, I was tentatively thinking of not having a contract without any static (decorator-time) constraints:
But I can't think of a clear counter-example where the stricter contract would make desirable code invalid, whereas enforcing identical params at decorator-eval time has the benefit of catching incorrect code at import time, and also that it can be relaxed in the future if we want to (whereas the looser contract cannot easily be made stricter). I do think we want to allow names to differ, e.g. for this scenario where an underscore (perhaps required by a linter or styleguide) is used to mark an unused parameter. @workflow.defn
class MyWorkflow:
@workflow.init
def __init__(self, arg: str) -> None:
self.value = arg
@workflow.run
async def run(self, _arg: str) -> str:
return self.value |
||
``@workflow.run`` methods will usually be identical. | ||
|
||
Args: | ||
init_fn: The __init__function to decorate. | ||
""" | ||
if init_fn.__name__ != "__init__": | ||
raise ValueError("@workflow.init may only be used on the __init__ method") | ||
|
||
setattr(init_fn, "__temporal_workflow_init", True) | ||
return init_fn | ||
|
||
|
||
def run(fn: CallableAsyncType) -> CallableAsyncType: | ||
"""Decorator for the workflow run method. | ||
|
||
This must be set on one and only one async method defined on the same class | ||
This must be used on one and only one async method defined on the same class | ||
as ``@workflow.defn``. This can be defined on a base class method but must | ||
then be explicitly overridden and defined on the workflow class. | ||
|
||
|
@@ -238,7 +266,7 @@ def signal( | |
): | ||
"""Decorator for a workflow signal method. | ||
|
||
This is set on any async or non-async method that you wish to be called upon | ||
This is used on any async or non-async method that you wish to be called upon | ||
receiving a signal. If a function overrides one with this decorator, it too | ||
must be decorated. | ||
|
||
|
@@ -309,7 +337,7 @@ def query( | |
): | ||
"""Decorator for a workflow query method. | ||
|
||
This is set on any non-async method that expects to handle a query. If a | ||
This is used on any non-async method that expects to handle a query. If a | ||
function overrides one with this decorator, it too must be decorated. | ||
|
||
Query methods can only have positional parameters. Best practice for | ||
|
@@ -983,7 +1011,7 @@ def update( | |
): | ||
"""Decorator for a workflow update handler method. | ||
|
||
This is set on any async or non-async method that you wish to be called upon | ||
This is used on any async or non-async method that you wish to be called upon | ||
receiving an update. If a function overrides one with this decorator, it too | ||
must be decorated. | ||
|
||
|
@@ -1307,13 +1335,12 @@ def _apply_to_class( | |
issues: List[str] = [] | ||
|
||
# Collect run fn and all signal/query/update fns | ||
members = inspect.getmembers(cls) | ||
run_fn: Optional[Callable[..., Awaitable[Any]]] = None | ||
seen_run_attr = False | ||
signals: Dict[Optional[str], _SignalDefinition] = {} | ||
queries: Dict[Optional[str], _QueryDefinition] = {} | ||
updates: Dict[Optional[str], _UpdateDefinition] = {} | ||
for name, member in members: | ||
for name, member in inspect.getmembers(cls): | ||
if hasattr(member, "__temporal_workflow_run"): | ||
seen_run_attr = True | ||
if not _is_unbound_method_on_cls(member, cls): | ||
|
@@ -1406,9 +1433,9 @@ def _apply_to_class( | |
|
||
if not seen_run_attr: | ||
issues.append("Missing @workflow.run method") | ||
if len(issues) == 1: | ||
raise ValueError(f"Invalid workflow class: {issues[0]}") | ||
elif issues: | ||
if issues: | ||
if len(issues) == 1: | ||
raise ValueError(f"Invalid workflow class: {issues[0]}") | ||
raise ValueError( | ||
f"Invalid workflow class for {len(issues)} reasons: {', '.join(issues)}" | ||
) | ||
|
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.
Is there value in separating out this single-use method? I don't mind if just for clarity, just confirming there is not another reason
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.
Yes, I strongly object to code styles that fail to break out helper functions. It doesn't matter at all to me whether they are used once or multiple times at this point in the codebase. Some reasons (could write much more...):
Abstraction: if there are (say) 10 or more lines within a function that create their own independent subset of local function state with the sole aim of computing a value, then it is much easier to read and reason about if that is broken out as a helper function. There is no need for the reader to understand the computation if they don't want to; it's one of the main reasons functions exist. As an extra benefit, the helper function is an opportunity to choose a good descriptive name for the unit of functionality. It also gives a target for unit testing.
Even if it were desirable as an alternative to a helper functions (I don't think it is) Python doesn't have block-level scope; it's not possible to determine how the local state interacts with other parts of the function, without reading all the implementation detail closely and mentally tracking scope / lifetime of each variable.
Python has a strong and long tradition of being writing as a collection of small functions; I think this is due to its long history of having no static typing; even moreso without static typing, functions have to be small to give readers and writers a chance of avoiding bugs.
https://martinfowler.com/bliki/FunctionLength.html is somewhat similar to my view.
Reasons against:
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.
All good, no strong preference. For the same reason people have a hard time reading many lines in a method, they have a hard time reading many methods in a class. I just want to make sure we strike a good balance and not have a ton of small, single-use methods littering the class (as opposed to normal reusable methods).
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.
Yeah I think it's the great often forgotten-about religious war in programming :)
I don't agree that there's an equivalence, but I agree that the scope of helper functions has to be tastefully chosen, and that the author has to be very good at choosing function names. With those two things in hand, helper functions basically only make it easier to read. The point is that you can choose how deep into implementation detail you wish to go, whereas without helper functions you cannot choose.
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.
For myself, I struggle hopping around amongst disparately placed logic compared to scrolling (but still don't want to scroll too much of course)
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.
Right, so the question is: why do you feel the need to hop to the implementation? A helper function has a return type, and a well-chosen name. That should be all that you need to understand the implementation at the call site: you don't need to see the implementation detail of the helper function. Is the issue that you don't trust methods to not mutate instance state? If so, that implies that you'd be happier with helper functions (with immutable parameters), just not so much with methods (in Python).
But the issue of mutating state is where helper functions bring a big advantage: if you include unrolled implementation logic at the call site then in most languages it's very hard to tell whether the implementation is mutating local state, whereas a helper function makes this clearer or even guaranteed (i.e. in Python, pass immutable arguments; in other languages pass non-writable references, etc).
In general, IMO, complex logic should be written as a sequence of function calls. For example, this should be preferred over an unrolled version, even if the functions are called once only.