Conversation
wenjin272
left a comment
There was a problem hiding this comment.
HI, @addu390, thanks for your work. Due to some urgent tasks that came up, I apologize for not responding in a timely manner.
I think the intent of this PR aligns with our expectations, but there are some disagreements regarding the specific design.
I think we can keep the subclassed events, but there's no need to retain the support for declaring actions and sending events according to FQCNs and the cloudpickle SerDe . We can standardize on using the type string and json SerDe.
For the type safety, we can validate the data field when receive the event. Take internal ChatRequestEvent as an example.
class ChatRequestEvent(Event):
"""Event representing a request to chat model.
Attributes:
----------
model : str
The name of the chat model to be chatted with.
messages : List[ChatMessage]
The input to the chat model.
output_schema: OutputSchema | None
The expected output schema of the chat model final response. Optional.
"""
def __init__(self, model: str, messages: List[ChatMessage], output_schema: OutputSchema):
super().__init__(type="_chat_request_event", attributes={"model": model,
"messages": messages,
"output_schema": output_schema})
@classmethod
@override
def from_event(cls, event: Event) -> "ChatRequestEvent":
assert "model" in event.attributes
assert "messages" in event.attributes
assert "output_schema" in event.attributes
return ChatRequestEvent(event.attributes["messages"], event.attributes["output_schema"])
@property
def model(self) -> str:
return self.attributes["model"]
@property
def messages(self) -> List[ChatMessage]:
return self.attributes["messages"]
@property
def output_schema(self) -> OutputSchema:
return self.attributes["output_schema"]
When a action received the chat request event, they can call ChatRequestEvent.from_event(event) to validate the event, and then use it as before.
|
|
||
| 2. **Shorthand form** — provide ``identifier`` and key-value attrs:: | ||
|
|
||
| ctx.send_event(identifier="MyEvent", field1="test", field2=1) |
There was a problem hiding this comment.
In #424, I described that I would like to support this shorthand form. But after the offline discuss with @xintongsong, we consider this way to be somewhat too casual.
I think we can keep support only for the first approach, having send_event accept only pre-built event object. This ensures consistency of the send_event API between Python and Java, and eliminates the need to introduce a _send_event method.
|
|
||
|
|
||
| def action(*listen_events: Type[Event]) -> Callable: | ||
| def action(*listen_events: Type[Event] | str) -> Callable: |
There was a problem hiding this comment.
I think we can just remove support for Type[Event], and only support declaring type-identifier strings in the action decorator.
Regarding type safety and ease of use, I will describe them in another comment.
|
|
||
|
|
||
| def test_input_event_ignore_row_unserializable() -> None: # noqa D103 | ||
| def test_input_event_ignore_row_unserializable_duplicate() -> None: # noqa D103 |
There was a problem hiding this comment.
Why we need modify this method name.
|
|
||
| Unified events (where ``event.type`` is set) are serialized as JSON and | ||
| sent via ``sendUnifiedEvent`` so that Java can reconstruct them without | ||
| cloudpickle. Subclassed events use the legacy cloudpickle path. |
There was a problem hiding this comment.
In our plan, we will support cross-language usage of events and actions. To support call actions in another language, we should always serialize events to json, including unified events and subclassed events, so that another language can reconstruct them. The cloudpickle path should be removed.
| """Test that unified events survive JSON serialization/deserialization.""" | ||
| import json | ||
|
|
||
| original = Event(type="RoundTrip", attributes={"a": 1, "b": "two"}) |
There was a problem hiding this comment.
I think Row field should also be tested, for when user using Table api, the input of InputEvent will be Row type.
Linked issue: #424
Purpose of change
Add unified event support. Users can create events with a
typestring andattributesmap instead of defining subclasses. Enables string-based event routing and JSON-based cross-language transport. Fully backward compatible with existing subclassed events.Tests
New unit tests in Java (
EventTest,EventLogRecordJsonSerdeTest) and Python (test_event,test_decorators,test_agent_plan,test_local_execution_environment) covering unified event creation, serialization, routing, and backward compatibility.API
Yes.
Eventclass (Java/Python) gainstype,attributes,getType()/get_type().@Actionannotation gainslistenEventTypes().@actiondecorator accepts string identifiers.RunnerContext.send_event()accepts string identifier with kwargs.More details about the design in the issue #424
Documentation
doc-neededdoc-not-neededdoc-included