Repository navigation
fix: synchronize global parameter registration and lookup - #2806
Rana Singh (ranadeepsingh) merged 3 commits into
Conversation
## Summary Protect global parameter registration, value updates, lookup and reset with one shared monitor. Add bounded child-JVM regressions for concurrent stage use, including the existing present-null and reset semantics. ## Prompting Intent Keep the AIPrompt feature focused and publish independently demonstrated pre-existing defects as separate fixes. Reproduce the concurrency failure on baseline without allowing a corrupted map to hang the test runner. ## Linked Sources - Bug: microsoft#2804 - Scope: microsoft#2759 - Deferred recovery: microsoft#2800 ## Rationale A shared monitor makes the two-map lookup consistent and preserves existing signatures and Scala Map null/Option behavior. Replacing only one map or using a null-rejecting concurrent collection would not preserve that contract. Baseline concurrency probes failed; the fixed probes and existing global parameter tests passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ba39157-c8dd-4adb-90e0-ff963a1e6733
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to 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. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the shared-registry synchronization change.
No open issues 🎉
What changed in this PR
This PR addresses concurrent access to SynapseML’s shared GlobalParams registry, preventing map corruption and stalled lookups while preserving existing API behavior.
Changes:
- Synchronizes registration, lookup, assignment, and reset using one shared monitor.
- Adds isolated child-JVM concurrency regressions with bounded termination and a sequential test for present-null values.
| File | Description |
|---|---|
core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyGlobalParams.scala |
Adds concurrent registration and value-operation probes, plus present-null compatibility coverage. |
core/src/main/scala/com/microsoft/azure/synapse/ml/param/GlobalParams.scala |
Synchronizes all registry operations without changing public signatures or lookup semantics. |
Review effort: Balanced
## Summary Keep concurrent registry regression probes outside the shared test JVM so a broken mutable map cannot hang later suites. Bound child termination, finish cleanup before restoring interruption, and retain the review findings and resolution evidence. ## Prompting Intent Complete the validation and review gaps in the AIPrompt prerequisite PR stack before reporting a merge order. Preserve existing behavior and do not merge. ## Linked Sources - microsoft#2806 - reviews/pr-2806/ ## Rationale Child JVM isolation contains corrupt-map loops and permanent registrations. A Java 8-compatible manifest classpath avoids command-line limits. Explicit timeout and interruption tests verify termination as well as result reporting. Current source passed core compile, Test compile, VerifyGlobalParams (14/14, zero skips), and both scalastyle gates on master and the native port snapshots. The complete six-stage precommit review passed with all three reviewers. Current-head Azure CI and a fresh CI-qualified review remain separate gates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ba39157-c8dd-4adb-90e0-ff963a1e6733
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Remove three trailing-whitespace lines from preserved PR review artifacts. No production code, tests, build inputs, or review conclusions change. ## Prompting Intent Make the necessary minor fixes to the reviewed SynapseML PRs, address review feedback, and verify full CI against each final head without merging. ## Linked Sources - PR: microsoft#2806 - Issue: microsoft#2804 - Repository artifact policy: AGENTS.md, Pull requests and CI ## Rationale Keep the numbered review artifacts and their provenance as required by the repository guide while making the complete PR diff whitespace-clean. The existing production and regression-test changes are intentionally untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e8fdfb7-dcdd-4074-aabb-8634695107f2
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
CI triage for head The changed-path suites have complete named results: Requested a retry of only those failed jobs on the same build and exact source/target. No certificate bypass, weaker test, product change, or pipeline change was introduced. Full CI remains pending until the retry publishes the missing test results and coverage successfully. |
|
Engineering readiness verified for The minor follow-up removed trailing whitespace only. Required numbered review artifacts and their provenance were preserved; production and test code are unchanged from the independently evaluated revision.
Only the required human approval remains. This is evidence-based engineering readiness, not a claim of absolute regression freedom; the PR has not been merged. |
AB#5837660 ## Summary Add a combined persistence regression after integrating the latest master fixes. Verify that an explicitly assigned Spark session is used without promoting default scalar parameters into explicit values. Preserve the historical review archive through rebased links and include the focused integration review. ## Prompting Intent The engineer approved resolving the newly introduced merge conflicts without losing either CU behavior or upstream fixes, validating and reviewing the result, publishing with an explicit force-with-lease, and rerunning CI. ## Linked Sources - Feature PR: microsoft#2815 - Python method extraction: microsoft#2810 - Assigned Spark sessions: microsoft#2809 - Response cleanup: microsoft#2808 - Attachment cancellation: microsoft#2807 - Global parameter synchronization: microsoft#2806 - Integration target: 6c2699d - Focused review: reviews/pr-2815/task-5837660-attempt-10-review-0-harness.md ## Rationale Keep both sides of the code-generation and persistence conflicts rather than choosing one wholesale. A combined public save/load test checks the interaction that independent upstream and CU tests do not explicitly cover. Rebase preserves the six original changes and their messages; all 53 archived report blobs and modes are unchanged. Local validation passed 310 Scala and 73 Python tests, with one existing ignored global-fuzzing test, plus style, compilation, codegen and pinned formatting. New-head CI remains a separate post-publication gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 653e4257-0554-408f-9e0e-b23750249edb
Concurrent stage construction and default lookup can corrupt the shared GlobalParams maps or leave a request spinning indefinitely. This serializes the small registry operations so registration, lookup, assignment and reset use a consistent view.
Related Issues/PRs
Fixes #2804.
Extracted from the scope refinement for #2759. This fix is independent of the native feature and optional recovery work.
What changes are proposed in this pull request?
Use one shared monitor for both maps, preserving existing signatures, precedence and present-null behavior. Add coordinated child-JVM concurrency regressions with bounded termination, plus sequential compatibility controls. No AIPrompt or Delta code is included.
How is this patch tested?
The files published in
1e8a5098b8a845945b91ad4eaf71659e5469cef1match the validated source blobs.sbt "set core / Test / fork := false" core/compile core/Test/compile "core/testOnly com.microsoft.azure.synapse.ml.param.VerifyGlobalParams" core/scalastyle core/Test/scalastyle- passed in this standalone PR worktree: 14/14 tests, zero skips, both compiles, and both style gates. The same targeted gates also passed on the integrated master and both native port validation snapshots, using each branch's selected JDK.Coverage includes concurrent registration and value operations over 64,000 entries per child JVM, present-null behavior, invalid probe arguments, bounded timeout cleanup, and interruption restoration. Earlier baseline probes exposed mutable-map corruption; targeted cleanup mutations also failed as expected.
All six precommit review stages completed with three independent reviewers per stage. Findings and resolutions are preserved under
reviews/pr-2806/. Current-head Azure CI and the subsequent CI-qualified review are still pending; these local results are not a merge-ready claim.Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?
website/docs/documentationfolder.Make sure you choose the correct class
estimators/transformersand namespace.DocTablepoints to correct API link.npm startto make sure the website renders correctly.<!--pytest-codeblocks:cont-->before each python code blocks to enable auto-tests for python samples.WebsiteSamplesTestsjob pass in the pipeline.