Skip to content
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
1bd2d66
Enable _reset to update quent_context; fast quent integration tests
TomAugspurger Oct 16, 2026
a8d2dd4
Deduplicate quent open/close across engine _reset methods
TomAugspurger Oct 16, 2026
33820dd
Use fallback_mode=silent in quent integration tests
TomAugspurger Oct 16, 2026
6f302e2
Clarify why engine_with_quent_context uses _unconfigured_engine directly
TomAugspurger Oct 16, 2026
4a3de24
Preserve allow_gpu_sharing across _reset calls
TomAugspurger Oct 16, 2026
c5a5234
Fix Ray quent test: pass full engine_options in the in-test _reset
TomAugspurger Oct 16, 2026
9da5f3f
Fix stale Quent closure capture in Ray/Dask shutdown callbacks
TomAugspurger Oct 16, 2026
6243197
Merge remote-tracking branch 'upstream/main' into tom/fast-quent-tests
TomAugspurger Oct 19, 2026
ec21c4c
Merge remote-tracking branch 'upstream/main' into tom/fast-quent-tests
TomAugspurger Oct 19, 2026
625885f
Ensure shutdown in dask/ray engine
TomAugspurger Oct 19, 2026
257cd05
bigger refactor
TomAugspurger Oct 19, 2026
154e2bd
coverage
TomAugspurger Oct 19, 2026
557bbd6
Move the test
TomAugspurger Oct 19, 2026
436e0fa
sync quent context
TomAugspurger Oct 20, 2026
b2b0a4f
set_quent
TomAugspurger Oct 20, 2026
e2599f8
Merge remote-tracking branch 'upstream/main' into tom/fast-quent-tests
TomAugspurger Oct 20, 2026
9dab31b
ensure closed
TomAugspurger Oct 20, 2026
06b884d
wip
TomAugspurger Oct 20, 2026
ddfd1d4
wip
TomAugspurger Oct 20, 2026
3d623f5
fixup
TomAugspurger Oct 20, 2026
019c4a2
Merge remote-tracking branch 'upstream/main' into tom/fast-quent-tests
TomAugspurger Oct 20, 2026
ce04869
Merge branch 'main' into tom/fast-quent-tests
TomAugspurger Oct 20, 2026
1427992
Merge remote-tracking branch 'upstream/main' into tom/fast-quent-tests
TomAugspurger Oct 21, 2026
2eff55f
docstrings
TomAugspurger Oct 21, 2026
5c1bda1
sync before set
TomAugspurger Oct 21, 2026
952af6f
Merge branch 'main' into tom/fast-quent-tests
TomAugspurger Oct 22, 2026
d1bada3
Merge remote-tracking branch 'upstream/main' into tom/fast-quent-tests
TomAugspurger Oct 23, 2026
74ea0e4
Trigger CI
TomAugspurger Oct 23, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions python/cudf_polars/cudf_polars/engine/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -539,12 +539,6 @@ def _reset(
existing_executor_options = self.config.get("executor_options", {})
if not isinstance(existing_executor_options, dict):
existing_executor_options = {}
existing_quent_context = existing_executor_options.get("quent_context")
if (
"quent_context" in executor_options
and executor_options["quent_context"] != existing_quent_context
):
raise ValueError("quent_context cannot be changed during reset")

_disallowed_exec = {"num_py_executors"} & executor_options.keys()
if _disallowed_exec:
Expand Down Expand Up @@ -589,6 +583,42 @@ def __exit__(self, *_: object) -> None:
"""Exit the context manager, calling :meth:`shutdown`."""
self.shutdown()

def _update_quent(
Comment thread
TomAugspurger marked this conversation as resolved.
Outdated
self,
new_quent_context: QuentConfig | None,
existing_quent_context: QuentConfig | None,
executor_options: dict[str, Any],
) -> None:
"""
Apply a ``quent_context`` change in-place.

Closes existing Quent runtimes, starts new ones for
``new_quent_context``, and patches ``executor_options["quent_context"]``
and ``_quent_output_root``. Subclasses implement :meth:`_quent_close`
and :meth:`_quent_open` for the backend-specific work.
"""
if new_quent_context != existing_quent_context:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questions for me to see if there are any implicit invariants:

  1. When would new_quent_context == existing_quent_context?
  2. Is a None existing_quent_context mostly passed when the __init__ happens?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding to @mroeschke's questions, from my reading (@TomAugspurger please correct me if I am wrong):

  • new == existing happens on most resets, since every backend's _reset defaults new to existing when quent_context isn't in executor_options. It also covers None == None at __init__ with Quent off.
  • existing is None is always the case at __init__ (hard-coded in dask.py:1128, ray.py:975, spmd.py:653), on a reset where Quent was off, and in SPMD after the early close (spmd.py:1024).

One thing that worries me: this compares the whole QuentConfig, including the per-query fields. Callers change those fields in engine.config between collects, e.g. the benchmark runner sets query_name per iteration (streaming/benchmarks/utils.py:1193). So a later _reset with the original config looks like a change, and Quent gets torn down and restarted.

The restart reuses the same engine_id, so the output directory ends up with two Engine lifecycles under one id. Toggling A → None → A does the same.

Could we compare only engine_id and output_root here, and raise if a closed engine_id is re-opened?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that worries me: this compares the whole QuentConfig, including the per-query fields. Callers change those fields in engine.config between collects, e.g. the benchmark runner sets query_name per iteration (streaming/benchmarks/utils.py:1193). So a later _reset with the original config looks like a change, and Quent gets torn down and restarted.

But (correct me if I'm wrong), regular users will never trigger a _reset, right? IIUC, _reset is only used by test code.

Regular usage, including our streaming benchmarks, will hit this code path when the engine is initially created, and they'll compare a QuentConfig with None and setup the Quent engine. But all subsequent collect calls will use the one engine and so use the one QuentConfig (which might have been mutated to update the query group / query name).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the only caller of _reset outside the engines is cudf_polars/testing/engine_utils.py. Regular use, including the benchmarks, only hits this at construction. Unlike the SPMD case, the worst outcome here is a duplicate Engine lifecycle in the trace output, not a hang. I'm fine leaving it as is.

if existing_quent_context is not None:
self._quent_close()
Comment thread
TomAugspurger marked this conversation as resolved.
Outdated
if new_quent_context is not None:
new_quent_context = self._quent_open(new_quent_context)
self._quent_output_root = (
new_quent_context.run_root if new_quent_context is not None else None
)
executor_options["quent_context"] = new_quent_context

def _quent_close(self) -> None:
"""Close all Quent runtimes for this engine (workers then controller)."""
raise NotImplementedError( # pragma: no cover
f"{type(self).__name__} does not implement _quent_close"
)

def _quent_open(self, quent_context: QuentConfig) -> QuentConfig:
"""Start fresh Quent runtimes and return the effective configuration."""
raise NotImplementedError( # pragma: no cover
f"{type(self).__name__} does not implement _quent_open"
)

def _run(self, func: Callable[..., T], *args: Any, **kwargs: Any) -> list[T]:
"""
Execute a function on all ranks.
Expand Down
Loading
Loading