Skip to content

fix: skip VW barrier execution for single-partition training - #2592

Merged
Rana Singh (ranadeepsingh) merged 1 commit into
microsoft:masterfrom
ranadeepsingh:copilot/ancient-1912-vw-barrier
Aug 7, 2026
Merged

fix: skip VW barrier execution for single-partition training#2592
Rana Singh (ranadeepsingh) merged 1 commit into
microsoft:masterfrom
ranadeepsingh:copilot/ancient-1912-vw-barrier

Conversation

@ranadeepsingh

Copy link
Copy Markdown
Collaborator

Summary

  • use Vowpal Wabbit barrier execution only when it is enabled and training has more than one partition
  • preserve the existing multi-partition barrier path and disabled-barrier path
  • add execution-path coverage for enabled single-partition, enabled multi-partition, and disabled multi-partition training

Validation

  • TDD red: the new one-partition test observed a BarrierTaskContext before the source guard
  • vw/testOnly com.microsoft.azure.synapse.ml.vw.VerifyVowpalWabbitClassifier -- -z skips
  • vw/testOnly com.microsoft.azure.synapse.ml.vw.VerifyVowpalWabbitClassifier -- -z libsvm
  • vw/compile and vw/Test/compile
  • scalastyle test:scalastyle

Compatibility

The production change is a source- and binary-compatible execution guard; no public API changes are introduced. This change is scoped only to Vowpal Wabbit and does not alter LightGBM.

Related

Recreates the still-relevant proposal from #1912 on current master without closing or modifying that original PR.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Hey Rana Singh (@ranadeepsingh) 👋!
Thank you so much for contributing to our repository 🙌.
Someone from SynapseML Team will be reviewing this pull request soon.

We use semantic commit messages to streamline the release process.
Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix.
This helps us to create release messages and credit you for your hard work!

Examples of commit messages with semantic prefixes:

  • fix: Fix LightGBM crashes with empty partitions
  • feat: Make HTTP on Spark back-offs configurable
  • docs: Update Spark Serving usage
  • build: Add codecov support
  • perf: improve LightGBM memory usage
  • refactor: make python code generation rely on classes
  • style: Remove nulls from CNTKModel
  • test: Add test coverage for CNTKModel

To test your commit locally, please follow our guild on building from source.
Check out the developer guide for additional guidance on testing your change.

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

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.

Pull request overview

This PR adjusts Vowpal Wabbit training to avoid Spark barrier execution when training runs with a single partition, while preserving the existing behavior for multi-partition training (both barrier-enabled and barrier-disabled). This improves compatibility with single-partition workloads by preventing unnecessary barrier scheduling.

Changes:

  • Guard barrier execution in VowpalWabbitBaseLearner so it only runs when barrier mode is enabled and the training DataFrame has more than one partition.
  • Add execution-path tests that verify barrier usage (enabled multi-partition), non-barrier usage (disabled multi-partition), and skipping barrier usage (enabled single-partition) via a tracking subclass and accumulator.
Show a summary per file
File Description
vw/src/main/scala/com/microsoft/azure/synapse/ml/vw/VowpalWabbitBaseLearner.scala Adds a partition-count guard to skip barrier execution for single-partition training.
vw/src/test/scala/com/microsoft/azure/synapse/ml/vw/VerifyVowpalWabbitClassifier.scala Adds tests that assert which execution path was used (barrier vs non-barrier) across partition and configuration combinations.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.24%. Comparing base (d3ef6e3) to head (87a96ee).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2592      +/-   ##
==========================================
+ Coverage   86.07%   86.24%   +0.16%     
==========================================
  Files         331      331              
  Lines       17654    17654              
  Branches     1646     1631      -15     
==========================================
+ Hits        15195    15225      +30     
+ Misses       2459     2429      -30     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ranadeepsingh
Rana Singh (ranadeepsingh) force-pushed the copilot/ancient-1912-vw-barrier branch from 91c7c96 to 71a0815 Compare August 1, 2026 13:51
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI review requested due to automatic review settings August 7, 2026 05:09
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

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.

Review details

Suppressed comments (1)

vw/src/main/scala/com/microsoft/azure/synapse/ml/vw/VowpalWabbitBaseLearner.scala:187

  • df.rdd is evaluated twice here (once for getNumPartitions and again for barrier()), which can rebuild the underlying RDD/query plan and is unnecessary. Assign it to a local val and reuse it for both the partition check and the barrier path.
    if (getUseBarrierExecutionMode && df.rdd.getNumPartitions > 1)
      df.rdd.barrier().mapPartitions(inputRows => trainIteration(inputRows, localInitialModel)).collect().toSeq
    else
      df.mapPartitions(inputRows => trainIteration(inputRows, localInitialModel))(encoder).collect().toSeq
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

## Summary
Use barrier execution only when VowpalWabbit training enables it and the prepared dataset has more than one partition. Add Spark-stage execution-path assertions for enabled multi-partition, disabled multi-partition, and enabled single-partition training.

## Prompting Intent
Revalidate the current VowpalWabbitBaseLearner behavior, restore the minimal source-compatible rule proposed by the ancient PR, prove it through TDD and targeted VW validation, and keep the change isolated from LightGBM.

## Linked Sources
- Original proposal: microsoft#1912
- Initial CI failure: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229206650
- Repository review guidance: .github/skills/code-review/SKILL.md
- Local validation guidance: .github/skills/synapseml-local-setup/SKILL.md

## Rationale
Keep the public API and multi-partition synchronization behavior unchanged with a short-circuit partition-count guard. Observe Spark stage metadata rather than adding a test-only PipelineStage subclass, and use a marker job to drain asynchronous listener events deterministically; this proves the selected execution path without entering global stage discovery.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ranadeepsingh
Rana Singh (ranadeepsingh) force-pushed the copilot/ancient-1912-vw-barrier branch from 1536421 to 87a96ee Compare August 7, 2026 07:03
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@ranadeepsingh
Rana Singh (ranadeepsingh) merged commit 3c988d0 into microsoft:master Aug 7, 2026
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants