ci: validate Spark release replay fix with #2595 - #2610
Closed
Rana Singh (ranadeepsingh) wants to merge 10 commits into
Closed
ci: validate Spark release replay fix with #2595#2610Rana Singh (ranadeepsingh) wants to merge 10 commits into
Rana Singh (ranadeepsingh) wants to merge 10 commits into
Conversation
## 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>
…rSuite Adds four regression tests for the error paths introduced by the port reservation handoff that had no coverage: - Native init is never invoked when the advertised port cannot be released before LGBM_NetworkInit; the close failure propagates with the retry close failure suppressed. - A native-init retry whose backoff reservation cannot be closed aborts with the original native failure (close failure suppressed) rather than binding a port that is still held. - Port scanning stops after its 1k-port contention window and never scans past MaxPort. - Out-of-range base ports are rejected before any socket is allocated. No production code changes: this only closes the patch-coverage gap in NetworkManager's release/retry paths. All 15 NetworkManagerSuite tests and lightgbm scalastyle (main + test) pass locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1272df23-c4f3-4048-8789-0320ea286628
## Summary Stage the sbt retry helper outside the repository before switching to Spark release branches, and parameterize the shared cache template so it can invoke that stable path after rebase. ## Prompting Intent Investigate why Spark 3.5 and Spark 4.1 compatibility checks still failed after PR microsoft#2608, reproduce the failure with PR microsoft#2595 changes, implement the complete hotfix, and validate the real release replay path. ## Linked Sources - Failing PR: microsoft#2595 - Prior identity hotfix: microsoft#2608 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229969678 ## Rationale The rebased working tree intentionally comes from the Spark release branch, so master-only CI helpers cannot remain repository-relative. Copying the helper to Agent.TempDirectory preserves release-specific dependency resolution and avoids moving cache warming ahead of the rebase, where exact cache hits could hide missing release dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Replace commit-history rebasing with a three-way application of the synthetic PR merge tree's release-relevant patch onto each Spark release branch. ## Prompting Intent Validate the compatibility hotfix with PR microsoft#2595's real source changes while ensuring CI-only commits do not conflict with old Spark branches that predate the current pipeline and helper files. ## Linked Sources - Validation PR source: microsoft#2595 - Prior identity hotfix: microsoft#2608 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229969678 ## Rationale The compatibility job needs the effective PR content on the release tree, not unrelated CI and documentation commits. Building the patch from the synthetic merge commit preserves GitHub's merge result, handles source branches behind master, retains three-way conflict detection, and avoids requiring commit identity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Collaborator
Author
|
/azp run |
|
Hey Rana Singh (@ranadeepsingh) 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2610 +/- ##
==========================================
+ Coverage 85.81% 86.37% +0.55%
==========================================
Files 331 331
Lines 17654 17742 +88
Branches 1646 1660 +14
==========================================
+ Hits 15150 15324 +174
+ Misses 2504 2418 -86 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
## Summary Remove the redundant Spark 3.5 release matrix leg and replace broad Spark 4.1 runtime suites with full test compilation of the effective PR patch. ## Prompting Intent Explain why the release compatibility jobs exist and keep fixing the failures exposed by validation PR microsoft#2610, accounting for master already targeting Spark 3.5. ## Linked Sources - Original compatibility PR: microsoft#2550 - Streamlining PR: microsoft#2583 - Integration validation PR: microsoft#2610 - Azure validation build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229984834 ## Rationale Normal PR validation already compiles and tests master on Spark 3.5, so replaying onto the older spark3.5 maintenance snapshot duplicates coverage and introduces unrelated JVM drift. Spark 4.1 test compilation catches cross-version source and test API breakage, while the existing master test fan-out supplies runtime coverage without rerunning broad, memory-heavy suites on a constrained compatibility agent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Collaborator
Author
Rana Singh (ranadeepsingh)
added a commit
that referenced
this pull request
Aug 7, 2026
* ci: preserve sbt retry helper during release replay ## Summary Stage the sbt retry helper outside the repository before switching to Spark release branches, and parameterize the shared cache template so it can invoke that stable path after rebase. ## Prompting Intent Investigate why Spark 3.5 and Spark 4.1 compatibility checks still failed after PR #2608, reproduce the failure with PR #2595 changes, implement the complete hotfix, and validate the real release replay path. ## Linked Sources - Failing PR: #2595 - Prior identity hotfix: #2608 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229969678 ## Rationale The rebased working tree intentionally comes from the Spark release branch, so master-only CI helpers cannot remain repository-relative. Copying the helper to Agent.TempDirectory preserves release-specific dependency resolution and avoids moving cache warming ahead of the rebase, where exact cache hits could hide missing release dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: replay only release-relevant PR changes ## Summary Replace commit-history rebasing with a three-way application of the synthetic PR merge tree's release-relevant patch onto each Spark release branch. ## Prompting Intent Validate the compatibility hotfix with PR #2595's real source changes while ensuring CI-only commits do not conflict with old Spark branches that predate the current pipeline and helper files. ## Linked Sources - Validation PR source: #2595 - Prior identity hotfix: #2608 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229969678 ## Rationale The compatibility job needs the effective PR content on the release tree, not unrelated CI and documentation commits. Building the patch from the synthetic merge commit preserves GitHub's merge result, handles source branches behind master, retains three-way conflict detection, and avoids requiring commit identity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: focus release compatibility on Spark 4.1 compilation ## Summary Remove the redundant Spark 3.5 release matrix leg and replace broad Spark 4.1 runtime suites with full test compilation of the effective PR patch. ## Prompting Intent Explain why the release compatibility jobs exist and keep fixing the failures exposed by validation PR #2610, accounting for master already targeting Spark 3.5. ## Linked Sources - Original compatibility PR: #2550 - Streamlining PR: #2583 - Integration validation PR: #2610 - Azure validation build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229984834 ## Rationale Normal PR validation already compiles and tests master on Spark 3.5, so replaying onto the older spark3.5 maintenance snapshot duplicates coverage and introduces unrelated JVM drift. Spark 4.1 test compilation catches cross-version source and test API breakage, while the existing master test fan-out supplies runtime coverage without rerunning broad, memory-heavy suites on a constrained compatibility agent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: SynapseML CI <synapseml-ci@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Validation purpose
This draft PR combines the release-relevant LightGBM changes from #2595 with the follow-up compatibility-pipeline fix. It exists to force both Spark 3.5 and Spark 4.1 jobs through the real cross-branch replay path; it should not be merged as-is.
Root causes covered
git rebase, but CI-only PRs skipped that path and therefore did not prove it.tools/ci/sbt_retry.sh, so cache warming failed before compilation.Proposed fix
sbt_retry.shin$(Agent.TempDirectory)before switching trees.templates/sbt_cache.ymlwith the staged helper path.git apply --3way --index.Local evidence
spark3.5andspark4.1.sbt updateon both rebased release trees using isolated caches.After Azure validation, the two CI commits will be proposed separately as a focused hotfix against
master.