Converters and scaffolding #4

cretz · 2022-01-31T20:52:12Z

What was changed

Added initial impl of data converters. Also added some scaffold for the bridge and client, but both are likely to change quite a bit, so no need to review them too hard. Just trying to get eyes before moving on.

# Conflicts: # tests/api/test_grpc_stub.py

temporalio/client/client.py

temporalio/client/__init__.py

temporalio/client/client.py

temporalio/client/workflow_execution.py

temporalio/converter.py

bergundy · 2022-02-01T01:38:39Z

temporalio/converter.py

+    """Base converter to/from single payload/value."""
+
+    @abstractmethod
+    async def encode(self, value: Any) -> Optional[temporalio.api.common.v1.Payload]:


Not convinced this needs to be an async method, it shouldn't do any IO or CPU intensive operations.
The DataConverter should be async because it's responsible for compression and encryption.

Y'all chose to make it async-capable in TypeScript, why not here? What are the concerns with making it async?

I think it needs to be async unless we plan to separate out bytes-only stuff soon, as that easily could be cpu heavy for encryption (or need to do IO by reading keys from some source, etc)

We're going to make the TS PayloadConverter synchronous.
In TS, async framework code could change the order of workflow execution which could break determinism but that might not apply here, and we'd need to restructure things a bit for that.
I would suggest that users don't do any CPU or IO in the PayloadConverter and instead do it in the higher level DataConverter where multiple payloads could be compressed together.

We're going to make the TS PayloadConverter synchronous.

Why? What are the concerns with making it async?

In TS, async framework code could change the order of workflow execution which could break determinism but that might not apply here, and we'd need to restructure things a bit for that.

Why is this a problem for payload converters but not data converters?

I would suggest that users don't do any CPU or IO in the PayloadConverter and instead do it in the higher level DataConverter where multiple payloads could be compressed together.

While we might suggest this, the question is why is it ok for data converters to be async but not payload converters?

In TS we're going to separate conversion into 2 steps, one sync that maps object -> payload and runs in workflow context, and the other async that maps payloads -> payloads and runs outside of workflow context.

I see. I think if we were using subprocess sandboxing or similar we might need to do the same (and we'd also have to take the class of a data converter instead of an instance of it), but I am hoping our sandboxing does not cross some boundary requiring serialization to occur on the workflow side of that boundary. Though a method on a dataclass could provide a way to escape the sandbox. Will think on it.

Even if it does, I think the async payload conversion in the asyncio event loop will be in a predictable order.

The order is predictable but if you change the workflow data converter it's essentially a breaking non-deterministic change.

Here I don't expect the data converter to be inline with the workflow wrt determinism, nor am I more concerned about this change of a data converter mid-use any more than other types of data converter changes mid-use.

temporalio/converter.py

bergundy · 2022-02-01T01:47:51Z

tests/converter_test.py

+    await assert_payload(False, "json/plain", "false")
+
+    # Unknown type
+    with pytest.raises(RuntimeError) as excinfo:


I think TypeError is better here.

Not sure it's a type error necessarily vs lack of converter. I have now documented the error.

…date python version in CI

Sushisource · 2022-02-01T18:00:45Z

temporalio/converter.py

+# TODO(cretz): Should this be a var that can be changed instead? If so, can it
+# be replaced _after_ client creation? We'd just have to fallback to this
+# default at conversion time instead of instantiation time.


If we do that, IMO it'd make sense to keep this and also introduce some kind of set_default so that you don't wipe out the original defaults and they're always easily available if you want to restore them for some reason.

Yeah, I think I'd like to keep it an immutable default. We have ways to set the default at the client level which then applies to all things within anyways. You don't need to set the "default default" I don't think (we don't allow it in Go).

The other SDKs have an immutable default converter, but I see Spencer's point about setting the "default default" being more convenient than having to pass data converters everywhere.

👍 Keeping it immutable here. I have exposed converters from the default for easy creation of a new one.

bergundy

No blocking comments.

bergundy · 2022-02-01T20:13:31Z

temporalio/converter.py

+        Args:
+            values: Values to be converted.
+
+        Returns:
+            Converted payloads. Note, this does not have to be the same number
+            as values given, but at least one must be present.
+
+        Raises:
+            Exception: Any issue during conversion.


My rationale is that's what pycharm defaults to and from my experience is the most popular format.
I don't have a strong preference, my request was based on personal experience.

bergundy · 2022-02-01T20:19:11Z

temporalio/converter.py

+    """Base converter to/from single payload/value."""
+
+    @abstractmethod
+    async def encode(self, value: Any) -> Optional[temporalio.api.common.v1.Payload]:


We're going to make the TS PayloadConverter synchronous.
In TS, async framework code could change the order of workflow execution which could break determinism but that might not apply here, and we'd need to restructure things a bit for that.
I would suggest that users don't do any CPU or IO in the PayloadConverter and instead do it in the higher level DataConverter where multiple payloads could be compressed together.

bergundy · 2022-02-01T20:22:21Z

temporalio/converter.py

+        """See base class."""
+        message_type = payload.metadata.get("messageType", b"<unknown>").decode()
+        try:
+            value = _sym_db.GetSymbol(message_type)()


Since we have the type hint here I recommend using that and falling back to the messageType header.

The type hint is optional for proto payloads, the message type is not (at least how we're developing this across SDKs), so we might as well use the thing we know is always there.

I know Dmitry had his concerns with breaking compatibility if the message name changes.

If we're concerned with message name changes in the payload, he should also be concerned with incompatible type hint changes. As it stands, there are changes a dev can make that can break things.

bergundy · 2022-02-01T20:25:55Z

temporalio/converter.py

+# TODO(cretz): Should this be a var that can be changed instead? If so, can it
+# be replaced _after_ client creation? We'd just have to fallback to this
+# default at conversion time instead of instantiation time.


The other SDKs have an immutable default converter, but I see Spencer's point about setting the "default default" being more convenient than having to pass data converters everywhere.

cretz · 2022-02-04T14:08:46Z

Gonna go ahead and merge this given a lack of large outstanding concerns and with an impending review for the client coming.

cretz added 9 commits January 27, 2022 15:35

Rework grpc locations and add some different structure

509e7a6

Support moving protos to top-level of module

ca18e1b

Add mypy check

2c1b826

Fix test stub

1c0f2c4

Initial work on client and converter

d04150b

trigger GitHub actions

ccbdb01

Converter docs and tests

da44910

Rework converters to support payload converter and type hint

4431156

Merge remote-tracking branch 'remotes/origin/main' into initial-impl

eca5b0a

# Conflicts: # tests/api/test_grpc_stub.py

cretz requested review from bergundy and Sushisource January 31, 2022 20:52

cretz commented Jan 31, 2022

View reviewed changes

temporalio/client/client.py Outdated Show resolved Hide resolved

bergundy reviewed Feb 1, 2022

View reviewed changes

cretz force-pushed the initial-impl branch from 0981522 to 4192a77 Compare February 1, 2022 16:02

Remove client and common packages, rework some converter approach, up…

4192a77

…date python version in CI

Sushisource reviewed Feb 1, 2022

View reviewed changes

bergundy approved these changes Feb 1, 2022

View reviewed changes

cretz merged commit 7b5a69c into temporalio:main Feb 4, 2022

cretz deleted the initial-impl branch February 4, 2022 14:09

cretz mentioned this pull request Feb 10, 2022

Early client impl and docs scaffolding #6

Merged

Converters and scaffolding #4

Converters and scaffolding #4

Uh oh!

Conversation

cretz commented Jan 31, 2022

What was changed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergundy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz commented Feb 4, 2022

Uh oh!

Uh oh!

cretz Feb 1, 2022 •

edited

Loading