Add runtime setters for MULTI_POLL reactor count, dispatch, and request ceiling - #1049
Conversation
madsbk
left a comment
There was a problem hiding this comment.
Overall looks good.
- update the stale sentence at
runtime_settings.rst:82 - add missing docs for three newly user-facing settings
| * Takes effect only if set before the `MULTI_POLL` reactor pool is first used (the pool, like | ||
| * the default thread pool, is created lazily on first use and is never rebuilt). |
There was a problem hiding this comment.
nit: Like Mads, I think we should throw if this set call would not take effect.
| * Takes effect only if set before the `MULTI_POLL` reactor pool is first used (the pool, like | ||
| * the default thread pool, is created lazily on first use and is never rebuilt). |
| * Takes effect only if set before the `MULTI_POLL` reactor pool is first used (the pool, like | ||
| * the default thread pool, is created lazily on first use and is never rebuilt). |
| * runtime, overriding `KVIKIO_REMOTE_IO_NUM_REACTORS`. | ||
| * | ||
| * Takes effect only if set before the `MULTI_POLL` reactor pool is first used (the pool, like | ||
| * the default thread pool, is created lazily on first use and is never rebuilt). |
There was a problem hiding this comment.
I agree with the suggestion. Just want to point out that for the BS thread pool used for local I/O and easy backend, we do support changing the number of threads midway at runtime, which would block the calling thread, drain the task queue, destroy the worker threads, and then recreate a new set of worker threads. So I think the phrasing "like the default thread pool" should be removed from the doc string here.
madsbk
left a comment
There was a problem hiding this comment.
Since we cannot reset the pool size, the defaults context manager is now broken:
with kvikio.defaults.set("remote_io_num_reactors", 8):
remote_file.pread(...)It fails when trying to reset remote_io_num_reactors on exit.
Can we just not support context managers for these settings? I did that in 7373370 |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/merge |
Defaults cudf-polars to the kvikio MULTI_POLL backend with new defaults for `kvikio_task_size`, `kvikio_bounce_buffer_bytes`, `kvikio_reactor_count`, `kvikio_reactor_dispatch`, and `kvikio_request_ceiling`. These are the best defaults we've found based on benchmarking on a g7e.8xlarge instance. | Setting | MULTI_POLL default | EASY_THREADPOOL default | |---|---|---| | `kvikio_remote_io_backend` | `MULTI_POLL` | `EASY_THREADPOOL` | | `kvikio_task_size` | 16 MiB | 64 MiB | | `kvikio_bounce_buffer_bytes` | 16 MiB | 16 MiB | | `kvikio_nthreads` | unset (defers to kvikio's own default) | 256 | | `kvikio_reactor_count` | 24 | not used | | `kvikio_reactor_dispatch` | `PER_CHUNK` | not used | | `kvikio_request_ceiling` | 256 | not used | Notes: - MULTI_POLL doesn't use `kvikio_nthreads` for remote I/O, it uses the reactor threads instead, so we don't force that pool to 256 threads under MULTI_POLL. - EASY_THREADPOOL doesn't use the reactor settings at all. - `kvikio_bounce_buffer_bytes` applies to both backends, it's not MULTI_POLL-specific. - Depends on rapidsai/kvikio#1049 - Depends on #23683 Authors: - Matthew Murray (https://github.com/Matt711) - Peter Andreas Entschev (https://github.com/pentschev) Approvers: - Peter Andreas Entschev (https://github.com/pentschev) URL: #23839
Adds setters for
remote_io_num_reactors,remote_io_reactor_dispatch, andremote_io_max_concurrent_requestsMULTI_POLLbackend NVIDIA/cudf#23839