💥 Add generic payload visitor for WorkflowActivation[Completion] #1075
Conversation
|
I'm not currently trying to cover all message types, just the two needed for workflow activation. Others can be added later as needed when we want to expose this to users. |
There was a problem hiding this comment.
I like this implementation. I added a couple of minor comments.
Can we make a parenthetical next to the Python "TODO" at temporalio/features#468 to point to this PR for when that work is started? Also, I'd like @dandavison to take a peek here if possible, this is big enough to deserve an extra pair of eyes IMO. Part of me wonders if we should leave the existing logic around and let it be used if an env var is set at least for a release or two if we're afraid of behavior here, but that may be going too far.
scripts/gen_visitors.py
Outdated
| for r in roots: | ||
| self.walk(r) | ||
|
|
||
| header = """ |
There was a problem hiding this comment.
I think we want something pretty visible here indicating that it's generated code.
scripts/gen_visitors.py
Outdated
| from temporalio.api.common.v1.message_pb2 import Payload | ||
| class VisitorFunctions(abc.ABC): | ||
| \"\"\"Set of functions which can be called by the visitor. Allows handling payloads as a sequence.\"\"\" |
There was a problem hiding this comment.
Python docstring law dictates that the first line shall have one sentence only and any subsequent sentences shall be separated by a newline from the first.
There was a problem hiding this comment.
I would maybe call it docstring suggestion if the docstring linter doesn't care, which it seemingly doesn't.
There was a problem hiding this comment.
Yeah not sure why that is. Regardless, it's the style we follow for public docstrings, so it's less distracting to just follow it everywhere. It's pretty ubiquitous
https://peps.python.org/pep-0008/#documentation-strings
https://peps.python.org/pep-0257/#multi-line-docstrings
There was a problem hiding this comment.
Wasn't trying to say I wouldn't
There was a problem hiding this comment.
Huge code cleanup and nice new utility!
It's obviously pretty involved and confirming correctness is going to come down to testing against a good range of inputs. The omes fuzzer could help give confidence. Also running the samples-python test suite against it. Let's discuss offline what more we're going to do before release.
3 participants
What was changed
Switched to using a new code generated visitor for encoding and decoding workflow activations.
Notably, this will include encoding summaries, which were not previously encoded. This is a slightly breaking change, as it will result in encoded summaries for users who are using codecs without a codec server. In addition, we no longer use the
encode_failureanddecode_failurehelpers. These weren't intended to be overridden to uniquely encode failure payloads, and aren't used everywhere.Why?
We want to encode summaries.
Checklist
Closes [Bug] Ensure every inbound and outbound workflow payload goes through codec #1071
How was this tested: