fix: resolve batched service header columns - #2729
Rana Singh (ranadeepsingh) merged 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Lenin Mookiah (@leninworld) 👋! 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. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues remain, and all assessments indicate approval readiness.
Review effort: Lite
Findings: None
What changed in this PR
Fixes batched Cognitive Service authentication by resolving credentials and headers from batched values while preserving payload arrays.
Changes:
- Adds batch-aware credential and header-map resolution.
- Updates Fabric fallback handling and validates incompatible types.
- Adds regression tests and documents batch credential behavior.
| File | Description |
|---|---|
docs/Explore Algorithms/AI Services/Advanced Usage - Async, Batching, and Multi-Key.ipynb |
Documents per-batch credential behavior. |
cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/CognitiveServiceBaseSuite.scala |
Tests batching, fallback, validation, and payload preservation. |
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/ServiceHeaderValues.scala |
Resolves and validates batched header values. |
cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/CognitiveServiceBase.scala |
Applies batch-aware authentication and header resolution. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Keep the contributor's header-only batch resolution and accept mutable as well as immutable Scala sequences. Add credential-free public-transformer regressions and clarify per-batch header selection. ## Prompting Intent Review and repair the submitted PR on its existing contributor branch. Prove that it fixes the reported issue, check security and regressions, and complete current-head review and CI rather than stopping at a queued build. ## Linked Sources - Submitted PR: microsoft#2729 - Original issue and reproduction: microsoft#2064 - Contributor implementation: microsoft@14c59c4 ## Rationale Scala 2.13's unqualified Seq excludes mutable Spark array representations. Matching scala.collection.Seq retains lazy, allocation-light traversal without changing public signatures, parameter serialization, or payload handling. Replaying the contributor implementation on both Spark ports demonstrated the rejection before this adjustment. Loopback HTTP tests exercise TextSentiment and AnalyzeHealthText through automatic batching, request creation, asynchronous polling, response parsing, and flattening. They also check partial batches, scalar manual batches, batch-size-one routing, copy/save/load, document languages, and output alignment. The original master implementation fails the four automatic-batching cases with the issue's WrappedArray-to-String cast, while the scalar control passes. Strengthen helper coverage for auth precedence, lazy Fabric fallback, nullable maps, invalid-type redaction, and mutable arrays. Make the batching helper test deterministic with one partition. Document that batches do not group by credential and header maps from later rows are not merged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
I checked both observations in the current-head review. Subscription regions. Empty values and schema validation. The new helper validates values it consumes, not every declared Spark column type. Empty arrays and empty maps contain no usable credential/header, so they are treated as absent. Present incompatible values raise parameter-specific errors without echoing their contents. Full schema validation, including rejection of empty arrays with incompatible declared element types, is not part of this fix. I clarified the PR description to avoid claiming otherwise. Neither observation invalidates the reproduced #2064 fix or identifies a regression in the supported path. The distinction matters: the four automatic-batching public regressions fail on the original master code and pass with this patch, and all 22 targeted tests pass on each of Spark 3.5, 4.0, and 4.1. No review threads or suppressed findings were present on |
|
Thanks Lenin Mookiah (@leninworld) for fixing the batched credential-column crash in #2064. I kept your shared-header approach and pushed 06f84e3a4a to this PR without rewriting your commit. The production adjustment is to match The public regressions reproduced four automatic-batching failures on pre-fix master and passed with the patch. All 22 targeted Scala tests passed locally on Spark 3.5, 4.0, and 4.1. The generated Python APIs also passed the credential-free loopback checks against the packaged JARs. Azure build 236902583, triggered with
The final review audit found no unresolved threads or suppressed current-head findings. I addressed the automated review's scope observations here. The PR description records the inspected CI warnings and distinguishes CI's Spark 4.1 compile replay from the local runtime tests. The description also makes the scope explicit: this fixes shared headers, not all column-bound request settings. Separate probes still reproduce pre-existing Please review my additions and sign off if they fit your intent. If they do not, feel free to revert my commit, or let me know and I will revert it. CLA status is green. Your sign-off on these additions and human maintainer approval are still outstanding. I have not merged the PR. |
Thanks for the detailed follow-up. I reviewed commit I also agree with keeping the other column-bound settings and full schema validation outside this PR. The additions fit my intent, and I’m signing off on the current head. I have no further changes to request. |
714d365
into
microsoft:master
Related Issues/PRs
Fixes #2064
What changes are proposed in this pull request?
Automatically batched Cognitive Service transformers now resolve request-header service parameters from the batched values instead of casting the entire Spark array to a scalar.
Batching behavior is otherwise unchanged. No warning is emitted for multiple distinct credentials within one batch.
This is a shared-header fix, not a general solution for every column-bound request parameter. Independent public-API probes on the current packaged code still reproduce automatic-batching failures for
modelVersionColandloggingOptOutColinAnalyzeTextandAnalyzeTextLongRunningOperations. The legacyTextSentiment.modelVersionColpath sends the batched collection's string representation as the query value. Those request-body/query paths are unchanged by this PR and need a separate fix. The credential-column control passes in all three stages.Translator's separate
subscriptionRegionparameter is unchanged. Its scalar region column is not automatically batched; manually supplied array-valued regions remain unsupported.How is this patch tested?
Local validation with JDK 11 and Spark 3.5.0:
cognitive/Test/compilepassed.CognitiveServiceBaseSuite: 12/12 passed, covering batched string/map headers, null and blank credentials, Fabric fallback, invalid element types, public batching/flattening, and payload isolation.Docker integration used Spark 3.5.1, Java 11.0.22, and Linux x86-64. A real
TextSentimentstage loaded the branch-built Cognitive JAR and called a credential-free local mock endpoint. Two rows were combined into one request, the first usable subscription key authenticated the batch, andFlattenBatchpreserved both original row keys.Validation evidence
Local compilation, style, and focused Scala regression suite:
Docker Spark end-to-end regression using the branch-built artifact:
Repaired
TextSentimentfunction loaded and transformed the automatically batchedsubscriptionKeyCol:Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?
Maintainer follow-up
Added 06f84e3a4a on the original contributor branch without rewriting the contributor commit.
Seqmeans immutable sequences in Scala 2.13. Matchingscala.collection.Seqalso accepts Spark's mutable arrays and keeps the existing lazy traversal without copying a batch.TextSentimentandAnalyzeHealthTexttransforms. They cover automatic and manual batching, partial batches, per-row routing with batch size one, copy/save/load, asynchronous polling credentials, text/language alignment, and output keys.Independent local evidence for the final source:
The Spark 4 runs replayed the patch on current port heads
7251246d45andb4ca894139, preserving their collection normalization and runtime settings. No shared port branch was modified.The public regression suite was also run against the pre-fix master implementation. Four automatic-batching tests reproduced the reported
WrappedArray-to-Stringexception; the scalar manual-batch control passed. Replaying the contributor helper on both Spark 4 ports exposed mutable-array rejection before the maintainer correction.Both generated Python stages passed the loopback regression using the packaged Core/Cognitive JARs, with class-loading provenance checked. This local Python probe used Python 3.12; CI provides the repository-pinned Python environment. Core/Cognitive compile, test compile, Scala style, code generation, pinned Black, and notebook JSON checks passed.
Current-head Azure validation passed after
/azp run: build 236902583, with trigger reasonpullRequestand merge source058aa19fa3164b0f8336871adc621d7e96cd90f6. Its parents are master3878cae184909569328ea41a6aaccf06f9e80d36and PR head06f84e3a4a228f05217fa4e43be20587b4de2bf0.The final readiness audit found the PR zero commits behind master, no failed/pending/missing required checks, no unresolved review threads, and no suppressed current-head findings. The current-head automated review reported no findings; its scope observations are addressed in this response. CLA status passed. Contributor sign-off on the maintainer additions and human maintainer approval remain outstanding; this PR has not been merged.
The earlier build, 236892351, had published 3,592 passing tests and 22 not-executed tests at an intermediate inspection, but hit an Azure certificate-name mismatch during coverage publication and was subsequently canceled after the follow-up push. Those intermediate counts are not a clean or final CI result.
No workflow, dependency, public JVM signature, serialized parameter shape, or payload-handling changes are included.