[Core][Streaming Generator] Fix the perf regression from a serve handle bug fix #38171 #38280
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 tasks
rkooo567
changed the title
rkooo567
commented
Aug 10, 2023
| @@ -1012,7 +1013,7 @@ cdef class StreamingGeneratorExecutionContext: | |||
| return self | |||
|
|
|||
|
|
|||
| cpdef report_streaming_generator_output( | |||
| cdef report_streaming_generator_output( | |||
There was a problem hiding this comment.
It doesn't have to be cpdef, and cdef is faster
| except StopIteration: | ||
| return True | ||
| except Exception as e: | ||
| if isinstance(output_or_exception, Exception): |
There was a problem hiding this comment.
if I raise an exception again like before, it caused the circular dependencies issues.
| # the output (which has nogil). | ||
| done = await loop.run_in_executor( | ||
| worker.core_worker.get_thread_pool_for_async_event_loop(), | ||
| report_streaming_generator_output, |
There was a problem hiding this comment.
this is the core fix (dispatching the cpp code to another thread)
python/ray/_raylet.pyx
Outdated
| def get_thread_pool_for_async_event_loop(self): | ||
| if self.thread_pool_for_async_event_loop is None: | ||
| self.thread_pool_for_async_event_loop = ThreadPoolExecutor( | ||
| max_workers=int(os.getenv("RAY_ASYNC_THREAD_POOL_SIZE", 1))) |
There was a problem hiding this comment.
Q: maybe I should always use 1 thread? I used multiple for some exploration. I think when there are lots of async tasks, it may have some perf benefits
There was a problem hiding this comment.
So it's thread safe to use multiple threads? Before it's done in main thread so it's 1.
There was a problem hiding this comment.
Hmm actually a good point. I think it should be thread-safe, but not 100% sure. Maybe I will just use 1 thread for now
jjyao
approved these changes
Aug 12, 2023
|
FAiled tests seem unrelated |
NripeshN
pushed a commit
to NripeshN/ray
that referenced
this pull request
Aug 15, 2023
…le bug fix ray-project#38171 (ray-project#38280) Before ray-project#37972, we ran the reporting & serilization output (in cpp) in a main thread while all the async actor tasks run in an async thread. However, after the PR, we now run both of them in an async thread. This caused regression when there are decently large size (200~2KB) generator workloads (Aviary) because the serialization code was running with nogil. It means we could utilize real multi-threading because serialization code runs in a main thread, and async actor code runs in an async thread. This PR fixes the issue by dispatching a cpp code (reporting & serialization) to a separate thread again. I also found when I used threadPoolExecutor, there were some circular dependencies issues where it leaks objects when exceptions happen. I realized this was due to the fact that Python exception captures the local references (thus there were some circular references). I refactored some part of code to avoid this from happening and added an unit test for that. Signed-off-by: NripeshN <[email protected]>
arvind-chandra
pushed a commit
to lmco/ray
that referenced
this pull request
Aug 31, 2023
…le bug fix ray-project#38171 (ray-project#38280) Before ray-project#37972, we ran the reporting & serilization output (in cpp) in a main thread while all the async actor tasks run in an async thread. However, after the PR, we now run both of them in an async thread. This caused regression when there are decently large size (200~2KB) generator workloads (Aviary) because the serialization code was running with nogil. It means we could utilize real multi-threading because serialization code runs in a main thread, and async actor code runs in an async thread. This PR fixes the issue by dispatching a cpp code (reporting & serialization) to a separate thread again. I also found when I used threadPoolExecutor, there were some circular dependencies issues where it leaks objects when exceptions happen. I realized this was due to the fact that Python exception captures the local references (thus there were some circular references). I refactored some part of code to avoid this from happening and added an unit test for that. Signed-off-by: e428265 <[email protected]>
vymao
pushed a commit
to vymao/ray
that referenced
this pull request
Oct 11, 2023
…le bug fix ray-project#38171 (ray-project#38280) Before ray-project#37972, we ran the reporting & serilization output (in cpp) in a main thread while all the async actor tasks run in an async thread. However, after the PR, we now run both of them in an async thread. This caused regression when there are decently large size (200~2KB) generator workloads (Aviary) because the serialization code was running with nogil. It means we could utilize real multi-threading because serialization code runs in a main thread, and async actor code runs in an async thread. This PR fixes the issue by dispatching a cpp code (reporting & serialization) to a separate thread again. I also found when I used threadPoolExecutor, there were some circular dependencies issues where it leaks objects when exceptions happen. I realized this was due to the fact that Python exception captures the local references (thus there were some circular references). I refactored some part of code to avoid this from happening and added an unit test for that. Signed-off-by: Victor <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Before #37972, we ran the reporting & serilization output (in cpp) in a main thread while all the async actor tasks run in an async thread. However, after the PR, we now run both of them in an async thread.
This caused regression when there are decently large size (200~2KB) generator workloads (Aviary) because the serialization code was running with nogil. It means we could utilize real multi-threading because serialization code runs in a main thread, and async actor code runs in an async thread.
This PR fixes the issue by dispatching a cpp code (reporting & serialization) to a separate thread again. I also found when I used threadPoolExecutor, there were some circular dependencies issues where it leaks objects when exceptions happen. I realized this was due to the fact that Python exception captures the local references (thus there were some circular references). I refactored some part of code to avoid this from happening and added an unit test for that.
Related issue number
Closes #38163
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.