Upsert memo support

[maciejdudko]

What was changed

Added workflow.upsert_memo function to support memo upsert functionality.

Why?

Feature request: temporalio/features#119

Checklist

1. Closes [Feature Request] Upsert memo support #190

2. How was this tested:

Modified test_workflow_memo to also test upsert operation.

3. Any docs updates needed?

[CLAassistant]

All committers have signed the CLA.

[cretz]

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).

[maciejdudko]

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.

[cretz]

👍 While the top level fields are read-only, we do mutate a couple of maps in there (memo and search attributes)

[cretz]

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.

[maciejdudko]

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).

[cretz]

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.

But we already do this with search attributes and this happens in other SDKs. I think it's actually more surprising to users when multiple instances of a workflow's memo are independent. Sure in some languages where immutable collections are more common it can make more sense to copy-on-write, but not here (and again we don't with other mutable collections like search attributes). We do have a type that tells people the map is an immutable view of it, so they shouldn't mutate, but we can't stop them.

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).

I think we can keep the behavior of lazily mutating the typed map only if it had been accessed before, but instead let it continue to be lazily created from raw (which we also update here) if it hasn't been accessed. All of the existing functionality for individual memo values by type hint is retained because it works off of raw. Only this one workflow_memo method does this deserialization with default type hints (so often dicts and arrays and such) and that should continue to work.

workflow.memo() should return the same object every time for clarity and consistency.

[cretz]

Suggested change

	self._untyped_converted_memo[k] = self._payload_converter.from_payload(
	v
	)
	self._untyped_converted_memo[k] = updates[k]

I don't think you should go back through the converter, it's fine if we use the value as the user gave it to us

[cretz]

After discussion, to have the same value whether they have called memo() before or not, we do need to re-convert here. Hopefully people don't use memo() and they use type-hint-based ones instead. Feel free to add comment mentioning that we re-convert because we don't want the addition of memo() earlier in the workflow to affect the this value.