perf: run InternalCompat Scala and Python validation in parallel lanes - #2657
Closed
Rana Singh (ranadeepsingh) wants to merge 7 commits into
Closed
Conversation
…r published 'Run Internal Python tests (ExcludeAIFunc)' fails on essentially every PR. All ~21 tests error at setup and the only visible cause is an opaque JAVA_GATEWAY_EXITED, which hides an Ivy resolve failure for an Internal jar that no step ever published. Internal's version comes from sbt-dynver, which appends a live '-<yyyyMMdd>-<HHmm>' suffix whenever the working tree is dirty and recomputes it from the wall clock on every sbt load. The 'Retarget Internal to this build' step edits build.sbt with two sed commands, which is exactly what makes the tree dirty for the rest of the job. Each later sbt session therefore picks a different version. Observed on build 231488245 (PR microsoft#2645): 11:32 'sbt packagePython publishM2' bakes ...-1132-SNAPSHOT into the generated Python package and publishes that same jar to ~/.m2 11:36 'sbt testPythonExcludeAIFunc' re-runs CodeGen in a new sbt session, rebakes the package as ...-1136-SNAPSHOT and pip-installs it 11:37 pytest fixtures read the baked coordinate out of the installed package via synapse.ml.ebm.__spark_package_version__ and ask Ivy for ...-1136-SNAPSHOT, which nobody published make_mlflow_models.py runs in between and succeeds: it resolves the 1132 jar correctly from local-m2-cache, because the rebake has not happened yet. The existing same-session mitigation for packagePython and publishM2 is therefore necessary but not sufficient, since a later sbt session recomputes the version regardless. Committing the retarget makes the tree clean, so dynver stops appending a timestamp and every sbt session in the job computes the same version. The OSS side already demonstrates the end state: its checkout is never edited, so its version carries no timestamp at all (1.1.3-python3.13-102-bfba9c82-SNAPSHOT in that same build). Verified against git directly: clean tree v1.1.3.0-1-g0a826a4f tracked file modified (the sed) v1.1.3.0-1-g0a826a4f+DIRTY after commit v1.1.3.0-2-gdc0c4fca + gitignored target/ + untracked v1.1.3.0-2-gdc0c4fca The last row matters for correctness here: 'git describe --dirty', which is what dynver reads, only considers modifications to tracked files, so sbt's own build output cannot reintroduce the drift later in the job. A diff-index guard fails the step loudly if the tree is ever dirty at that point anyway. The commit is local to the agent's checkout and never pushed. Targeted at spark4.1 rather than master so the fix is validated on a branch where this check actually fails, and against SynapseML-Internal's own spark4.1 branch. Also fixes the diagnostic listing published versions: it hardcoded '*-internal_2.12', so on this branch, which builds _2.13, it printed nothing - exactly where a version mismatch most needed to be visible. Internal Scala tests were never affected: sbt resolves through its own classpath and never goes through Ivy for the Internal artifact. Fixes microsoft#2653 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses review feedback on the guard added by this PR: 'git commit -am' exits non-zero when nothing is staged, and this step runs under 'set -e', so an unconditional commit would turn the 'already retargeted' case into a hard step failure and make the step non-idempotent on retry. Verified against git directly: git commit --quiet --no-verify -am (nothing staged) -> exit 1 git diff-index --quiet HEAD -- (clean tree) -> exit 0 git diff-index --quiet HEAD -- (dirty tree) -> exit 1 The preceding grep -qF checks already prove build.sbt holds the requested synapseMLVersion and Resolver.mavenLocal, so an unchanged tree is a valid state rather than an error: it means the edits were already in place. Either way dynver sees a clean tree and emits one stable version, which is the property the rest of the job depends on. The post-commit diff-index guard is unchanged and still fails loudly if the tree is dirty at that point. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The glob comment was written on spark4.1 and asserted 'this branch builds _2.13', which is false on master (scalaVersion 2.12.17). pipeline.yaml is kept identical across master, spark4.1 and spark4.0, so the comment has to hold on all of them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The guard tested whether 'git diff-index --name-only' printed anything. That output is empty both when the tree is clean and when git itself fails, so a broken checkout read as 'clean' and the job continued with a version that was not actually stable - the exact state the guard exists to catch. Measured (unborn HEAD): git diff-index --name-only HEAD -- stdout empty, exit 128 git diff-index --quiet HEAD -- exit 128 Isolating the guard with the commit already skipped: old guard, git broken -> exit 0 (passes) new guard, git broken -> exit 1 (fails loudly) both guards, dirty -> exit 1 both guards, clean -> exit 0 '--quiet' is non-zero for both 'dirty' and 'git failed', neither of which is safe to continue from. The 'if' condition is exempt from 'set -e' and the diagnostic is guarded with '|| true' so a failing git cannot abort the step before the explicit exit 1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Every other paragraph in this block is separated by a bare comment line; these two were run together, so the dynver rationale and the idempotency rationale read as one paragraph. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot suppressed a low-confidence comment pointing out that sed -i '/^resolvers ++= Seq(/a\ Resolver.mavenLocal,' build.sbt appends unconditionally, unlike the version edit above it which is a substitution. Running the step twice against one tree adds a second Resolver.mavenLocal line each time. Measured: 1 -> 2 -> 3 over three runs. That cannot accumulate on a hosted agent, which starts from a fresh VM, but the comment exposed a real defect: because the append always dirtied the tree, the 'tree is already clean' branch added in 6ced9d8 could never be taken, so the state it documents was unreachable. Executing the real step three times against a simulated Internal checkout then surfaced a second, larger problem. Runs 2 and 3 exited 1: Retargeting Internal to OSS version ... On branch master nothing to commit, working tree clean >> step exit=1 'sed -i' rewrites build.sbt in place, so its mtime and inode change even when the bytes are identical. 'git diff-index' trusts stat info before comparing content and reported a phantom modification, so the step took the commit branch, 'git commit' found nothing staged and exited 1, and 'set -e' failed the step. The earlier fix for the same Copilot comment was therefore only half a fix - the guard it added was reading a stale index. Three changes: - 'git update-index -q --refresh || true' before testing the tree, so the guard compares content rather than stat data. '|| true' because --refresh exits non-zero when a file really did change. - Insert Resolver.mavenLocal only when the resolvers block lacks it. - Verify with the same block-scoped predicate the insert decides on, so the two cannot disagree. A file-wide test could be satisfied by the unrelated Resolver.mavenLocal in usage/build.sbt and report success for a resolvers block that never received one - a false green. Re-running the real extracted step three times now gives: run 1 commits, exit 0 run 2 'Retarget produced no changes; tree is already clean', exit 0 run 3 same, exit 0 Resolver.mavenLocal count stays 1 git describe stays v1.1.3.0-1-g22c27875 with no +DIRTY Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
InternalCompat took 67.6 min on build 231549771, making it the slowest signal on a SynapseML PR. Its Scala and Python halves are independent -- neither reads the other's output -- and only ran back to back because they happened to share one checkout. Split the job into two matrix legs, mirroring how SynapseML-Internal's own pipeline shards work across templates/scala_test_job.yml and templates/python_test_job.yml. Measured breakdown of the 67.6 min (build 231549771): shared prefix (checkout, publish OSS to M2, retarget, compile) 12.5m Create Internal conda env 7.1m Scala: spark.aifunc 22.8m + ebm 2.5m + predict 0.4m 25.9m Python: package 1.0m + testPythonExcludeAIFunc 19.2m 20.2m The Scala leg additionally skips conda env creation. SynapseML-Internal runs ScalaAIFuncTests*, ScalaEBMTests and ScalaPredictTests with useConda unset, reserving that env for its PowerBI and nbtest jobs -- and those are exactly the three packages this leg runs. The step already exported CREATE_SEMPY_WRITER=false, which is Internal's non-conda path. Activation is now guarded on the env actually existing, so the step keeps working if the env is ever reintroduced. Result: wall clock becomes max(~39, ~41) rather than the sum, so InternalCompat reports roughly 27 min sooner (-39%). Two legs and no more: every leg re-pays the 12.5 min shared prefix, so a third leg (splitting spark.aifunc away from ebm+predict) would only reach ~38 min while consuming another agent. Internal can shard 6-and-8 ways because its legs resolve a published OSS artifact instead of building one from the change under test, so they have no comparable prefix to amortise. Validation: - YAML parses; all 12 inline scripts pass bash -n - Lane simulation confirms the 7 shared steps run in both legs and the 9 lane-specific steps run in exactly one - The conda-presence predicate was exercised against 5 `conda env list` shapes (present, present+active, absent, conda missing, similar-but- different name) and for set -e safety when conda is absent InternalCompat remains continueOnError: true and is not a required status check, so the new per-leg check names do not affect branch protection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rana Singh (ranadeepsingh)
requested a review
from Brendan Walsh (BrendanWalsh)
as a code owner
August 18, 2026 02:51
|
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. |
Collaborator
Author
|
/azp run |
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
August 18, 2026 02:52
View session
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the InternalCompat Azure Pipelines job by splitting the SynapseML-Internal compatibility validation into parallel matrix lanes (scala and python) so the Scala and Python halves can run concurrently, reducing PR feedback wall-clock time.
Changes:
- Convert
InternalCompatinto a 2-leg matrix (COMPAT_LANE=scala|python) and gate lane-specific steps with conditions. - Skip Internal conda environment creation and activation in the Scala lane (while keeping Python lane behavior intact).
- Harden the “Retarget Internal to this build” step (idempotent
Resolver.mavenLocalinsertion, commit-to-clean-tree logic, improved diagnostics).
Show a summary per file
| File | Description |
|---|---|
pipeline.yaml |
Splits InternalCompat into parallel Scala/Python lanes and adjusts step conditions/retargeting to reduce wall-clock time and improve reliability. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
pipeline.yaml:1722
- With the new matrix, this Key Vault step runs in both lanes. It appears only the Scala lane needs AI-service secrets for
spark.aifunc; running it in the Python lane adds extra time and introduces an avoidable failure point for a lane that doesn’t consume these secrets.
displayName: 'Fetch AI service secrets'
retryCountOnTaskFailure: 3
inputs:
azureSubscription: 'SynapseML Build'
keyVaultName: mmlspark-keys
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
Collaborator
Author
|
Superseded by #2658. This PR was opened from a fork. #2658 carries the identical commit, rebased onto master after #2655 merged, from a branch on |
Rana Singh (ranadeepsingh)
deleted the
perf/internal-compat-parallel-lanes
branch
August 18, 2026 04:03
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
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.
What
Splits the
SynapseML-Internal Compatibility Check(InternalCompat) job into two parallel matrix legs —scalaandpython— cutting its wall clock from 67.6 min to ~41 min (−39%).Important
Stacked on #2655. This branch is based on that PR's head, so the commit list below currently includes its six commits. Once #2655 merges, this reduces to a single commit touching one file (+50 / −9). Please merge #2655 first.
Why
InternalCompatis the slowest signal on a SynapseML PR. Its Scala and Python halves are independent — neither reads the other's output — and only ran back to back because they happened to share one checkout.Measured on build 231549771 (67.6 min, all 67 jobs green):
Create Internal conda envspark.aifunc22.8m +ebm2.5m +predict0.4m (211 tests)testPythonExcludeAIFunc19.2m (26 tests)How
This mirrors what SynapseML-Internal's own pipeline already does — it shards into 6 Scala jobs and 8 Python jobs via
templates/scala_test_job.ymlandtemplates/python_test_job.yml.Shared steps stay ungated; the 9 lane-specific steps get
eq(variables['COMPAT_LANE'], ...).The Scala leg also skips conda env creation (−7.1 min). This is not a guess: SynapseML-Internal runs
ScalaAIFuncTests*,ScalaEBMTestsandScalaPredictTestswithuseCondaunset, reserving that env for its PowerBI and nbtest jobs — and those are exactly the three packages this leg runs. The step already exportedCREATE_SEMPY_WRITER=false, which is Internal's non-conda path. Activation is now guarded on the env actually existing, so the step still works unchanged if the env is ever reintroduced.Why two legs and not more
Every leg re-pays the 12.5 min shared prefix, so sharding has sharply diminishing returns here:
spark.aifuncfromebm+predict)Internal can shard 6-and-8 ways because its legs resolve a published OSS artifact rather than building one from the change under test, so they have no comparable prefix to amortise.
Validation
bash -n.lane=scala→ runs 14, skips 7 (all conda + all Python steps)lane=python→ runs 19, skips 2 (Scala tests + Scala results)conda env listshapes — env present, present-and-active (*), absent, conda missing entirely, and a similar-but-different name (synapseml-internal-old) — plus aset -esafety check confirming the guard does not abort the step when conda is absent. All 5 correct.Risk
InternalCompatkeepscontinueOnError: trueand is not a required status check, so the new per-leg check names (... Compatibility Check scala/... python) do not affect branch protection.succeededOrFailed()back tosucceeded(). That condition existed only to keep Python running when the Scala tests failed in the same job; the split now guarantees that structurally.Not done here
The 12.5 min shared prefix is now the floor. Caching the Internal conda env, or publishing the OSS M2 artifacts once and fanning out, would attack it — but both add serialisation and belong in their own change with their own measurements.