Repository navigation
fix: honor the assigned Spark session for complex Params - #2809
Rana Singh (ranadeepsingh) merged 2 commits into
Conversation
## Summary Pass the MLReader or MLWriter session into complex parameter serialization and deserialization. Retain the existing helper overloads and stored format. Cover writer selection and both directions of reader deserialization policy. ## Prompting Intent Separate unrelated persistence repairs from the native AIPrompt feature. Prove that a caller-selected Spark session governs complex payload handling rather than inheriting an unrelated active session's configuration. ## Linked Sources - Bug: microsoft#2803 - Scope: microsoft#2759 ## Rationale Threading the already assigned session through overloads fixes the actual boundary without changing global Spark state, serialized parameter shapes or unsafe-deserialization defaults. The three regressions failed on baseline and passed with the existing serialization and UDF persistence suites. 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 were identified for the changes to honor assigned Spark sessions during complex parameter persistence.
No open issues 🎉
What changed in this PR
This PR makes complex parameter persistence honor the Spark session assigned to an MLReader or MLWriter, keeping payload handling consistent with metadata handling.
Changes:
- Pass the assigned session through complex payload helpers using new overloads, preserving existing signatures and serialized formats.
- Add a writer-session regression test with a payload round-trip check.
- Test both allowed and denied legacy deserialization through public
UDFTransformerreads when assigned and active session policies differ.
| File | Description |
|---|---|
core/src/test/scala/com/microsoft/azure/synapse/ml/stages/VerifyUDFTransformerPersistence.scala |
Tests assigned-reader deserialization policy in both directions. |
core/src/test/scala/com/microsoft/azure/synapse/ml/core/serialize/ValidateComplexParamSerializer.scala |
Verifies assigned-writer session selection and payload round-trip. |
core/src/main/scala/org/apache/spark/ml/ComplexParamsSerializer.scala |
Propagates assigned sessions through new, backward-compatible helper overloads. |
Review effort: Balanced
|
CI triage: the train execution reports 78/78 passing tests; the R LightGBM runtime also completed. Test-result and coverage publication then failed with a TLS hostname mismatch, tracked in #2812. The missing named train/R outcomes are not counted as verified passes, even where the publication task badge is green. Required CI remains blocked; no product workaround or blind full-build rerun was made. |
## Summary Exercise the retained three-argument save/load helpers with active-session selection and default-session fallback. Check selected-session markers during both serialization and deserialization and verify the restored byte payload. ## 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. Close the identified compatibility-test gap without expanding production scope. ## Linked Sources - PR: microsoft#2809 - Issue: microsoft#2803 - Existing regression suite: core/src/test/scala/com/microsoft/azure/synapse/ml/core/serialize/ValidateComplexParamSerializer.scala ## Rationale Keep the compatibility contract executable rather than relying solely on JVM descriptor checks and external review probes. A small Spark-package test bridge accesses the private[ml] helpers without changing production visibility. Restore prior session state in finally and retain the shared SparkContext. Both new cases pass on the exact base implementation, confirming retained behavior; the relevant current suites pass 15/15 with no skipped tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0e8fdfb7-dcdd-4074-aabb-8634695107f2
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
CI triage for head The only failed job was Requested a retry of that single failed job on the same build and exact source/target, retaining the original attempt evidence. No product, dependency, or pipeline change was made. Full CI remains pending until this job completes and its named Python results are verified. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2809 +/- ##
=======================================
Coverage 87.99% 87.99%
=======================================
Files 347 347
Lines 21608 21608
Branches 2297 2282 -15
=======================================
Hits 19015 19015
Misses 2593 2593
🚀 New features to boost your workflow:
|
|
Engineering readiness verified for The test-only follow-up now exercises both retained three-argument persistence helpers: active-session selection and default-session fallback, including session markers during save/load and restored payload equality. No production signature, serialized format, or dependency changed. The PR description was updated without replacing its template.
Scope remains the direct complex-payload boundary, not recursive nested-Pipeline session propagation or changing a DataFrame's owning session. Only the required human approval remains; 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
## Summary Add direct coverage for the existing no-argument and null-session serializer fallback, including active/default session preservation, and retain the independent review evidence. No production behavior changes in this commit. ## Prompting Intent Complete validation, independent reviews, CI and comment reconciliation for the remaining SynapseML pull requests after the prerequisites were merged. Preserve legacy behavior and public signatures; do not merge any pull request. ## Linked Sources - microsoft#2818 - microsoft#2809 - microsoft#2803 - reviews/pr-2818/task-unknown-attempt-11-stage-attempt-1-review-3-grok-4.7-slice1.md - reviews/pr-2818/task-unknown-attempt-11-stage-attempt-1-review-6-grok-4.7-slice1.md ## Rationale Keep fallback behavior covered at the public serializer boundary rather than changing it in response to an incorrect baseline assumption. The baseline and current runtime agree in 18 absent-session cases; review rebuttals preserve the original findings and document this evidence. ## Validation sbt "set core / Test / fork := false" "core/compile" "core/Test/compile" "core/testOnly com.microsoft.azure.synapse.ml.core.serialize.ValidateComplexParamSerializer com.microsoft.azure.synapse.ml.stages.VerifyUDFTransformerPersistence" "core/scalastyle" "core/Test/scalastyle" Passed: 31 tests, 0 skipped, production/test compilation and both style gates. Six independent three-family review rounds completed with explicit dispositions. New-head CI, remote-comment reconciliation and the final CI-qualified review remain separate gates; the pull request remains a draft. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5ba39157-c8dd-4adb-90e0-ff963a1e6733
Selecting an MLReader or MLWriter Spark session currently does not consistently select the session used for complex payloads. Serializer policy and filesystem configuration can therefore come from a different active session. This propagates the assigned session through the payload helpers.
Related Issues/PRs
Fixes #2803.
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?
Add session-bearing helper overloads and retain the existing signatures and serialized format. Test writer session selection and both directions of the existing legacy-deserialization opt-in through public UDFTransformer reads. Also exercise the retained three-argument helpers with active-session selection and default-session fallback, checking both the selected session and the saved/restored payload. Do not change global deserialization defaults or add native-AI persistence.
This fixes the direct complex-payload helper boundary. It does not claim recursive session propagation through nested Spark Pipeline stages or transfer ownership of a DataFrame's write session.
How is this patch tested?
All three relevant baseline regressions failed: the writer used the wrong session, and both assigned-reader policy directions behaved incorrectly. All three passed with the fix.
The original SBT commands below ran with the branch-selected JDK on baseline
d3f3af6bd3e4b7d191f51c9eb29f9aeffe788193plus the five isolated prerequisite fixes, without AIPrompt feature code. The production implementation remains byte-identical to that tested implementation. Those original five fixes were tested together; the later isolated validation below covers this PR separately, including the two added compatibility tests. Current-head CI is tracked separately.Baseline:
sbt "core/testOnly com.microsoft.azure.synapse.ml.param.VerifyGlobalParams com.microsoft.azure.synapse.ml.core.serialize.ValidateComplexParamSerializer com.microsoft.azure.synapse.ml.stages.VerifyUDFTransformerPersistence com.microsoft.azure.synapse.ml.codegen.PyCodegenSuite com.microsoft.azure.synapse.ml.io.split1.VerifyResponseBodyInspection -- -z \"concurrent global\" -z \"assigned Spark session\" -z \"generated additional method stubs\" -z \"response conversion closes\""- 7 expected failures across the extracted core regressions with unchanged baseline production.Fixed:
sbt "core/testOnly com.microsoft.azure.synapse.ml.param.VerifyGlobalParams com.microsoft.azure.synapse.ml.param.GlobalParamSuite com.microsoft.azure.synapse.ml.core.serialize.ValidateComplexParamSerializer com.microsoft.azure.synapse.ml.stages.VerifyUDFTransformerPersistence com.microsoft.azure.synapse.ml.codegen.PyCodegenSuite com.microsoft.azure.synapse.ml.io.split1.VerifyResponseBodyInspection"- 52 passed, none canceled or ignored.sbt core/scalastyle core/Test/scalastyle- passed, zero errors/warnings.Additional isolated validation for
88b559e794b18a87af0d969edf17b16a84536cbe, using JDK 11 on the master baseline:sbt core/compile core/Test/compile- passed.sbt "core/testOnly com.microsoft.azure.synapse.ml.core.serialize.ValidateComplexParamSerializer com.microsoft.azure.synapse.ml.stages.VerifyUDFTransformerPersistence"- 15/15 passed, zero failed, aborted, canceled, ignored, or pending tests.sbt core/scalastyle core/Test/scalastyle- passed, zero errors/warnings.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.