[core] Suppress harmless ObjectRefStreamEndOfStreamError when using asyncio #37062
Conversation
Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>
python/ray/_raylet.pyx
Outdated
| # NOTE(swang): Hack to avoid asyncio warnings about not retrieving the | ||
| # exception. The exception will get returned to the user through the | ||
| # below code. | ||
| for result in ready: | ||
| exc = result.exception() |
There was a problem hiding this comment.
This will suppress any form of unexpected exception. I would suggest to explicitly wrap the ref in a task instead and ignore the specific exception type:
async def peek_stream_wrapper(ref: "ImNotSureWhatTypeThisIs"):
try:
await ref
except ObjectRefStreamEndOfStreamError:
pass # Expected.
peek_stream_task = asyncio.create_task(
peek_stream_wrapper(core_worker.peek_object_ref_stream(self._generator_ref))
)
ready, unready = await asyncio.wait([peek_stream_task], timeout=timeout_s)asyncio.wait does this under the hood anyways so there shouldn't be additional overhead
There was a problem hiding this comment.
Got it, thanks!
There was a problem hiding this comment.
Hmm actually I don't think we should do this. This part of the code is only meant to wait for the ref to become ready; we just don't have a way to do this for asyncio right now without also fetching the data.
The expected use case is that the user will then do something with the returned ref once it's ready. I believe Ray also warns if the ref has an exception and is never read, so we'll be warning twice if we don't suppress the asyncio error.
There was a problem hiding this comment.
Ah I see so this could raise other exceptions aside from ObjectRefStreamEndOfStreamError?
Ideally we would "whitelist" the expected exceptions here -- maybe a conservative way to do it would be except RayError? I'm just a little uneasy about suppressing all exceptions, there may be some corner case that raises an unexpected exception and identifying/debugging it will be much easier if the exception is logged.
Ultimately up to you though, if you don't think this is an issue then suppressing them all is fine. Though it's probably still slightly less hacky to wrap it in a task and catch bare Exception.
There was a problem hiding this comment.
Yeah, to clarify, the reasoning for suppressing all errors is that at this point, the user hasn't actually tried to receive the error yet. This function is supposed to return the ObjectRef that the user can await, and then at point, asyncio will warn if the user doesn't retrieve the exception.
So either we don't suppress the error here, and the user will always see an extra warning, or we do suppress the error here, and then asyncio or Ray will warn if the user doesn't do anything with the ObjectRef.
There was a problem hiding this comment.
Got it. Thanks for clarifying.
stephanie-wang
added
@author-action-required
Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>
Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>
stephanie-wang
added
the
@author-action-required
| # The exception will get returned (or warned) to the user once they | ||
| # actually await the ref. | ||
| try: | ||
| await ref |
There was a problem hiding this comment.
Should we use exception() instead here? https://docs.python.org/3/library/asyncio-task.html#asyncio.Task.exception
This way, we don't need to await on actual value if there's no exception
There was a problem hiding this comment.
asyncio.wait will await on the actual value anyway right now.
| ref = await generator._next_async(timeout_s=0) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
are you planning to delete this, or is it temporary?
There was a problem hiding this comment.
These tests failed afterwards. I took a look and I don't think we really need them (they're testing internal behavior).
…syncio (ray-project#37062) The last ref returned by a streaming generator is a sentinel ObjectRef that contains the end-of-stream error. This suppresses an error from asyncio that the exception is never retrieved (which is expected). Related issue number Closes ray-project#36956. --------- Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>
…syncio (#37062) (#37200) The last ref returned by a streaming generator is a sentinel ObjectRef that contains the end-of-stream error. This suppresses an error from asyncio that the exception is never retrieved (which is expected). Related issue number Closes #36956. --------- Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>
…syncio (ray-project#37062) The last ref returned by a streaming generator is a sentinel ObjectRef that contains the end-of-stream error. This suppresses an error from asyncio that the exception is never retrieved (which is expected). Related issue number Closes ray-project#36956. --------- Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu> Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
Why are these changes needed?
The last ref returned by a streaming generator is a sentinel ObjectRef that contains the end-of-stream error. This suppresses an error from asyncio that the exception is never retrieved (which is expected).
Related issue number
Closes #36956.
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.