fix: fix bind port failed on spark yarn environment by coddderX · Pull Request #2233 · microsoft/SynapseML

coddderX · 2024-06-11T11:16:50Z

Related Issues/PRs

What changes are proposed in this pull request?

When training in a yarn Spark environment, sometimes it may encounter port binding failure. This is because multiple Spark jobs might be running on the same machine, but the port binding does not occur simultaneously. For example, at 10:01:00, task 1 of JobA finds that port 10001 is available. Then all tasks in JobA find the port and close immediately, not until 10:03:00 does lightgbm bind to port 10001. At the same time, task 1 of Spark JobB finds port 1001 available at 10:02:00, then all tasks in JobB find the port and close immediately. However, until 10:04:00, lightgbm attempts to bind to port 10001, but the binding fails due to port conflict.

like issuse 2230

How is this patch tested?

I have written tests (not required for typo or doc fix) and confirmed the proposed feature/bug-fix/change works.

Does this PR change any dependencies?

No. You can skip this section.
Yes. Make sure the dependencies are resolved correctly, and list changes here.

Does this PR add a new feature? If so, have you added samples on website?

No. You can skip this section.
Yes. Make sure you have added samples following below steps.

Find the corresponding markdown file for your new feature in website/docs/documentation folder.
Make sure you choose the correct class estimators/transformers and namespace.
Follow the pattern in markdown file and add another section for your new API, including pyspark, scala (and .NET potentially) samples.
Make sure the DocTable points to correct API link.
Navigate to website folder, and run yarn run start to make sure the website renders correctly.
Don't forget to add  before each python code blocks to enable auto-tests for python samples.
Make sure the WebsiteSamplesTests job pass in the pipeline.

Mark Hamilton (mhamilton723) · 2024-06-12T15:08:22Z

/azp run

azure-pipelines · 2024-06-12T15:08:33Z

Azure Pipelines successfully started running 1 pipeline(s).

Codecov Comments Bot (codecov-commenter) · 2024-06-12T15:21:32Z

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.43%. Comparing base (d453ba2) to head (db5e950).
⚠️ Report is 167 commits behind head on master.

Files with missing lines	Patch %	Lines
.../azure/synapse/ml/lightgbm/BasePartitionTask.scala	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2233      +/-   ##
==========================================
- Coverage   84.43%   84.43%   -0.01%     
==========================================
  Files         327      327              
  Lines       16773    16771       -2     
  Branches     1511     1530      +19     
==========================================
- Hits        14162    14160       -2     
  Misses       2611     2611

☔ View full report in Codecov by Sentry.
📢 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.

Mark Hamilton (mhamilton723) · 2024-06-13T17:22:37Z

/azp run

azure-pipelines · 2024-06-13T17:22:50Z

Azure Pipelines successfully started running 1 pipeline(s).

Mark Hamilton (mhamilton723) · 2024-07-22T17:11:38Z

+          try {
+            taskCtx.networkTopologyInfo.localSocket.close()
+          } catch {
+            case e: Exception => log.warn("close local bind port socket failed ")


this looks like its a bit generic of a error, can we make it more specific so we dont catch other errors?

Mark Hamilton (mhamilton723) · 2024-07-22T17:13:30Z

/azp run

azure-pipelines · 2024-07-22T17:13:44Z

Azure Pipelines successfully started running 1 pipeline(s).

## Summary Keep each JVM socket reservation alive through topology exchange and data preparation, then release it immediately before native LightGBM network initialization. Clean up training, helper, and failure paths and add focused contention and cleanup regressions. ## Prompting Intent Recreate the still-valid fix from ancient PR microsoft#2233 on current master, resolve issue microsoft#2230 without broad exception handling or silent fallback, preserve public API compatibility, and validate port competition and cleanup using TDD. ## Linked Sources - Original pull request: microsoft#2233 - Original issue: microsoft#2230 - Unresolved review comment: microsoft#2233 (comment) ## Rationale Store the reservation as transient internal state on the existing three-field topology type so callers remain source compatible. Advance to another port only for BindException; propagate other failures. Release at the unavoidable JVM-to-native handoff because the Java socket cannot be transferred to LightGBM, with final cleanup covering initialization, preparation, helper, and failure paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

## Summary Preserve primary task failures when port-reservation cleanup also fails, retry socket closure while the reservation remains reachable, and hold the exact advertised port during native network-init backoff. ## Prompting Intent Resolve the two independent medium findings on PR microsoft#2595: prevent cleanup from masking primary failures or losing open sockets, and eliminate the native-init retry race without broad fallback catches. Add focused suppression, cleanup-retry, exact-port backoff, and contention regressions. ## Linked Sources - Replacement pull request: microsoft#2595 - Original pull request: microsoft#2233 - Original issue: microsoft#2230 - Original unresolved review: microsoft#2233 (comment) ## Rationale Retry only IOException failures declared by Socket.close, while a tightly scoped Throwable-preservation helper immediately rethrows the protected operation and records cleanup failures as suppressed. Re-reserve only the already-advertised port between native attempts; if contention wins, fail safely with the native error primary so Spark can renegotiate instead of retrying an unreserved or different port. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

## Summary Carry the preceding native-init failure through backoff-reservation release, suppress close failures onto it, and continue the retry when the socket ultimately closes. ## Prompting Intent Resolve the remaining medium review finding on PR microsoft#2595 without weakening cleanup reporting: a first close may fail while the immediate retry succeeds, so the native retry must proceed without replacing its original failure. ## Linked Sources - Replacement pull request: microsoft#2595 - Original pull request: microsoft#2233 - Original issue: microsoft#2230 ## Rationale Use an explicit internal overload to preserve the existing test seam and pass the prior native failure only across the recursive handoff. Continue only after synchronized reservation state confirms the socket closed; otherwise throw the native failure with aggregated close errors suppressed so no unsafe bind occurs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

## Summary Keep each JVM socket reservation alive through topology exchange and data preparation, then release it immediately before native LightGBM network initialization. Clean up training, helper, and failure paths and add focused contention and cleanup regressions. ## Prompting Intent Recreate the still-valid fix from ancient PR microsoft#2233 on current master, resolve issue microsoft#2230 without broad exception handling or silent fallback, preserve public API compatibility, and validate port competition and cleanup using TDD. ## Linked Sources - Original pull request: microsoft#2233 - Original issue: microsoft#2230 - Unresolved review comment: microsoft#2233 (comment) ## Rationale Store the reservation as transient internal state on the existing three-field topology type so callers remain source compatible. Advance to another port only for BindException; propagate other failures. Release at the unavoidable JVM-to-native handoff because the Java socket cannot be transferred to LightGBM, with final cleanup covering initialization, preparation, helper, and failure paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

## Summary Preserve primary task failures when port-reservation cleanup also fails, retry socket closure while the reservation remains reachable, and hold the exact advertised port during native network-init backoff. ## Prompting Intent Resolve the two independent medium findings on PR microsoft#2595: prevent cleanup from masking primary failures or losing open sockets, and eliminate the native-init retry race without broad fallback catches. Add focused suppression, cleanup-retry, exact-port backoff, and contention regressions. ## Linked Sources - Replacement pull request: microsoft#2595 - Original pull request: microsoft#2233 - Original issue: microsoft#2230 - Original unresolved review: microsoft#2233 (comment) ## Rationale Retry only IOException failures declared by Socket.close, while a tightly scoped Throwable-preservation helper immediately rethrows the protected operation and records cleanup failures as suppressed. Re-reserve only the already-advertised port between native attempts; if contention wins, fail safely with the native error primary so Spark can renegotiate instead of retrying an unreserved or different port. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

## Summary Carry the preceding native-init failure through backoff-reservation release, suppress close failures onto it, and continue the retry when the socket ultimately closes. ## Prompting Intent Resolve the remaining medium review finding on PR microsoft#2595 without weakening cleanup reporting: a first close may fail while the immediate retry succeeds, so the native retry must proceed without replacing its original failure. ## Linked Sources - Replacement pull request: microsoft#2595 - Original pull request: microsoft#2233 - Original issue: microsoft#2230 ## Rationale Use an explicit internal overload to preserve the existing test seam and pass the prior native failure only across the recursive handoff. Continue only after synchronized reservation state confirms the socket closed; otherwise throw the native failure with aggregated close errors suppressed so no unsafe bind occurs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Rana Singh (ranadeepsingh) · 2026-08-07T22:52:22Z

Superseded by #2595, which reimplemented the port-reservation fix on current master, addressed the unresolved cleanup/error-handling review concern, added lifecycle and concurrency coverage, and has now merged. The linked issue #2230 is also closed.

fix bind port failed on spark yarn environment

2f508ea

coddderX requested a review from Scott Votaw (svotaw) as a code owner June 11, 2024 11:16

coddderX changed the title ~~fix bind port failed on spark yarn environment~~ fix: fix bind port failed on spark yarn environment Jun 11, 2024

Mark Hamilton (mhamilton723) approved these changes Jun 12, 2024

View reviewed changes

Merge branch 'master' into master

3888968

Mark Hamilton (mhamilton723) reviewed Jul 22, 2024

View reviewed changes

Merge branch 'master' into master

db5e950

Rana Singh (ranadeepsingh) mentioned this pull request Aug 1, 2026

fix: reserve LightGBM worker ports until network init #2595

Merged

Rana Singh (ranadeepsingh) closed this Aug 7, 2026

Navigation Menu

Uh oh!

fix: fix bind port failed on spark yarn environment - #2233

fix: fix bind port failed on spark yarn environment#2233
coddderX wants to merge 3 commits into
microsoft:masterfrom
coddderX:master

coddderX commented Jun 11, 2024 •

edited

Loading

Uh oh!

Mark Hamilton (mhamilton723) commented Jun 12, 2024

Uh oh!

azure-pipelines Bot commented Jun 12, 2024

Uh oh!

Codecov Comments Bot (codecov-commenter) commented Jun 12, 2024 •

edited

Loading

Uh oh!

Mark Hamilton (mhamilton723) commented Jun 13, 2024

Uh oh!

azure-pipelines Bot commented Jun 13, 2024

Uh oh!

Mark Hamilton (mhamilton723) Jul 22, 2024

Uh oh!

Mark Hamilton (mhamilton723) commented Jul 22, 2024

Uh oh!

azure-pipelines Bot commented Jul 22, 2024

Uh oh!

Rana Singh (ranadeepsingh) commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

coddderX commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues/PRs

What changes are proposed in this pull request?

How is this patch tested?

Does this PR change any dependencies?

Does this PR add a new feature? If so, have you added samples on website?

Uh oh!

Mark Hamilton (mhamilton723) commented Jun 12, 2024

Uh oh!

azure-pipelines Bot commented Jun 12, 2024

Uh oh!

Codecov Comments Bot (codecov-commenter) commented Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Mark Hamilton (mhamilton723) commented Jun 13, 2024

Uh oh!

azure-pipelines Bot commented Jun 13, 2024

Uh oh!

Mark Hamilton (mhamilton723) Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

Mark Hamilton (mhamilton723) commented Jul 22, 2024

Uh oh!

azure-pipelines Bot commented Jul 22, 2024

Uh oh!

Rana Singh (ranadeepsingh) commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coddderX commented Jun 11, 2024 •

edited

Loading

Codecov Comments Bot (codecov-commenter) commented Jun 12, 2024 •

edited

Loading