[Core] handle optional values in ray_event_converter #58083
Conversation
Signed-off-by: sampan <sampan@anyscale.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with how optional fields from TaskLifecycleEvent are handled during conversion. By conditionally setting fields like node_id, worker_id, worker_pid, and ray_error_info only when they have non-default values, you've prevented unintended overwrites during protobuf merges. The logic change is sound and directly fixes the described problem.
The addition of comprehensive tests is excellent. The parameterized test for node_id, worker_id, and worker_pid covers a wide range of scenarios effectively. I've left one suggestion in the test file to refactor the ray_error_info test to reduce code duplication and align it with the parameterized testing pattern used elsewhere in the file.
Overall, this is a solid improvement that enhances the robustness of the event conversion logic.
Signed-off-by: sampan <sampan@anyscale.com>
ray-gardener
bot
added
core
| task_event.set_attempt_number(event.attempt_number()); | ||
| task_event.set_job_id(event.job_id()); | ||
|
|
||
| *task_event.mutable_profile_events() = std::move(*event.mutable_profile_events()); |
There was a problem hiding this comment.
do we need to check for provile event emptiness before moving, similar to ray error info?
There was a problem hiding this comment.
yes; either we do the same thing here or line 155 doesn't need the if condition; i think for actual protobuf message (not scalar), we don't need to have the if condition (the mergeFrom work fine when merging empty message)
*task_state_update->mutable_error_info() = std::move(*event.mutable_ray_error_info());
There was a problem hiding this comment.
Ill do the same as what we are doing for error_info for consistency
Signed-off-by: sampan <sampan@anyscale.com>
|
build are failing though |
|
failures seem unrelated to changes in this pr, merging with main to see if that helps. |
|
yeah master branch is probably broken |
|
let's go |
## Description in the original taskEvent proto, worker_id is marked as optional https://github.com/ray-project/ray/blob/830a456b9b558028853423c9042f7e2763ec5283/src/ray/protobuf/gcs.proto#L201 but in ray event it is not https://github.com/ray-project/ray/blob/f635de7c86d0d0f813a305a9fd5e864a64257894/src/ray/protobuf/public/events_task_lifecycle_event.proto#L42 in the converter we always set the worker_id field even if its an empty string https://github.com/ray-project/ray/blob/master/src/ray/gcs/gcs_ray_event_converter.cc#L145. if an optional field is set, even if it is empty(default proto value) it is considered as having a value, and during mergeFrom() calls the value is considered and overwrites the destination objects existing value. source:https://protobuf.dev/programming-guides/field_presence/ Explicitly set fields – including default values – are merged-from this pr fixes this gap in the conversion logic --------- Signed-off-by: sampan <sampan@anyscale.com> Co-authored-by: sampan <sampan@anyscale.com> Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com>
## Description in the original taskEvent proto, worker_id is marked as optional https://github.com/ray-project/ray/blob/830a456b9b558028853423c9042f7e2763ec5283/src/ray/protobuf/gcs.proto#L201 but in ray event it is not https://github.com/ray-project/ray/blob/f635de7c86d0d0f813a305a9fd5e864a64257894/src/ray/protobuf/public/events_task_lifecycle_event.proto#L42 in the converter we always set the worker_id field even if its an empty string https://github.com/ray-project/ray/blob/master/src/ray/gcs/gcs_ray_event_converter.cc#L145. if an optional field is set, even if it is empty(default proto value) it is considered as having a value, and during mergeFrom() calls the value is considered and overwrites the destination objects existing value. source:https://protobuf.dev/programming-guides/field_presence/ Explicitly set fields – including default values – are merged-from this pr fixes this gap in the conversion logic --------- Signed-off-by: sampan <sampan@anyscale.com> Co-authored-by: sampan <sampan@anyscale.com> Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
Description
in the original taskEvent proto, worker_id is marked as optional
ray/src/ray/protobuf/gcs.proto
Line 201 in 830a456
but in ray event it is not
ray/src/ray/protobuf/public/events_task_lifecycle_event.proto
Line 42 in f635de7
in the converter we always set the worker_id field even if its an empty string https://github.com/ray-project/ray/blob/master/src/ray/gcs/gcs_ray_event_converter.cc#L145. if an optional field is set, even if it is empty(default proto value) it is considered as having a value, and during mergeFrom() calls the value is considered and overwrites the destination objects existing value.
source:https://protobuf.dev/programming-guides/field_presence/
Explicitly set fields – including default values – are merged-from
this pr fixes this gap in the conversion logic