[core] fix None yield from restarted streaming generator with application errors - #65121
[core] fix None yield from restarted streaming generator with application errors#65121rueian wants to merge 12 commits into
Conversation
…tion errors Signed-off-by: Rueian Huang <rueiancsie@gmail.com>
b705c88 to
81b3443
Compare
Signed-off-by: Rueian Huang <rueiancsie@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request ensures that streaming generator replays failing with an application error are correctly validated for object count consistency, and prevents copying the static None return onto stream ObjectRefs during such replays. Specifically, FailStreamingGeneratorReplayIfInconsistent is now executed for both successful and application-error completions. Additionally, regression and unit tests have been added to verify these behaviors. There are no review comments, so I have no feedback to provide.
Kunchd
left a comment
There was a problem hiding this comment.
Thanks for the fix! I'm a little confused on the exact desired behavior that we want here, so I left a few questions.
| // the inconsistent objects before the failure propagates. Applies to both | ||
| // successful and application-error completions: retries must reproduce the | ||
| // same object count, and app-error replays often report fewer IDs. | ||
| if (FailStreamingGeneratorReplayIfInconsistent(task_id, reply)) { |
There was a problem hiding this comment.
If FailStreamingGeneratorReplayIfInconsistent returns true, it means one of the following:
- An error (application or system level) has caused the generator to terminate before it could reach the same point as the first execution. In this case, should we return the actual error that caused the issue instead of
STREAMING_GENERATOR_REPLAY_INCONSISTENT? - The generator was non-deterministic and returned a different number of results. In this case, we should return
STREAMING_GENERATOR_REPLAY_INCONSISTENT.
There was a problem hiding this comment.
Here we only deal with application errors. System-level failures are handled via FailPendingTask, which doesn't have the FailStreamingGeneratorReplayIfInconsistent check.
The actual application error should still be observed by users since an application error will be treated as a normal yield. For example, if the user has ref1 and ref2 originated from a streaming generator on hand but the actual objects are lost, and the reconstructed streaming generator fails with an Exception1 before yielding anything. Then ray.get(ref1) will raise Exception1 and ray.get(reg2) will raise STREAMING_GENERATOR_REPLAY_INCONSISTENT
There was a problem hiding this comment.
Is the application error is non-retriable, I don't think we actually report the application error as a generator yield (see https://github.com/ray-project/ray/blob/master/python/ray/_raylet.pyx#L1911-L1925). Instead the error is stored as a return value: https://github.com/ray-project/ray/blob/master/python/ray/_raylet.pyx#L2411-L2420. In this case, wouldn't we be missing this error if we don't do what we did in the past and explicitly populate the streaming generator with the error?
Also, maybe I'm missing something, but I don't know if FailPendingTask is guaranteed to trigger on system failures. It seems like we only report a non-ok status for system errors if the output was not written (see https://github.com/ray-project/ray/blob/master/src/ray/core_worker/task_execution/task_receiver.cc#L123-L133). I'm not sure if it's possible for the output to be correctly written, but we still receive a system error afterwards.
There was a problem hiding this comment.
If the application error is non-retriable, we do report it as a yield. ray.get(gen.completed()) will be None though. The user can only get the error via ray.get(next(gen)).
If the application error is retriable, the expectation is that we never report it as a yield. The user can only get the error via ray.get(gen.completed()) after the retries are exhausted.
In this case, wouldn't we be missing this error if we don't do what we did in the past and explicitly populate the streaming generator with the error?
We will only miss an application error if it meets both of the following:
- It is retriable so we don't report it as a yield.
- The streaming generator has succeeded once so it can't be injected into
gen.completed().
At this point, I think we still shouldn't make the retriable application error a yield. That is not aligned with the expectation of marking an error retriable. Users should still expect to only get the retriable error via ray.get(gen.completed()) after the retries are exhausted. This has nothing to do with the FailStreamingGeneratorReplayIfInconsistent.
Also, maybe I'm missing something, but I don't know if FailPendingTask is guaranteed to trigger on system failures. It seems like we only report a non-ok status for system errors if the output was not written (see https://github.com/ray-project/ray/blob/master/src/ray/core_worker/task_execution/task_receiver.cc#L123-L133). I'm not sure if it's possible for the output to be correctly written, but we still receive a system error afterwards.
I think we really don't care about the system errors in the path of task_receiver.cc#L123-L133 because when objects_valid is true for a streaming generator, it means the streaming generator has ended already by itself, not by the system error. So if it has inconsistent yields, then we should raise STREAMING_GENERATOR_REPLAY_INCONSISTENT.
…ator-retry-none Keep master's first-execution EOF error materialization, and drop the re-execution app-error path that copied return_objects(0) onto stream refs. Signed-off-by: Rueian Huang <rueiancsie@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 768956c. Configure here.
Kunchd
left a comment
There was a problem hiding this comment.
Thanks for clarifying! I left one last comment from my interpretation of what's expected of the test.
| app_error_reply.add_streaming_generator_return_ids(); | ||
| return_id_proto->set_object_id(spec.StreamingGeneratorReturnId(i).Binary()); | ||
| return_id_proto->set_is_plasma_object(true); | ||
| } |
There was a problem hiding this comment.
One final comment. From my walk through the code, the current invocations in the test doesn't fully replicate the states for a resubmission. When an error is reported, we should actually invoke HandleReportGeneratorItemReturns on them as well. And this should in turn cause the error to be added to plasma via put_in_local_plasma_callback and the assertion for plasma_put_error_types should not be false below.
Signed-off-by: Rueian Huang <rueiancsie@gmail.com>

Description
Previously, there was a special path in the streaming generator reconstruction:
If the retried streaming generator stops with an application exception, it will try to fill
return_objects(0)for all refs that have not yet been restored. Butreturn_objects(0)isNoneif the first streaming generator succeeds.The result is that, in the case of lineage reconstruction, we can see
ray.get(ref)unexpectedly returnNoneif the restarted streaming generator stops with an application exception.This PR removes the special path. We will still treat the exception as the recovered object for the corresponding ref, but if the restarted streaming generator produces a different number of yields than its first execution, the user will get a
StreamingGeneratorReplayInconsistentErrorexception on the followingray.getinstead.