[RPC] Fix tuning on macOS and Windows (#15771)#16357
Merged
Johnson9009 merged 2 commits intoapache:mainfrom Jan 12, 2024
Merged
[RPC] Fix tuning on macOS and Windows (#15771)#16357Johnson9009 merged 2 commits intoapache:mainfrom
Johnson9009 merged 2 commits intoapache:mainfrom
Conversation
gmeeker
force-pushed
the
bugfix/autotune
branch
from
3197d51 to
bf41f88
Compare
Fix regression in (apache#15187) when multiprocessing start method is not 'fork', which prevented tuning from working. This affects macOS and Windows. Also in python 3.14 the default start method will be 'spawn'.
gmeeker
force-pushed
the
bugfix/autotune
branch
from
bf41f88 to
6e61c39
Compare
Member
|
CC @Johnson9009 |
Johnson9009
reviewed
Jan 11, 2024
Contributor
Johnson9009
left a comment
Johnson9009
left a comment
There was a problem hiding this comment.
sorry for late review, threre are some questions.
| """Server loop""" | ||
| sockfd = sock.fileno() | ||
| temp = _server_env(load_library, work_path) | ||
| _ffi_api.ServerLoop(sockfd) |
Contributor
There was a problem hiding this comment.
can the sockfd be inline?
|
|
||
|
|
||
| def _serve_loop(sock, load_library, work_path=None): | ||
| """Server loop""" |
Contributor
There was a problem hiding this comment.
del the meaningless comment because it give no more information beyond the function name, or write a meaning one.
Johnson9009
requested changes
Jan 11, 2024
Contributor
Author
|
The changes from the original code are now minimal, just removing the closure. |
Johnson9009
approved these changes
Jan 11, 2024
Contributor
Johnson9009
left a comment
Johnson9009
left a comment
There was a problem hiding this comment.
Thanks, look good to me.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fix regression in (#15187) when multiprocessing start method is not 'fork', which prevented tuning from working. This affects macOS and Windows. Also in python 3.14 the default start method will be 'spawn'.