Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,12 @@ before running them because some create or delete cloud resources.
- Target `master` unless the change exists only for a port branch.
- Resolve active and suppressed review findings; document why any finding is
invalid.
- Write review artifacts directly to `reviews/pr-<pr_number>/` and pass that
output directory explicitly to review tools. Preserve attempt/round/model
filenames, the reviewed commit SHA, and resolution evidence for debugging.
Before the PR number exists, keep drafts in the session workspace; their
first committed location must be the numbered PR directory, not a flat or
task-named folder under `reviews/`.
- Trigger Azure validation with `/azp run` where supported. Branch-specific
exceptions are documented in the
[branch context skill](.github/skills/synapseml-branches/SKILL.md).
Expand Down
84 changes: 30 additions & 54 deletions pipeline.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ schedules:
- master

parameters:
- name: fullTests
displayName: Force all PR test families
type: boolean
default: false
- name: testStyle
displayName: Run Style Tests
type: boolean
Expand Down Expand Up @@ -105,66 +109,37 @@ variables:
runCoverage: $[or(eq(variables['Build.Reason'], 'PullRequest'), eq(variables['Build.SourceBranch'], 'refs/heads/master'), startsWith(variables['Build.SourceBranch'], 'refs/tags/'))]

jobs:
- job: CIHelpers
displayName: 'Test CI helpers'
timeoutInMinutes: 15
pool:
vmImage: $(UBUNTU_VERSION)
steps:
- bash: |
set -euo pipefail
python3 -m pip install --disable-pip-version-check --retries 5 --timeout 30 pytest pyyaml
displayName: 'Install CI test dependencies'
retryCountOnTaskFailure: 2
- bash: |
set -euo pipefail
python3 -m pytest tools/ci/tests/ -q
displayName: 'Test CI helpers'

- job: BuildAndCacheSbt
displayName: 'Prewarm sbt bootstrap cache'
cancelTimeoutInMinutes: 0
pool:
vmImage: $(UBUNTU_VERSION)
steps:
- checkout: self
fetchDepth: 1
fetchDepth: 2
- bash: |
set -uo pipefail
run_databricks_cpu=true
run_databricks_gpu=true

if [ "$(Build.Reason)" = "PullRequest" ]; then
target_ref="${SYSTEM_PULLREQUEST_TARGETBRANCH:-}"
if [ -z "$target_ref" ]; then
echo "##vso[task.logissue type=warning]PR target branch was unavailable; running Databricks E2E"
elif git fetch --no-tags --depth=1 origin "$target_ref"; then
target_commit="$(git rev-parse FETCH_HEAD)"

# Azure checks out the PR merge ref, so target tip -> HEAD is the
# effective PR diff. A source-only checkout can only over-report
# target changes, which safely keeps Databricks enabled.
echo "Changed paths used for Databricks E2E impact detection:"
changed_paths_file="$(mktemp)"
trap 'rm -f "$changed_paths_file"' EXIT
git diff --name-only -z --diff-filter=ACMRD "$target_commit" HEAD > "$changed_paths_file"
tr '\0' '\n' < "$changed_paths_file" | sed 's/^/ /'

cpu_decision="$(
python3 tools/ci/databricks_impact.py --null --suite cpu < "$changed_paths_file"
)"
gpu_decision="$(
python3 tools/ci/databricks_impact.py --null --suite gpu < "$changed_paths_file"
)"
case "$cpu_decision" in
true|false) run_databricks_cpu="$cpu_decision" ;;
*)
echo "##vso[task.logissue type=warning]Invalid Databricks CPU impact result; running CPU E2E"
;;
esac
case "$gpu_decision" in
true|false) run_databricks_gpu="$gpu_decision" ;;
*)
echo "##vso[task.logissue type=warning]Invalid Databricks GPU impact result; running GPU E2E"
;;
esac
else
echo "##vso[task.logissue type=warning]Could not fetch PR target branch; running Databricks E2E"
fi
else
echo "Non-PR build; Databricks E2E remains enabled"
fi

echo "Databricks CPU E2E enabled: $run_databricks_cpu"
echo "Databricks GPU E2E enabled: $run_databricks_gpu"
echo "##vso[task.setvariable variable=runDatabricksCpuE2E;isOutput=true]$run_databricks_cpu"
echo "##vso[task.setvariable variable=runDatabricksGpuE2E;isOutput=true]$run_databricks_gpu"
name: detectDatabricksImpact
displayName: 'Detect Databricks E2E impact'
set -euo pipefail
python3 tools/ci/e2e_impact.py
name: detectTestImpact
displayName: 'Select PR notebook E2E jobs'
env:
SYNAPSEML_FULL_TESTS: ${{ parameters.fullTests }}
- template: templates/update_cli.yml
- template: templates/sbt_cache.yml
parameters:
Expand Down Expand Up @@ -260,7 +235,7 @@ jobs:
succeeded(),
eq(variables.runTests, 'True'),
eq('${{ parameters.testDatabricksE2E }}', true),
eq(dependencies.BuildAndCacheSbt.outputs['detectDatabricksImpact.runDatabricksCpuE2E'], 'true')
ne(dependencies.BuildAndCacheSbt.outputs['detectTestImpact.runDatabricksCpuE2E'], 'false')
)
timeoutInMinutes: 300
cancelTimeoutInMinutes: 0
Expand Down Expand Up @@ -293,7 +268,7 @@ jobs:
succeeded(),
eq(variables.runTests, 'True'),
eq('${{ parameters.testDatabricksE2E }}', true),
eq(dependencies.BuildAndCacheSbt.outputs['detectDatabricksImpact.runDatabricksGpuE2E'], 'true')
ne(dependencies.BuildAndCacheSbt.outputs['detectTestImpact.runDatabricksGpuE2E'], 'false')
)
timeoutInMinutes: 300
cancelTimeoutInMinutes: 0
Expand All @@ -316,6 +291,7 @@ jobs:
succeeded(),
eq(variables.runTests, 'True'),
eq('${{ parameters.testFabricE2E }}', true),
ne(dependencies.BuildAndCacheSbt.outputs['detectTestImpact.runFabricE2E'], 'false'),
ne(variables['System.PullRequest.IsFork'], 'True')
)
timeoutInMinutes: 120
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# Round 1 review

## Review summary

| Field | Result |
| --- | --- |
| Round / attempt | 1 / 1 |
| Theme | Broad correctness and specification review |
| Mode / model | Sequential / `gpt-6-astra` |
| Scope | Current uncommitted diff, including both new selector files, against `714d365e71` on the branch targeting `master` |
| Issues found | 0 |
| Verdict | CLEAN |

No concrete correctness bug, unsafe skip relationship, failure masking, or
pipeline-condition defect was found in the reviewed changes.

## Evidence checklist

- [x] Read this worktree's `AGENTS.md`, branch guidance, tracked diff, complete
`tools\ci\test_impact.py`, and complete
`tools\ci\tests\test_test_impact.py`. Reviewed the replacement of the old
Databricks classifier and its tests, pipeline wiring, and documentation.
- [x] `tools\ci\test_impact.py:43-74` uses a positive allowlist and unions mixed
changes. Runtime, Scala tests, resources, notebooks, build/dependency files,
tools, unknown paths, and ambiguous names retain all seven families.
- [x] Checked the isolation claims against actual consumers.
`core\src\main\scala\com\microsoft\azure\synapse\ml\codegen\CodegenConfig.scala:36-49`
separates test overrides from runtime sources.
`core\src\test\scala\com\microsoft\azure\synapse\ml\codegen\TestGen.scala:34-35`
copies overrides into test trees; `project\CodegenPlugin.scala:107-121,317-337`
runs the respective R/Python tests. The pipeline keeps their entire matrices.
- [x] `website\doctest.py:126-143` executes Quick Examples, so their Markdown
retains website tests.
`core\src\test\scala\com\microsoft\azure\synapse\ml\nbtest\DatabricksUtilities.scala:250-252`
and `SharedNotebookE2ETestUtilities.scala:89-106` select notebook inputs by
`.ipynb`, not the allowlisted Markdown.
- [x] `tools\ci\test_impact.py:87-129` verifies the queued merge ref/SHA, two
parents, and source SHA; compares against the first parent; disables rename
detection; and rejects symlink/gitlink modes and malformed diff records.
`tools\ci\test_impact.py:132-165` retains all families for non-PR builds,
forced/unknown overrides, and handled detection errors, and emits all seven
named outputs.
- [x] `pipeline.yaml:119-131` runs helper regressions before detection.
The conditions at `pipeline.yaml:227,260,283,616,703,775,823` use matching
output names with `ne(..., 'false')`; missing outputs do not authorize skips.
Existing dependency-success checks, explicit family switches, and the Fabric
fork restriction remain. Detector crashes fail the prerequisite visibly.
The daily schedule at `pipeline.yaml:45-51` remains unchanged.
- [x] Independently ran focused checks using Python 3.14.6, with bytecode and
pytest cache writes disabled: five union/YAML/prewarm/wiring cases passed;
thirteen real-Git add/modify/delete/rename/empty-diff, moving-target,
symlink/gitlink, and CLI-output cases passed. No full suites were rerun.

## Limits and handoff

This is a local round-1 result, not evidence that Azure has exercised selective
job scheduling. The reported 120 WSL selector passes were supplied by the
requester; the running full-helper and pipeline suites were not treated as
completed. Azure template expansion and a representative selective PR remain
unverified here, as the updated CI README already states.

No implementation changes were made. Preserve this artifact and commit it with
the reviewed code after the required gauntlet; this round does not authorize an
early code commit.

## Later scope correction

The subsequent Opus review found the Codecov upload threshold coupling missed
by this pass. The final patch retains all 54 coverage-producing matrix legs and
limits optional jobs to Databricks CPU/GPU and Fabric E2E. The helper tests now
run independently of prewarm, and the detector is named `e2e_impact.py`.
This original review is retained as history; it is not final-head evidence.
Loading
Loading