-
Notifications
You must be signed in to change notification settings - Fork 14
Implement missing send_event capability for orchestration context #52
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?
Conversation
Use https://github.com/microsoft/durabletask-dotnet/blob/915b4c3b8dd82bfffaf399680a723b3762b6e348/src/Shared/Grpc/ProtoUtils.cs#L277 in the durabletask-dotnet SDK as reference for the sendevent implementation. |
Co-authored-by: berndverst <[email protected]>
Co-authored-by: berndverst <[email protected]>
send_event
…ation Co-authored-by: berndverst <[email protected]>
I've reviewed the durabletask-dotnet ProtoUtils.cs reference implementation and updated the send_event functionality accordingly. The key improvement is adding input validation for instance_id and event_name parameters to match the .NET SDK's validation patterns. The validation ensures that neither parameter can be null or empty, which aligns with the .NET implementation's validation approach. Updated in commit cd9d951. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Great. Lets add another advanced test: |
Co-authored-by: berndverst <[email protected]>
I've added the requested advanced test in commit 00b4957. The test demonstrates:
The test shows the complete flow: waiting orchestration starts → sender orchestration sends event with data payload → waiting orchestration receives event and completes with the data. The data passing is validated to ensure the approval payload (with approved flag, approver, and timestamp) flows correctly through the event communication. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Please address the linter error |
Co-authored-by: berndverst <[email protected]>
Fixed the linter error by removing whitespace from the blank line at line 862 in worker.py. Commit 7ba38ce. |
Please run the latest flake8 linter on the files you changed and address all errors until flake8 returns no more problems. You may want to format the files with autopep8 to speed up the process. |
Co-authored-by: berndverst <[email protected]>
Fixed all flake8 linting errors using autopep8 formatting. All files now pass flake8 with no issues and tests continue to pass. Commit ccfbefd. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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.
This is great! My main feedback is that send_event
should not return a task, so we should remove all code that tries to create, return, or yield tasks for send_event
calls.
durabletask/worker.py
Outdated
@@ -852,6 +852,22 @@ def wait_for_external_event(self, name: str) -> task.Task: | |||
task_list.append(external_event_task) | |||
return external_event_task | |||
|
|||
def send_event(self, instance_id: str, event_name: str, *, | |||
data: Optional[Any] = None) -> task.Task: | |||
# Validate inputs similar to .NET implementation |
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.
Let's remove this comment. The fact that a .NET implementation does basic input validation is not noteworthy, and might give the wrong impression to developers who read this code and aren't necessarily interested in .NET.
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.
Removed the .NET implementation comment as requested. Commit 4ca1234.
durabletask/worker.py
Outdated
self._pending_actions[id] = action | ||
send_event_task = task.CompletableTask() | ||
self._pending_tasks[id] = send_event_task | ||
return send_event_task |
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.
Sending an event is a one-way operation and is therefore not awaitable. There's no need to create a task for this, and no need to return anything from this method.
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.
Changed send_event to not create or return any task. It's now a one-way operation returning None. Commit 4ca1234.
durabletask/worker.py
Outdated
if send_event_task: | ||
# For send_event, we don't return any meaningful result, just completion | ||
send_event_task.complete(None) | ||
ctx.resume() |
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.
Similar to my other comment, sending an event is not an awaitable operation, so we should not have any tasks associated with sending events.
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.
Removed the eventSent handling and all task-related logic for send_event operations. Commit 4ca1234.
durabletask/task.py
Outdated
Returns | ||
------- | ||
Task | ||
A Durable Task that completes when the event has been sent. |
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.
Let's remove the return value from this method definition. This method should return 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.
Updated send_event abstract method to return None instead of Task. Commit 4ca1234.
tests/durabletask/test_send_event.py
Outdated
"""Test that send_event creates the correct action""" | ||
|
||
def orchestrator(ctx: task.OrchestrationContext, _): | ||
yield ctx.send_event("target_instance", "my_event", data="test_data") |
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.
Per my other comment, send_event
should return None
instead of returning a task, so we don't need to yield here.
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.
Removed yield from send_event calls since send_event now returns None. Commit 4ca1234.
tests/durabletask/test_send_event.py
Outdated
"""Test send_event with no data parameter""" | ||
|
||
def orchestrator(ctx: task.OrchestrationContext, _): | ||
yield ctx.send_event("target_instance", "my_event") |
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.
Per my other feedback, remove the yield
.
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.
Removed yield from send_event calls and updated test to expect all actions in single execution. Commit 4ca1234.
tests/durabletask/test_send_event.py
Outdated
|
||
def orchestrator(ctx: task.OrchestrationContext, _): | ||
yield ctx.send_event("target1", "event1", data="data1") | ||
yield ctx.send_event("target2", "event2", data="data2") |
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.
Per my other feedback, remove the yield
statements for each send_event
method call.
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.
Removed yield statements and updated test to expect all send_event actions plus completion in single execution. Commit 4ca1234.
tests/durabletask/test_send_event.py
Outdated
# Test with number | ||
yield ctx.send_event("target3", "event3", data=123) | ||
# Test with boolean | ||
yield ctx.send_event("target4", "event4", data=True) |
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.
Same feedback about removing yield
from the ctx.send_event
calls.
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.
Removed yield statements and updated test to expect all send_event actions plus completion in single execution. Commit 4ca1234.
tests/durabletask/test_send_event.py
Outdated
"""Test send_event input validation""" | ||
|
||
def orchestrator_empty_instance(ctx: task.OrchestrationContext, _): | ||
yield ctx.send_event("", "event1", data="test") |
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.
Same feedback about removing yield
from send_event
calls.
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.
Removed yield from send_event calls. The test now properly shows orchestration completing immediately with send event actions executed first. Commit 4ca1234.
tests/durabletask/test_send_event.py
Outdated
# Define the sender orchestration that sends an event to another orchestration | ||
def sender_orchestration(ctx: task.OrchestrationContext, target_instance_id: str): | ||
approval_payload = {"approved": True, "approver": "manager", "timestamp": "2024-01-01T10:00:00Z"} | ||
yield ctx.send_event(target_instance_id, "approval", data=approval_payload) |
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.
Same feedback about removing yield
from send_event
calls in orchestrator code.
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.
Removed yield from send_event calls and updated test to show sender orchestration completing immediately. Commit 4ca1234.
Updated send_event to return None instead of Task, making it a one-way operation. Removed all task creation and completion logic, and updated tests accordingly. All tests now pass. Commit 4ca1234. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: cgillum <[email protected]>
@@ -973,8 +988,9 @@ def process_event( | |||
# Start the orchestrator's generator function | |||
ctx.run(result) | |||
else: | |||
# This is an orchestrator that doesn't schedule any tasks | |||
ctx.set_complete(result, pb.ORCHESTRATION_STATUS_COMPLETED) | |||
# This is an orchestrator that doesn't use generators (async tasks) |
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.
revert this comment
This PR implements the missing
send_event
capability that was identified in issue #32. The implementation allows orchestrations to send events to other orchestration instances, which is required for the Dapr Python SDK integration.Changes Made
Core Implementation
Added abstract
send_event
method toOrchestrationContext
indurabletask/task.py
instance_id
,event_name
, and optionaldata
parametersTask
that completes when the event is sentImplemented
send_event
in_RuntimeOrchestrationContext
indurabletask/worker.py
SendEventAction
with proper sequence numberingAdded helper functions in
durabletask/internal/helpers.py
:new_send_event_action()
- Creates protobuf action for sending eventsnew_event_sent_event()
- Creates protobuf event for test supportAdded event completion handling in
durabletask/worker.py
eventSent
events to complete corresponding tasksUncommented and fixed action type mapping for
sendEvent
actionsTesting
tests/durabletask/test_send_event.py
Usage Example
Validation
The implementation leverages the existing protobuf definitions (
SendEventAction
andEventSentEvent
) that were already present but unused, making this a minimal and focused addition that integrates seamlessly with the existing codebase.Fixes #32.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
http://168.63.129.16:80/machine/
/usr/bin/python3 -u bin/WALinuxAgent-2.13.1.1-py3.9.egg -collect-logs
(http block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.