NVIDIA / NVIDIA/cudf

narrow `UNSPECIFIED` to fields that actually need to defer

Open
#23,744 1 comment 0 reactions 1 assignee Claimed by @Matt711 View on GitHub
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

Part of #23740.

`_opt()` has no `default` parameter today, every `StreamingOptions` field becomes
`UNSPECIFIED` unconditionally when nothing explicit or env-driven applies. Most fields
don't need that: `StreamingExecutor`/`ParquetOptions`/etc. already assert a concrete
Python default for them. Only fields with a real reason to defer should keep it.

Audited all 29 `StreamingOptions` fields against their actual downstream consumers.
13 need `UNSPECIFIED` (deferred to rapidsmpf, which owns the real default); 16 don't.

| Field | Category | Concrete default (owner) | Needs `Unspecified`? |
|---|---|---|---|
| `num_streaming_threads` | rapidsmpf | (rapidsmpf C++) | Yes, deferred to rapidsmpf |
| `num_streams` | rapidsmpf | (rapidsmpf C++) | Yes |
| `log` | rapidsmpf | (rapidsmpf C++) | Yes |
| `statistics` | rapidsmpf | (rapidsmpf C++) | Yes |
| `memory_reserve_timeout` | rapidsmpf | (rapidsmpf C++) | Yes |
| `allow_overbooking_by_default` | rapidsmpf | (rapidsmpf C++) | Yes |
| `pinned_memory` | rapidsmpf | (rapidsmpf C++) | Yes |
| `pinned_initial_pool_size` | rapidsmpf | (rapidsmpf C++) | Yes |
| `pinned_max_pool_size` | rapidsmpf | (rapidsmpf C++) | Yes |
| `spill_device_limit` | rapidsmpf | (rapidsmpf C++) | Yes |
| `periodic_spill_check` | rapidsmpf | (rapidsmpf C++) | Yes |
| `unbounded_file_read_cache` | rapidsmpf | (rapidsmpf C++) | Yes |
| `ucxx_progress_mode` | rapidsmpf | (rapidsmpf C++) | Yes |
| `num_py_executors` | needed before #23745 can construct the executor | `8` | No, must resolve immediately |
| `kvikio_nthreads` | needed before #23745 can construct the executor | `resolve_kvikio_nthreads({})` | No, must resolve immediately |
| `hardware_binding` | needed before #23745 can construct the executor | `HardwareBindingPolicy()` | No, must resolve immediately |
| `memory_resource_config` | needed before #23745 can construct the executor | `MemoryResourceConfig.default()` | No, must resolve immediately |
| `quent_context` | needed before #23745 can construct the executor | `None` | No, must resolve immediately |
| `allow_gpu_sharing` | needed before #23745 can construct the executor | `False` | No, must resolve immediately |
| `dynamic_planning` | broken resolution | `DynamicPlanningOptions()` | No, once `_opt()` gets a real `env_var`/`default` (currently `Unspecified` only as a side effect of the ad hoc block in `from_polars_engine`) |
| `join_filter_pushdown` | broken resolution | `JoinFilterPushdownOptions()` | No, once fixed, same reason as above |
| `max_concurrent_io_tasks` | no reason | `2` | No |
| `fallback_mode` | no reason | `"warn"` | No |
| `max_rows_per_partition` | no reason | `1_000_000` | No |
| `broadcast_limit` | no reason | `0` (real "auto" sentinel, not `Unspecified`) | No |
| `target_partition_size` | no reason | `0`, same "auto" mechanism | No |
| `sink_to_directory` | no reason | `None` | No |
| `raise_on_fail` | no reason | `False` | No |
| `parquet_options` | no reason | `ParquetOptions()` | No |

13 need it (rapidsmpf-owned, genuinely deferred to a system outside cudf-polars). 16
don't (6 fields listed in #23745 that must resolve immediately, before `StreamingExecutor`
exists; 2 that only look like they need it because their env var resolution is currently
split into an ad hoc block instead of `_opt()`; and 8 with no structural reason at all).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.