-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-51966][PYTHON] Replace select.select() with select.poll() when running on POSIX os #53306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-51966][PYTHON] Replace select.select() with select.poll() when running on POSIX os #53306
Conversation
…osix On glibc based Linux systems select() can monitor only file descriptor numbers that are less than FD_SETSIZE (1024). This is an unreasonably low limit for many modern applications.
|
This is identical to #50774 (which was never reviewed and closed by the bot), but rebased against current |
|
@HyukjinKwon is that something you could maybe review (in combination with py4j/py4j#560)? We needed to implement this change to allow us to run 1000+ executors without running into |
|
Can we have an environment variable to fallback? |
|
@HyukjinKwon you can now use |
|
@HyukjinKwon @gaogaotiantian - I see you now merged #53388 Can you merge a similar change to |
|
See traceback: |
|
Could you make sure the CI pass? I think it’s the linter issue. Also it has some conflicts now. |
That would be my fault. I did not realize this PR exists when I fix the |
|
Just updated the branch - this should fix conflict and overall is similar to your changes in Removed the fallback - if we have one it should be for both As per your PR I updated |
|
Does this LGTM, @gaogaotiantian ? |
|
@gaogaotiantian let me know what you think re. latest commit - it will check for errors in both I also removed try/catch around: in Considering |
|
@gaogaotiantian happy to merge in this form? Let me know if ok and I can propagate similar changes to py4j/py4j#560. |
|
Yes I think similar changes should be done for py4j. |
| # Could be POLLERR or POLLNVAL (select would raise in this case). | ||
| raise PySparkRuntimeError(f"Polling error - event {event} on fd {fd}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this introduce any behavior change? What's the original behavior? Can we improve this error message to make it more user friendly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the original behavior is probably to raise a Python builtin exception. To be honest if we need to raise this exception the situation is pretty bad - it would be an unexpected networking issue. I don't think we have coverage here.
Anyway, because this is a Python exception raised from worker side, the driver will always see a PythonException. The traceback might be different - but this is already a super rare situation and I don't think users will be relying on this.
On the other hand, yes there's improvement room for exception message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original behaviour would be select() raising OSError in situations, where poll() events POLLERR/POLLNVAL raise PySparkRuntimeError.
Agree users shouldn't rely on that behaviour, so thought using PySparkRuntimeError here makes most sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allisonwang-db agree with @gaogaotiantian the exception here should be very rare and due to networking issues - can you think of a better worded error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the PySparkRuntimeError are user facing exceptions with proper error classes and actionable error messages. If this is a rare low level system issue, it's better to keep the original exception (OSError) error message. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I wouldn't make it OSError as this is not a system call error (why I went for RuntimeError instead).
But honestly not too fussed and happy to change it to whatever you or Spark committers find appropriate as your feel for the codebase is better than mine (thanks for reviewing it!).
My focus here is to merge the fix that removes dependency on select.select() as this is something we have been patching internally for quite some time now to allow our researchers to run with large number of executors. The problem is even worse if you load a lot of shared libs (for example via ctypes), which take fds <1024 so you can hit FD_SETSIZE with fewer than 1000 executors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like OSError either. I think the general rule is to make all known exceptions a spark error. We have the ability to add error class in the future. Again, on driver side this is just a PythonException - also no errorClass with it because it's not supported yet.
|
Merged to master. |
What changes were proposed in this pull request?
On glibc based Linux systems
select()can monitor only file descriptor numbers that are less thanFD_SETSIZE(1024).This is an unreasonably low limit for many modern applications.
This PR replaces
select.select()withselect.poll()when running on POSIX os.Why are the changes needed?
When running via
pysparkwe frequently observe:On POSIX systems
poll()should be used instead ofselect().Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing unit tests + we have been running this change (combined with py4j/py4j#560) on our YARN cluster (Linux) since April 2025.
Was this patch authored or co-authored using generative AI tooling?
No