Grant-onboarding #7

GSmithApps · 2025-01-13T16:12:29Z

Providing some optional changes while I was taking the course. Cheers!

The bottom half of the diffs are about changing the logic of the Workflow a bit. It probably warrants more discussion if you're interested. For example, I felt confused when I worked through the following code. For example, this internal loop seems to only run once because it has a return statement. And based on the TS example, i was thinking we wanted to be able to send a signal with False that means we couldn't fulfill, and the workflow should stop and not bill the customer, and I don't think this code supports that

    @workflow.run
    async def order_pizza(self, order: PizzaOrder) -> OrderConfirmation:

        # ...

        while True:
            await workflow.wait_condition(
                lambda: not self._pending_confirmation.empty()
            )

            while not self._pending_confirmation.empty():
                bill = Bill(
                    customer_id=order.customer.customer_id,
                    order_number=order.order_number,
                    description="Pizza order",
                    amount=total_price,
                )
                confirmation = await workflow.execute_activity_method(
                    PizzaOrderActivities.send_bill,
                    bill,
                    start_to_close_timeout=timedelta(seconds=5),
                )
                return confirmation

    @workflow.signal
    async def fulfill_order_signal(self, success: bool) -> None:
        if success == True:
            await self._pending_confirmation.put(True)

GSmithApps · 2025-01-13T16:14:57Z

exercises/querying-workflows/README.md

-1. Edit the `workflow.py` file. The Workflow runs a blocking `while True` loop that will cause it to wait for either the `exit()` Signal or a `submit_greeting()` Signal before doing anything. A variable called `self._current_state` that provides information about the Workflow Execution is also updated in several places. You will add a Query that returns the status of this variable.
+1. Edit the `workflow.py` file.
+
+The Workflow runs a blocking `while True` loop that will cause it to wait for either the `exit()` Signal or a `submit_greeting()` Signal before doing anything. A variable called `self._current_state` that provides information about the Workflow Execution is also updated in several places. You will add a Query that returns the status of this variable.
+


One time Tom suggested to me to try to separate my instructions from the prose/explanation. That's what I was going for here. But totally up to you 👍

GSmithApps · 2025-01-13T16:15:20Z

exercises/querying-workflows/README.md

+Notice that because `workflow.py` shuts down the Worker when
+the Workflow finishes, you cannot Query the Workflow after you
+have Signalled it to exit. In contrast, if you ran the Worker
+in one file/terminal and the Workflow in another, the Workflow
+would shut down and the Worker would keep running, and you could
+Query it even after it has exited.
+


Can include this if we want. Up to you 👍

GSmithApps · 2025-01-13T16:15:45Z

exercises/querying-workflows/practice/workflow.py

-    @workflow.query
-    def current_state_query(self) -> str:
-        return self._current_state


This was in the file, but I don't think it should be. I think putting it here is part of the exercise

GSmithApps · 2025-01-13T16:17:48Z

exercises/querying-workflows/practice/workflow.py

-            task_queue="queries",
+            task_queue="queries-queue",


the workflow ID and the task queue were both called queries, so I just changed the queue to queries-queue to disambiguate. Not necessary -- up to you

GSmithApps · 2025-01-13T16:19:03Z

exercises/sending-signals-client/practice/workflow.py

        greetings: List[str] = []
        workflow.logger.info("Running workflow.")
-        print("Running workflow.")
+


I know it isn't production code, but I figure we don't want print statements in workflow code

GSmithApps · 2025-01-13T16:19:43Z

exercises/sending-signals-external/README.md

-1. This exercise contains one Client that runs two different Workflows
-   — `PizzaWorkflow.order_pizza` and `FulfillOrderWorkflow.fulfill_order`. Both
-   Workflows are defined in `workflow.py`. `PizzaWorkflow.order_pizza` is
-   designed not to complete its final activity — `send_bill` — until it receives
-   a Signal from `FulfillOrderWorkflow.fulfill_order`. You'll start by defining
-   that Signal. Edit `workflow.py`. At the bottom of the `PizzaWorkflow`
+This exercise contains one Client that runs two different Workflows
+— `PizzaWorkflow.order_pizza` and `FulfillOrderWorkflow.fulfill_order`. Both
+Workflows are defined in `workflow.py`. `PizzaWorkflow.order_pizza` is
+designed not to complete its final activity — `send_bill` — until it receives
+a Signal from `FulfillOrderWorkflow.fulfill_order`. You'll start by defining
+that Signal. 
+
+1. Edit `workflow.py`. At the bottom of the `PizzaWorkflow`


same as above. just separating prose from instruction. totally up to you 👍

GSmithApps · 2025-01-13T16:50:26Z

exercises/sending-signals-external/practice/workflow.py

 class PizzaOrderWorkflow:
    def __init__(self) -> None:
-        self._pending_confirmation: asyncio.Queue[str] = asyncio.Queue()
+        self._pending_confirmation: asyncio.Queue[bool] = asyncio.Queue()


I think this was a copy-paste thing. The queue is bools

GSmithApps · 2025-01-13T16:51:07Z

exercises/sending-signals-external/practice/workflow.py

-    async def order_pizza(self, order: PizzaOrder) -> OrderConfirmation:
+    async def order_pizza(self, order: PizzaOrder) -> OrderConfirmation | None:


support if the order was not fulfilled and we don't return a confirmation

GSmithApps · 2025-01-13T16:52:03Z

exercises/sending-signals-external/solution/workflow.py

 from temporalio import workflow
 from temporalio.exceptions import ApplicationError

 # Import activity, passing it through the sandbox without reloading the module


These 👇 are the same changes as above, but in the solution instead of the practice

GSmithApps · 2025-01-13T16:55:06Z

exercises/sending-signals-external/practice/workflow.py

-        while True:
-            await workflow.wait_condition(
-                lambda: not self._pending_confirmation.empty()
-            )
+        await workflow.wait_condition(
+            lambda: self._signal_received,
+            timeout=3
+        )


I have a feeling I'm missing something, but I don't think we need to loop. Like, if we want to just get the signal and if it's yes or no, bill the customer accordingly, then I don't think we need it. But if we want to sort of wait for multiple signals, then maybe we'd need a loop. Curious what you think

Or maybe there's something in the readme or course content that I missed

Yeah, this is kind of a weird loop implementation, but based on earlier conversations with the Python SDK team and trying to align with our samples, Python really likes to use loops and queues for Signal detection. I'd like to take everything in this PR except these last two changes -- yes they're a bit weird, but I think they illustrate some expandable patterns.

Interesting discussion!

Regarding the differences between main and @GSmithApps's PR, there is a flaw in the Python SDK (temporalio/sdk-python#618) which means that the outer while loop is indeed technically necessary. However, I am not seeing a need for the inner while loop: if we enter it then we return or throw and hence never re-enter it.

At a higher level though, my take is that the current code (in main) is handling the signal in a Go-like style: pushing the work into a queue which is processed in the main workflow body. And I would say that that is not idiomatic for the Python SDK (or any SDK other than Go): the purpose of handler functions is that the implementation details of handling can be isolated in the handler function. (There are advantages other than modularity and implementation-hiding, e.g. the handling can more easily be instrumented with observability emissions if it is done in a handler function). Effectively, distinct handler invocations form their own queue.

So, my question is: is there a reason why these materials are encouraging the queue style? If not, then I think we should go with neither the code in main, nor the adjustments made in @GSmithApps's PR, but rather switch to the handler-based implementation.

@drewhoskins-temporal addressed this topic in a recent blog post, as you probably know: https://temporal.io/blog/robust-message-handlers#the-battle-between-handlers-and-queues

GSmithApps · 2025-01-13T16:56:07Z

exercises/sending-signals-external/practice/workflow.py

-            while not self._pending_confirmation.empty():
-                bill = Bill(
-                    customer_id=order.customer.customer_id,
-                    order_number=order.order_number,
-                    description="Pizza order",
-                    amount=total_price,
-                )
-                confirmation = await workflow.execute_activity_method(
-                    PizzaOrderActivities.send_bill,
-                    bill,
-                    start_to_close_timeout=timedelta(seconds=5),
-                )
-                return confirmation
+        if self._pending_confirmation.get_nowait():
+            bill = Bill(
+                customer_id=order.customer.customer_id,
+                order_number=order.order_number,
+                description="Pizza order",
+                amount=total_price,
+            )
+            confirmation = await workflow.execute_activity_method(
+                PizzaOrderActivities.send_bill,
+                bill,
+                start_to_close_timeout=timedelta(seconds=5),
+            )
+            return confirmation


my understanding is that we want to check the signal for true or false, then bill the customer accordingly

See above comment. And thanks for this!

GSmithApps added 3 commits January 9, 2025 10:38

notes while onboarding

37f8db0

good

e797bbd

requirements.txt

9c7330c

GSmithApps commented Jan 13, 2025

View reviewed changes

changes to sending signals workflow

5c4b718

GSmithApps commented Jan 13, 2025

View reviewed changes

GSmithApps marked this pull request as ready for review March 18, 2025 17:18

		async def order_pizza(self, order: PizzaOrder) -> OrderConfirmation:
		async def order_pizza(self, order: PizzaOrder) -> OrderConfirmation \| None:

Grant-onboarding #7

Are you sure you want to change the base?

Grant-onboarding #7

Uh oh!

Conversation

GSmithApps commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

GSmithApps Jan 13, 2025 • 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!

GSmithApps Jan 13, 2025 • 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!

dandavison Apr 9, 2025 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GSmithApps commented Jan 13, 2025 •

edited

Loading

GSmithApps Jan 13, 2025 •

edited

Loading

GSmithApps Jan 13, 2025 •

edited

Loading

dandavison Apr 9, 2025 •

edited

Loading