-
Notifications
You must be signed in to change notification settings - Fork 97
Upsert memo support #858
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
base: main
Are you sure you want to change the base?
Upsert memo support #858
Conversation
@@ -218,8 +218,11 @@ def __init__(self, det: WorkflowInstanceDetails) -> None: | |||
self._current_history_length = 0 | |||
self._current_history_size = 0 | |||
self._continue_as_new_suggested = False | |||
self._raw_memo: Optional[ |
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 _raw_memo
needed here if it's already available in info? Also, we need to make sure we update the info's raw memo info on upsert. Note, we need to make sure we update the same dict, not recreate it (which you can't really anyways because it's a frozen data class).
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.
Oh, I thought _info
is read-only, that's why I made the other one. I've updated PR to operate on _info.raw_memo
directly.
# Clearing cached value, will be regenerated on next workflow_memo() call. | ||
self._untyped_converted_memo = None |
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.
We should update the existing dict with the updates IMO. A user may have assigned it to a variable. It also doesn't need to go back through the payload conversion process because you can just use the objects directly that you were given by the user. We should also document on memo()
that the result can be mutated with the results of upsert.
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.
I don't like an idea of handing over references to mutable state. I'd imagine most code would expect it to not change. Handing over a read-only copy feels safer and less surprising. Additionally, if the user ever modified the returned dict, our internal state would get desynchronized with what's in the command buffer and that smells like all kinds of trouble.
I deliberately round-trip the memo value for two reasons: to keep the result of the untyped memo read consistent regardless of how the memo was obtained (from server vs. from recent upsert), and to allow deserialization with different type hints than the original value (we already offer that ability in current SDK version, somebody somewhere probably depends on it).
What was changed
Added
workflow.upsert_memo
function to support memo upsert functionality.Why?
Feature request: temporalio/features#119
Checklist
Closes [Feature Request] Upsert memo support #190
How was this tested:
Modified
test_workflow_memo
to also test upsert operation.