[BC-Breaking][pipeline] default use_thread_output_queue to True - #1574
Open
moto-meta wants to merge 2 commits into
Open
[BC-Breaking][pipeline] default use_thread_output_queue to True#1574moto-meta wants to merge 2 commits into
moto-meta wants to merge 2 commits into
Conversation
Summary: ## Problem With `use_thread_output_queue=True`, the sink's output queue routes through `loop.run_in_executor(...)`, perturbing event-loop scheduling during shutdown. A failing stage raises inside `_queue_stage_hook`, which does `await queue.put(_EOF)` (a suspension point) *before* re-raising. When the downstream sink completes first, `_run_pipeline_coroutines` → `_cancel_orphaned` cancels the still-suspended upstream task before it re-raises; the `CancelledError` masks the real error and `PipelineFailure` was dropped on ~50% of runs. ## Root-cause fix Instead of recovering the masked error after the fact, prevent the masking: `_cancel_recursive` now skips cancelling a task that has already failed (detected via the terminal exception `_queue_stage_hook` records before its EOF handoff). The failed task resumes and re-raises naturally; its upstream producers are still cancelled. `_gather_error` reverts to its simple form (`done` + not-cancelled + `isinstance Exception`). The `use_thread_output_queue=False` path is unaffected — the skip is a no-op unless a stage actually failed. [Session trajectory link](https://www.internalfb.com/intern/devai/devmate/inspector/?id=1b804c2a-bac7-49ce-ac13-ebcb631cd954) Differential Revision: D110018369
Summary: Make the thread-backed sink output queue the default across the public pipeline build entry points by flipping the default of `use_thread_output_queue` from `False` to `True`. This affects `build_pipeline`, `PipelineBuilder.build`, `run_pipeline_in_subprocess`, and `run_pipeline_in_subinterpreter` (plus the internal forwarders that thread the argument through). When enabled, the sink hands the final batch from the background event loop to the foreground consumer thread via a `queue.Queue`-backed queue instead of `asyncio.run_coroutine_threadsafe`, cutting per-batch handoff latency from ~200-400us to ~10us. This changes a public default value in a backward-incompatible way, so the title is tagged `[BC-breaking]` and the affected docstrings carry a `.. versionchanged:: 0.6.0` directive. `profile_pipeline` reads the sink output queue's occupancy/lap stats, which only the asyncio stats queue collects, so it now builds its internal pipelines with `use_thread_output_queue=False` explicitly. Known issue — must be resolved before landing: pipeline-level exception propagation (e.g. `PipelineFailure` raised when a stage exceeds its failure threshold, or any raising stage) is currently unreliable (~50%) when the thread-backed output queue is used. This is a pre-existing race in the thread-output-queue path that is merely exposed by making it the default; it is documented and tracked separately. See the test plan for how to validate the fix. Differential Revision: D109953066
meta-cla
Bot
added
the
CLA Signed
Contributor
|
@moto-meta has exported this pull request. If you are a Meta employee, you can view the originating Diff in D109953066. |
mthrok
changed the title
mthrok
changed the title
mthrok
changed the title
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
2 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.
Summary:
Make the thread-backed sink output queue the default across the public pipeline build entry points by flipping the default of
use_thread_output_queuefromFalsetoTrue. This affectsbuild_pipeline,PipelineBuilder.build,run_pipeline_in_subprocess, andrun_pipeline_in_subinterpreter(plus the internal forwarders that thread the argument through).When enabled, the sink hands the final batch from the background event loop to the foreground consumer thread via a
queue.Queue-backed queue instead ofasyncio.run_coroutine_threadsafe, cutting per-batch handoff latency from ~200-400us to ~10us.This changes a public default value in a backward-incompatible way, so the title is tagged
[BC-breaking]and the affected docstrings carry a.. versionchanged:: 0.6.0directive.profile_pipelinereads the sink output queue's occupancy/lap stats, which only the asyncio stats queue collects, so it now builds its internal pipelines withuse_thread_output_queue=Falseexplicitly.Known issue — must be resolved before landing: pipeline-level exception propagation (e.g.
PipelineFailureraised when a stage exceeds its failure threshold, or any raising stage) is currently unreliable (~50%) when the thread-backed output queue is used. This is a pre-existing race in the thread-output-queue path that is merely exposed by making it the default; it is documented and tracked separately. See the test plan for how to validate the fix.Differential Revision: D109953066