wip: first attempt to workflow client #109

AstraBert · 2025-09-23T20:01:05Z

Attempting the creation of a workflow client

coveralls · 2025-09-23T20:02:47Z

Pull Request Test Coverage Report for Build 18064673242

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 90.271%

Totals
Change from base Build 18041425758:	0.0%
Covered Lines:	1531
Relevant Lines:	1696

💛 - Coveralls

adrianlyjak · 2025-09-23T22:02:04Z

pyproject.toml


 [project.optional-dependencies]
 server = ["starlette>=0.39.0", "uvicorn>=0.32.0"]
+client = ["httpx>=0.28.1"]


we should bound this to <1. Looks like they're working on some breaking changes

adrianlyjak · 2025-09-23T22:05:05Z

src/workflows/client/client.py

+            **kwargs: Any number of keyword arguments that would be passed on as additional keyword arguments to the workflow.
+
+        Returns:
+            str: ID of the handler running the workflow


Maybe return a typed dict for the workflow handler dict that @zhaotai is adding? #110

adrianlyjak · 2025-09-23T22:06:06Z

src/workflows/client/client.py

+
+            return response.json()["handler_id"]
+
+    async def _stream_events_sse(


I don't see a reason to support both of these. I think for python, the ndjson is a lot simpler

adrianlyjak · 2025-09-23T22:10:04Z

src/workflows/client/client.py

+                        if line.strip():  # Skip empty lines
+                            try:
+                                event = json.loads(line.replace("\n", ""))
+                                yield event.get("value", {})


this shouldn't just emit the value. Ignoring the qualified_name is throwing out valueable information, otherwise the caller doesn't know exactly what type this is supposed to be. Especially with workflows, where there's lots of meta programming based on classes (e.g. there's often events without even any values).

We could also perhaps have an option to plumb it through the JsonSerializer in case they happen to have the same python class around? Not sure what that would really look like though.

adrianlyjak · 2025-09-23T22:10:51Z

src/workflows/client/client.py

+    async def stream_events(
+        self,
+        handler_id: str,
+        event_callback: Callable[[dict[str, Any]], Any] | None = None,


why isn't this just an async generator too? That seems easier than having a callback

adrianlyjak · 2025-09-23T22:15:10Z

src/workflows/client/client.py

+        """
+        if isinstance(event, Event):
+            try:
+                event = event.model_dump_json()


this won't work. We need to use that JsonSerializer, otherwise the server will just get this back of json values, and not know what it's supposed to deserialize to. I think rather than requiring a dict[str, Any], we should have something simple like a TypedDict or yet another pydantic model to represent the serialized state. (the thing with the qualified_name)

adrianlyjak · 2025-09-23T22:16:09Z

src/workflows/client/client.py

+            if response.status_code == 202:
+                return
+
+            return response.json()["result"]


@zhaotai is also updating this API to return the full handler, so maybe don't throw out the rest of that info

adrianlyjak · 2025-09-23T22:17:56Z

IMO, I think we'll need a low level basic API client like this either way, so this is great, but I kind of think we should treat it as private, and rather expose an "easy to use" interface that looks more like the handler response from a workflow.run. That also maybe frees us up to make more internal changes as long as we keep the outer interface more stable

adrianlyjak · 2025-09-23T22:33:28Z

Also worth considering potentially generating a client for the lower API integration (with something like https://github.com/openapi-generators/openapi-python-client). The main advantage there is probably keeping us honest as types change while we develop the API.

We have the OpenAPI spec easily exportable already.

adrianlyjak

added some small questions around handler parameters. @AstraBert how do you feel about the generated client? I know I suggested it, but wondering if its helping or hurting (its astonishing how much code it generates!)

adrianlyjak · 2025-09-26T13:22:20Z

src/workflows/client/client.py

+        else:
+            raise ValueError("Response was not properly generated")
+
+    async def get_workflow_result(self, handler: Handler) -> Any:


shouldn't this just be a handler id? as input, and return a Handler as a result?

Yeah, it should, but I thought it would be easier and more future-proof passing the handler (maybe in the future we want to use more parameters from it). But it can be easily reverted to handler id, I also think it may be a little bit bloaty to put the entire handler there

adrianlyjak · 2025-09-26T13:23:23Z

src/workflows/client/client.py

+        """
+        Stream events using newline-delimited JSON format
+        """
+        url = f"/events/{handler.handler_id}?sse=false"


nit: url encode the handler in case its some garbage string that has slashes/question marks in it

I did not know it, I'll sanitize it later today along with reverting to pass onlu the handler id

adrianlyjak · 2025-09-26T13:23:33Z

src/workflows/client/client.py

+
+    async def get_workflow_events(
+        self,
+        handler: Handler,


shouldn't this just be a string id?

AstraBert · 2025-09-26T14:00:32Z

@adrianlyjak personally not a super fan of this last iteration, I feel like we could do more with less code (previous iteration :))

This reverts commit 8bb81c8.

This reverts commit b4bc1df.

AstraBert · 2025-09-27T20:39:14Z

Reverted back to the initial implementation with some adjustments, @adrianlyjak :)

adrianlyjak · 2025-09-30T17:24:58Z

src/workflows/client/client.py

+            return response.json().get("status", "") == "healthy"
+        return False
+
+    async def ping(self) -> float:


nit: not sure I see value in having this predefined in the client

adrianlyjak · 2025-09-30T17:26:44Z

src/workflows/client/client.py

+        Returns:
+            dict[str, Any]: JSON representation of the handler running the workflow
+        """
+        if isinstance(start_event, StartEvent):


nit: nice to have some shared functions to consolidate this duplication

adrianlyjak · 2025-09-30T17:28:02Z

src/workflows/client/client.py

+                context = context.to_dict()
+            except Exception as e:
+                raise ValueError(f"Impossible to serialize the context because of: {e}")
+        request_body = {


nit: I think we should build out some shared TypedDict models between client/server to lightly type things and make type discrepancies obvious

adrianlyjak · 2025-09-30T17:30:43Z

src/workflows/client/client.py

+        workflow_name: str,
+        start_event: Union[StartEvent, dict[str, Any], None] = None,
+        context: Union[Context, dict[str, Any], None] = None,
+        **kwargs: Any,


I've never actually found a use case for these kwargs. I'd recommend hiding them from the interface to avoid breaking changes, and if we do need to introduce them, use Unpack[TypedDict] types so that the exact set of supported options is obvious and documented

adrianlyjak · 2025-09-30T17:31:06Z

src/workflows/client/client.py

+        self,
+        workflow_name: str,
+        start_event: Union[StartEvent, dict[str, Any], None] = None,
+        context: Union[Context, dict[str, Any], None] = None,


this and run are missing the new handler_id option

adrianlyjak · 2025-09-30T17:31:38Z

src/workflows/client/client.py

+
+    async def get_workflow_events(
+        self,
+        handler_id: str,


needs the new include_internal option

adrianlyjak · 2025-09-30T17:32:19Z

src/workflows/client/client.py

+        """
+        if isinstance(event, Event):
+            try:
+                event = event.model_dump_json()


this is the wrong way to serialize. These need to be the JsonSerializer serialized values, otherwise the server will not know what type to deserialize to

adrianlyjak · 2025-09-30T17:33:23Z

src/workflows/client/client.py

+            if response.status_code == 202:
+                return
+
+            return response.json()["result"]


I think we should expose the full response TypedDict (already defined for the server), and return that. That way you can get the current status, etc.

adrianlyjak · 2025-09-30T17:33:54Z

src/workflows/client/client.py

+logger = getLogger(__name__)
+
+
+class WorkflowClient:


I don't see a /handlers method added here. Should we include that? Related to the /result api, it returns a list of the same data model

wip: first attempt to workflow client

2120c0a

adrianlyjak reviewed Sep 23, 2025

View reviewed changes

AstraBert and others added 3 commits September 24, 2025 13:06

Merge branch 'main' into clelia/workflows-client

16e01a9

chore: use openapi as a base for client

b4bc1df

chore: implement events streaming the good ol way

8bb81c8

AstraBert marked this pull request as ready for review September 25, 2025 14:53

adrianlyjak reviewed Sep 26, 2025

View reviewed changes

AstraBert added 3 commits September 27, 2025 22:23

Revert "chore: implement events streaming the good ol way"

beecc82

This reverts commit 8bb81c8.

Revert "chore: use openapi as a base for client"

bc97f89

This reverts commit b4bc1df.

chore: implement suggestions from first review

62f136d

Merge branch 'main' into clelia/workflows-client

f95b057

adrianlyjak reviewed Sep 30, 2025

View reviewed changes


		return response.json()["handler_id"]

		async def _stream_events_sse(

wip: first attempt to workflow client #109

Are you sure you want to change the base?

wip: first attempt to workflow client #109

Uh oh!

Conversation

AstraBert commented Sep 23, 2025

Uh oh!

coveralls commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18064673242

Details

💛 - Coveralls

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!

adrianlyjak commented Sep 23, 2025

Uh oh!

adrianlyjak commented Sep 23, 2025

Uh oh!

adrianlyjak 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!

AstraBert commented Sep 26, 2025

Uh oh!

AstraBert commented Sep 27, 2025

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!

coveralls commented Sep 23, 2025 •

edited

Loading