Add a downward-only shuffle partition pass sized from consumer-stage shuffle input - #2129
Conversation
Adds the six user-overridable tuning-config entries that drive the new downward-only shuffle partition pass, plus the pure policy layer that turns raw consumer-stage shuffle input records into either an applied reduction or a typed no-op reason. The calculator is deliberately isolated from AutoTuner state so its arithmetic can be proven on its own: configuration is validated as one fail-closed unit, byte and rung math is overflow-safe, rung generation is guaranteed to progress, and a requirement no representable rung can cover skips rather than recommending a partition count that is too small. Also adds the shared raw record types, which distinguish an application that executed no shuffle from a provider that produced no analysis at all. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the analysis that the downward shuffle-partition pass consumes: for every executed non-broadcast shuffle exchange in a final SQL plan, it resolves the uncompressed 'data size' once and attributes it to each distinct downstream branch, then totals those branches per consumer stage. A reused exchange is one node with several outgoing edges, so reuse multiplicity is preserved. The producing side of an exchange is identified by walking one edge upstream rather than by comparing stage numbers, because an exchange's own stage assignment spans both the write and the read side. Two gaps that would otherwise let unsafe evidence through are closed: - SparkListenerSQLExecutionEnd's errorMessage is now recorded, so a completed SQL execution can be told apart from a failed one. - Task accumulables now carry their stage attempt, which lets GPU spill be attributed to the attempt that reported it instead of leaking from a failed attempt into the later successful one. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Runs the new policy after the normal job-level and cluster-level passes so it sees the effective shuffle partition recommendation, and wires the shuffle-stage input analysis through both the profiling and the qualification providers using the SQL plan analyzer each tool already built. Every upward decision the normal passes made is now recorded where it is applied, so the downward pass can preserve it exactly instead of trying to infer it. Before writing anything, the pass also checks the affected consumer stages for spill and skew, checks that Databricks automatic shuffle optimization is not still governing partitioning, and checks that every property it must write is actually writable -- so the shuffle and AQE partition properties can never end up disagreeing. Ordinary no-ops stay log-only. Only an applied reduction, invalid policy configuration, or evidence the analysis ran on but could not complete produces a user-facing comment. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds end-to-end assertions over existing GPU and CPU event logs so the metric-to-record path is proven on real plan graphs rather than only on constructed records: exact per-consumer-stage totals for a GPU join and a CPU query, a multi-branch CPU AQE stage whose real spill evidence reaches the record, a log with no terminal SQL end event, and a log with several stage attempts. Also proves the qualification provider hands back the SQL analyzer's own cached analysis instead of traversing the plans a second time, and fails closed when no analyzer is supplied. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Real event logs showed the consumer-stage mapping failing on two common shapes, which disabled the downward pass for 17 of 25 qualification applications in a representative run. An exchange branch can reach several stages at once: AQE splits one logical shuffle read across multiple query stages, and the single consuming operator is assigned to all of them. Requiring exactly one stage rejected every such branch. The full exchange size is now attributed to each of those stages instead. That deliberately overstates each split stage, since AQE divided the data between them, but overstating can only raise the partition requirement and make a reduction less likely -- understating it is the unsafe direction. Near the root of a plan the downstream nodes often carry no metrics and therefore no stage assignment, so walking the graph dead-ends and the topmost exchange looked unmappable. Its reading stage is recovered from its own assignment instead, which spans both sides of the shuffle, by removing the writing side. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generated partition rung ladder had no relationship to the cluster being recommended for. Anchored at 500, it recommended 500 partitions on a 125-executor by 16-core cluster for a job whose worst stage carried about 2 GB, leaving three quarters of the cluster idle for that stage. The candidate is now quantized to whole execution waves of the recommended cluster: take the larger of the slot count and the worst-stage requirement, then round up to the next whole multiple of the slot count. A slot count is the recommended executor count times a per-executor multiplier, selected by the new DOWNWARD_SHUFFLE_SLOT_BASIS entry so the GPU-concurrency alternative can be measured without a code change. The executor count is read from the recommendation map only, never through getPropertyValue, which falls back to source properties and would yield the source CPU executor count on Databricks; the cluster record is the fallback when no recommendation exists. The floor and rung-multiplier entries are retired. The minimum-reduction factor survives as an escape hatch but defaults to 1.0, since the wave quantum already prevents trivial reductions on realistic clusters. An unknown cluster shape is an ordinary logged no-op with no user comment, which is the common qualification case. The pass now ships disabled so that enabling wave-based sizing is an explicit per-run opt-in while it is being evaluated. No gate is loosened: the spill and skew check, the atomic property write, every upward-safety floor, and the v1 consumer-stage mapping are unchanged. A property's "was not set" comment is now emitted at most once, because the pass recommending a value for a key a later time would otherwise repeat it. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adopts the two application-wide gates the reference implementation of the v2 heuristic carries and this pass did not: a run that failed a stage, or that hit an out-of-memory failure, is not evidence to size a global partition reduction from, even when the consumer stages it analyzed look healthy on their own. The failed-stage signal rides on the shuffle-stage input analysis so both tools see it. The OOM signal is only available on the profiling provider, so it is an overridable hook that reports false elsewhere rather than a base-class call that would not compile. Also stops the pass introducing an AQE partition property the application never carried. In practice the AQE pass has already recommended one by the time this pass runs, so the guard is inert; skipping the write when a value does exist would leave the higher normal value in place and defeat the reduction. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…le-autotuning # Conflicts: # core/src/main/scala/com/nvidia/spark/rapids/tool/AppSummaryInfoBaseProvider.scala # core/src/main/scala/com/nvidia/spark/rapids/tool/tuning/QualAppSummaryInfoProvider.scala # core/src/test/scala/com/nvidia/spark/rapids/tool/tuning/BaseAutoTunerSuite.scala # core/src/test/scala/com/nvidia/spark/rapids/tool/tuning/ProfilingAutoTunerSuiteV2.scala # core/src/test/scala/com/nvidia/spark/rapids/tool/tuning/QualificationAutoTunerSuite.scala
CI flagged five lines over the 100-character limit and four files whose copyright year had not been refreshed after this branch modified them. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@rishic3 Could you please help reviewing this PR? |
| None | ||
| } | ||
| } | ||
| // 4. Every affected consumer stage must be free of skew and spill. |
There was a problem hiding this comment.
I think what we want here is "affected stages", where
affected_stages = [
s for s in shuffle-consuming stages
if observed_task_count[s] > downward_suggestion
]
E.g., suppose we are suggesting 400, and stage A had 4,000 tasks with no spill, while stage B had 200 (post-AQE) tasks and spilled. Only A would be affected by the recommendation and B should not block.
There was a problem hiding this comment.
B's 200 is post AQE. The candidate sets the partition count before AQE coalesces, so comparing it to a count from after mixes two different things. B's 200 tasks may have come from 8000 partitions. Drop it to 400 and AQE has coarser input to work with, so B is still affected.
With coalescing off, numTasks is the fan-out and your filter is exactly right. So rather than drop the idea, I'd make the affected-stage filter depend on whether coalescing is on. Does that get you what you want? Follow-up with its own tests, since it loosens a gate.
There was a problem hiding this comment.
That makes sense, but IMO I still think we should consider the filter as I described. Maybe the right name for it is "constrained stages", i.e. stages that are actually constrained below their previously observed post-AQE tasks. In my example we are still giving enough nominal partitions to stage B to reproduce 200 tasks, so it isn't constrained. The counterfactual is that if B wasn't spilling, a seemingly "safe" reduction from 8,000 to 400 could now make B spill if AQE now coalesces more, so there is always this coalescing risk.
I'm fine with deferring after we do more testing, or starting with the 'coalescing is on' dependence. I am just coming from the fact that the example I described was literally describing a real job we saw that improved after we applied the heuristic, despite the spill in stage B.
There was a problem hiding this comment.
I think we should be proceed with more experimentation on this. Added as a follow up in #2133.
…le-autotuning Brings in the cache-derived maxPartitionBytes work (NVIDIA#2124). Two conflicts with this branch, both in the AutoTuner test scaffolding: getMockInfoProvider's constructor call gained maxFileScanInputOverride on dev and shuffleStageInputAnalysis here. Both are kept, ordered to match the AppInfoProviderMockTest signature the merge already produced. FileScanInputMetricsSuite's TestProfilingProvider is built with a null ApplicationInfo, so the downward pass added on this branch dereferenced null when it read the shuffle-stage input analysis through it. The stub now overrides that accessor to report no analysis, matching how it already stubs getClassPathEntries for the same reason. Only the test scaffold is affected: the field is genuinely present in production. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes DOWNWARD_SHUFFLE_SLOT_BASIS. The slot count is now always the recommended executor count times coresPerExecutor. GPU task concurrency is auto-tuned by recent plugin versions, and sizing a wave against it would badly under-use a cluster running mixed CPU and GPU stages, so the alternative the entry existed to measure is not one we want to select at runtime. Measuring it on paired runs now needs a code change. Removes DOWNWARD_SHUFFLE_MIN_REDUCTION_FACTOR. It already defaulted to 1.0, so this changes no behaviour. At 1.0 it is inert, and any other value is an arbitrary discontinuity with no principled way to set it; wave quantization is the only size gate worth having. The tests that covered the threshold now pin the opposite property, so a size floor cannot reappear unnoticed. Fixes the spill gate. One filter fed both the byte totals and the spill evidence, so a task that spilled and then failed was invisible to the gate that exists to see exactly that. Totals still require a successful task; the gate now only excludes speculative duplicates. The predicates moved onto the companion object so the asymmetry between them is directly testable. The shuffle-stage input analysis is no longer built when the pass is disabled. It walks every SQL plan, and the pass ships off, so that was the common path paying for an analysis nobody read. Also clamps each partition property against its own current value rather than the effective maximum across them. This is an invariant guard, not a fix: recommendAQEProperties levels the two before this pass runs, so the clamp is inert today and only matters if that ordering changes. The applied comment renders per-property values so it cannot misreport if it ever does. BranchTotals.add saturates instead of wrapping, because a wrapped total becomes small and would understate the requirement. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DOWNWARD_SHUFFLE_ENABLED's description now says what has to be settled before it defaults to true. It is not a count of validated applications: on a large workload the hand-tuned value, the ColumnarExchange bound and this pass at a 1 GiB target disagree, and this pass asks for the most partitions of the three. That reconciliation is the gate, and leaving it as tribal knowledge invites the flag being flipped for the wrong reason. The two analyzer TODOs now reference NVIDIA#2133, which describes both: sizing a CPU exchange inside a GPU run, blocked on the input-size factor being application-level, and apportioning an AQE-split exchange by per-stage shuffle read metrics rather than duplicating it. Both are conservative in the safe direction, so neither blocks shipping the pass enabled, which is why they are tracked apart from NVIDIA#2128. Signed-off-by: Partho Sarthi <psarthi@nvidia.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| None | ||
| } | ||
| } | ||
| // 4. Every affected consumer stage must be free of skew and spill. |
There was a problem hiding this comment.
That makes sense, but IMO I still think we should consider the filter as I described. Maybe the right name for it is "constrained stages", i.e. stages that are actually constrained below their previously observed post-AQE tasks. In my example we are still giving enough nominal partitions to stage B to reproduce 200 tasks, so it isn't constrained. The counterfactual is that if B wasn't spilling, a seemingly "safe" reduction from 8,000 to 400 could now make B spill if AQE now coalesces more, so there is always this coalescing risk.
I'm fine with deferring after we do more testing, or starting with the 'coalescing is on' dependence. I am just coming from the fact that the example I described was literally describing a real job we saw that improved after we applied the heuristic, despite the spill in stage B.
sayedbilalbari
left a comment
There was a problem hiding this comment.
Thanks @parthosa , LGTM !
Closes #2128 (partially — see Scope)
What this adds
A shuffle-input analysis and partition-sizing pass, running after the job-level and cluster-level recommendations so it sees the value those produced. It sums the uncompressed
data sizeof every shuffle branch entering each consumer stage — a join's inputs add up, a reused exchange counts twice — divides by a target size, takes the worst stage, and rounds up to whole cluster waves.It holds the existing recommendation on any of:
Against the 4.08 TB shuffle in #2128 it computes 3840 — within 1% of the 3800 that succeeded, from the first run's metrics rather than after three attempts.
Scope: applies downward only
It applies only when the result lowers the current value. On that workload the existing rules undershoot, so it computes the right number and rejects it as not-downward — it did not apply on any run of that workload. Fixing that needs the two changes tracked in #2128, so this ships disabled. What lands here is the measurement layer: correct, tested against real event logs, prerequisite for either direction.
Key design decisions
SQLAccumProfileResults.totalsums across stages and inflates any application that retried one.SparkListenerSQLExecutionEnd.errorMessageis recorded so a completed SQL execution can be told from a failed one, and task accumulables carry their stage attempt so GPU spill isn't attributed across attempts.Configuration
DOWNWARD_SHUFFLE_ENABLED=false, opted into per run via--tuning_configs; five knobs intuningConfigs.yaml.Testing
New
DownwardShufflePartitionsSuiteandShuffleStageInputMetricsSuite(real event logs: multi-branch joins, exchange reuse, AQE-split reads, missing metrics, unfinished SQL, attempt selection), plus AutoTuner coverage for the applied path and each gate. CImvn verifyis green across the matrix — Java 8/11/17, Spark 321/357, Scala 2.12 and 2.13.