Conversation
|
Hi @wenjin272, I closed the PR #561 and opened this. Do take a first pass when you get a chance. |
There was a problem hiding this comment.
Hi, @addu390, thanks for you work. Refactoring the Event will have a significant impact on the framework; therefore, this work requires careful design and attention to numerous details. Thanks Again.
I think though some details still need to be addressed, the current Python event design aligns with my expectations, including
- action listen to event.type
- json based DeSer
- remove
PythonEventin java, unified inEvent - Subclasses of
Eventprovidefrom_eventmethod to validate and reconstruct specific event object.
However, the Java event design does not appear to be aligned with the Python approach. In fact, after completing the Event refactoring, we will continue to support cross-language usage of Actions. This means a Python event sent by a Python action may be delivered to a Java action. Therefore, Python events and Java events must maintain consistency in both design and usage.
By the way, there are some conflicts between the PR and the main branch, likely because the main branch was recently reformatted.
| * @return Array of Event classes that this action listens to | ||
| */ | ||
| Class<? extends Event>[] listenEvents(); | ||
| Class<? extends Event>[] listenEvents() default {}; |
There was a problem hiding this comment.
On the Python side, actions only support listening to event string identifiers; on the Java side, we should align with this behavior.
| * @param eventJson JSON string with at least a {@code "type"} field | ||
| * @throws IOException if JSON parsing fails | ||
| */ | ||
| public void sendUnifiedEvent(String eventJson) throws IOException { |
There was a problem hiding this comment.
I think the name sendUnifiedEvent is a bit ambiguous. I suggest renaming it to sendEventJson or sentPythonEvent.
| /** Returns the event type string used for routing. */ | ||
| @JsonIgnore | ||
| public String getType() { | ||
| return type != null ? type : this.getClass().getName(); |
There was a problem hiding this comment.
I think we can directly require that the type is not null, which would simplify the implementation of Event.
| private final String type; | ||
| private final Map<String, Object> attributes; | ||
| /** The timestamp of the source record. */ | ||
| private Long sourceTimestamp; |
There was a problem hiding this comment.
I think we can remove this field to keep the align between python Event and java Event
There was a problem hiding this comment.
sourceTimestamp seems to be used in runtime for timestamp propagation (ActionExecutionOperator, RunnerContextImpl, JavaActionTask, etc.). Safe to remove?
I could however, specifically not use in the cross-language event contract, without any repercussions.
| attributes={ | ||
| "model": model, | ||
| "messages": messages, | ||
| "output_schema": output_schema, |
There was a problem hiding this comment.
We need to ensure consistency between the built-in events on the Python and Java sides, so we must make the same changes on the Java side.
| Key-value properties for the event data. | ||
| """ | ||
|
|
||
| _type_registry: ClassVar[Dict[str, Type["Event"]]] = {} |
There was a problem hiding this comment.
It appears that this variable is intended to handle the deserialization of subclasses, but it is not currently in use. I think the Event class doesn't need to handle this; deserialization of subclass objects can be managed within the subclass's from_event method. Because subclasses may have certain non-trivial fields that require special handling during deserialization.
|
|
||
| def test_action_decorator() -> None: # noqa D103 | ||
| @action(InputEvent) | ||
| @action("_input_event") |
There was a problem hiding this comment.
I think we can use InputEvent.EVENT_TYPE, and the same applies to other places where @action is used.
| err_msg = ( | ||
| "Failed to send event '" | ||
| + event.get_type() | ||
| + "' to runner context" |
There was a problem hiding this comment.
I think we can also add the event json in err_msg.
| event = Event.from_json(event_json) | ||
| event_class = Event._type_registry.get(event.type) | ||
| if event_class is not None and hasattr(event_class, "from_event"): | ||
| return event_class.from_event(event) |
There was a problem hiding this comment.
Similar to my comment in Event, the framework does not need to be aware of the subclass types of Event. This would require the framework to handle many details of serialization and deserialization.
Fro subclass types of Event, user need use SubClass.from_event() to reconstruct the specific event object in action, like what you do in built-in tool call action and chat action.
|
@wenjin272 Agreed on consistency between Java and Python, the reasoning was that it was quite a larger change, so, was considering having a follow-up PR. |
…vent() in actions
aff5f51 to
744e498
Compare
|
@wenjin272 Thanks again for the review! I’ve addressed the broader concern of consistency between Python and Java implementations, along side other review comments. Please take another look. |
| @SuppressWarnings("unchecked") | ||
| public List<ChatMessage> getMessages() { | ||
| return messages; | ||
| return (List<ChatMessage>) getAttr("messages"); |
There was a problem hiding this comment.
In the python ChatRequestEvent, we deserialize the messages field in from_event method. But the java Event is missing corresponding method. This may cause ClassCastException.
|
|
||
| /** Base class for all event types in the system. */ | ||
| public abstract class Event { | ||
| public class Event { |
There was a problem hiding this comment.
The python Event contains a class method from_event. In this method, we will perform field validation and deserialization. However, Java's event lacks the corresponding method.
| public static Event fromJson(String json) throws IOException { | ||
| Event event = MAPPER.readValue(json, Event.class); | ||
| if (event.type == null || event.type.isEmpty()) { | ||
| throw new IllegalArgumentException("Event JSON must contain a 'type' field."); |
There was a problem hiding this comment.
I think we can move this validation to the construct method annotated @JsonCreator. This ensures that all code paths constructing an Event will enforce this validation.
| * The fully qualified Java class name used for deserialization. For unified events (no | ||
| * subclass), this is {@code "org.apache.flink.agents.api.Event"}. | ||
| */ | ||
| private final String eventClass; |
There was a problem hiding this comment.
I think EventContext does not need to know the specific event class.
This information will enable us to restore the specific subclass type when reconstructing events from the event log. However, I believe there is no requirement to deserialize events from the event log. Furthermore, being aware of specific event classes will make the framework's implementation more complex.
I think that if users utilize event subclasses instead of the base event directly, they should be responsible for handling event deserialization, specifically by uniformly using from_event/fromEvent.
|
@wenjin272 I have addressed the comments related to |
Sorry for the delay in reviewing — I'll get to this PR today or tomorrow. |
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. Migrates/removes 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().More details about the design in the issue #424
Documentation
doc-neededdoc-not-neededdoc-included