-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[ResponseAPI] Further polish message serialization and unit tests #26728
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
Conversation
Signed-off-by: Jialin Ouyang <[email protected]>
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.
Code Review
This pull request refactors the unit tests for message serialization and improves the serialization logic. The movement of tests into a dedicated file test_protocol.py is a good change for code organization. The update in serialize_message to check for hasattr(msg, "to_dict") is more explicit and robust than checking for __dict__.
However, I've found a critical issue in the serialize_message function where it can return inconsistent types (a dict or a str), which will lead to incorrect JSON serialization for certain messages. I've left a detailed comment with a recommended fix. Addressing this is crucial for the correctness of the Response API.
yeqcharlotte
left a comment
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.
thanks for the change!
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: 1994 <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: Dhruvil Bhatt <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: bbartels <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: 0xrushi <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]> Signed-off-by: 0xrushi <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]>
…lm-project#26728) Signed-off-by: Jialin Ouyang <[email protected]>
Purpose
Address leftover comments in #26620
Test Plan & Test Result
With the change, unit tests are still finished successfully
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.