Skip to content

Support attached Fabric Spark compute in azure-ai-ml - #49501

Open
vaibhj wants to merge 5 commits into
mainfrom
users/vaibhj/fabric-spark-compute-sdk
Open

vaibhj wants to merge 5 commits into
mainfrom
users/vaibhj/fabric-spark-compute-sdk

Conversation

@vaibhj

@vaibhj vaibhj commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Background and defect

FabricSpark attachments already work through the AML ARM service, but the Python SDK treats their computeType as unsupported. SDK get/list and the v2 CLI therefore omit the Fabric IDs and pool details.

Change and compatibility

Add a FabricSparkCompute entity, YAML schema, ARM discriminator/models, and typed dispatch. Attach sends the four Fabric resource IDs, while get/list return the pool name, identity, attachment status, and provisioning data. Detach uses the existing begin_delete(name, action="Detach") path; the default Delete behavior for other compute types is unchanged. No Vienna or portal changes are included. Generated ARM models require the API spec/generation pipeline to retain this discriminator in future regenerations.

Dependencies and merge order

The source TypeSpec contract in Azure/azure-rest-api-specs#47205 must clear published-version compatibility review and support reproducible generated models before merge. The CLI pin in Azure/azure-cli-extensions#10459 must follow an SDK release containing this change.

Validation

Focused SDK compute tests: 42 passed, 1 skipped; Black and API digest checks passed. A FabricSpark response without optional nested details now deserializes to a typed compute, while attach requires all four IDs. Local generated-client regeneration stalled before output; model reproducibility remains unverified. Latest PR gate is running. No live detach was attempted.

Add typed Fabric Spark ARM models, YAML conversion, and compute dispatch so attach, get, and list expose the existing service contract. Preserve the existing delete default; attached compute is detached with action=Detach.

Authored-by: GitHub Copilot CLI v1.0.88`nModel: GPT-6 Sol (gpt-6-sol)`nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>`nCopilot-Session: 31c47880-d3a1-4cc6-a893-ab264315c7db
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
9 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Generated-model provenance, API snapshots, release notes, and public documentation remain unresolved.

4 open findings
What changed in this PR

Adds Fabric Spark compute attachments to azure-ai-ml, including serialization, ARM dispatch, public exports, and operation coverage.

Changes:

  • Adds FabricSparkCompute and its YAML schema.
  • Adds ARM models, discriminator, enum, and typed dispatch.
  • Tests attach, get, list, detach, and serialization behavior.
File Description
tests/​compute/​unittests/​test_compute_operations.py Tests Fabric Spark operations.
tests/​compute/​unittests/​test_compute_entity.py Tests validation and serialization.
entities/​_compute/​fabricspark_compute.py Implements the compute entity.
entities/​_compute/​compute.py Adds Fabric Spark dispatch.
entities/​__init__.py Exports the public entity.
constants/​_compute.py Adds the compute type constant.
_schema/​compute/​fabricspark_compute.py Defines the YAML schema.
_restclient/​arm_ml_service/​models/​_models.py Adds generated ARM models.
_restclient/​arm_ml_service/​models/​_enums.py Adds the ARM enum value.
_restclient/​arm_ml_service/​models/​__init__.py Exports ARM models.

🧠 Review effort: Balanced

Comment thread sdk/ml/azure-ai-ml/azure/ai/ml/entities/__init__.py
@github-actions

This comment has been minimized.

Document and snapshot the new compute entity; preserve ARM resource IDs and normalize provisioning states from get and list. Pin the authoritative TypeSpec source for later client regeneration.

Authored-by: GitHub Copilot CLI v1.0.88

Model: GPT-6 Sol (gpt-6-sol)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 31c47880-d3a1-4cc6-a893-ab264315c7db
Copilot AI balanced review requested due to automatic review settings October 12, 2026 23:24
@vaibhj

vaibhj commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

⚠️ PR Size Warning: This PR changes 448 lines across 14 files (441 insertions, 7 deletions). Consider splitting into smaller, focused PRs for easier review. A possible boundary is ARM client/spec model support first, then the public compute entity, YAML schema, API snapshot, and tests; the dependent pieces must still ship together for functional support.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The loader crashes when the optional ARM properties payload is absent, and the contract must be reconciled across the specification and SDK.

1 open finding
3 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Handle optional FabricSpark properties before dereferencing

sdk/​ml/​azure-ai-ml/​azure/​ai/​ml/​entities/​_compute/​fabricspark_compute.py:76

FabricSpark.properties is optional in both the generated model and the pinned TypeSpec contract, but this loader dereferences it unconditionally. A contract-valid get/list response with no nested details will raise AttributeError instead of returning a compute. Either make properties required upstream and regenerate, or handle None here and reconcile the entity/schema's required IDs.

🧠 Review effort: Balanced

Permit read-only Fabric Spark compute responses without optional nested properties. Continue to require all four resource IDs when attaching a pool, and keep the public API snapshot in sync.

Authored-by: GitHub Copilot CLI v1.0.88

Model: GPT-6 Sol (gpt-6-sol)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 31c47880-d3a1-4cc6-a893-ab264315c7db
Copilot AI balanced review requested due to automatic review settings October 12, 2026 23:45
@vaibhj

vaibhj commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the previously missed Copilot review-summary finding in 1280790: get/list now deserialize FabricSpark when the optional nested properties object is absent, returning a typed resource with unavailable Fabric IDs omitted. Attach remains strict: YAML requires the four IDs and serializing an incomplete read-only entity raises ValidationException rather than sending an invalid PUT. Added a regression test; focused compute tests pass (42 passed, 1 skipped). No inline review threads remain open.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The committed API metadata hash does not match the updated API snapshot.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Comment thread sdk/ml/azure-ai-ml/api.metadata.yml Outdated
@github-actions

This comment has been minimized.

Serialize pool IDs without an invalid ARM resourceId, permit sparse read-only YAML responses, and enforce complete IDs when attaching. Correct API metadata using the snapshot tool's normalized digest.

Authored-by: GitHub Copilot CLI v1.0.88

Model: GPT-6 Sol (gpt-6-sol)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 31c47880-d3a1-4cc6-a893-ab264315c7db
Copilot AI balanced review requested due to automatic review settings October 13, 2026 16:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The TypeSpec pin is not main-reachable, and the public constructor exposes a read-only parameter that attach silently ignores.

1 open finding
1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Remove ignored pool_name from public constructor

sdk/​ml/​azure-ai-ml/​azure/​ai/​ml/​entities/​_compute/​fabricspark_compute.py:54

pool_name is read-only in the REST model (FabricSparkProperties.pool_name has read visibility only) and _to_rest_object() never sends it, but this signature advertises it as a caller-supplied attach option. A user can pass a value that is silently ignored. Remove it from the public constructor signature and populate the returned attribute through the internal REST-loading path (for example via **kwargs), then regenerate the API snapshot.

🧠 Review effort: Balanced

directory: specification/machinelearningservices/MachineLearningServices.Management
commit: bce80108d7ce39dccc1e9c2df1b1bd99d018bf7f
directory: specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/MachineLearningServices
commit: 99b98e0bdb68f26290317a5b1ad5e7f6a31cc7e6

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed: this source commit is not yet reachable from Azure/azure-rest-api-specs/main. The authoritative FabricSpark contract is in Azure/azure-rest-api-specs#47205, whose published-version Swagger BreakingChange gate is still failing. We cannot replace this pin with a main-reachable commit containing FabricSpark until that upstream PR is approved and merged. I am leaving this thread open rather than claiming the SDK release-validation requirement is met; once the spec lands, I will update the pin to its main-reachable commit, verify the generator contract, and resolve this thread.

This comment was generated by an AI assistant on behalf of the PR author.

Preserve poolName in get/list output without accepting it as an attach input. Match existing compute identity-helper lint conventions and regenerate the public API snapshot.

Authored-by: GitHub Copilot CLI v1.0.88

Model: GPT-6 Sol (gpt-6-sol)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 31c47880-d3a1-4cc6-a893-ab264315c7db
Copilot AI balanced review requested due to automatic review settings October 13, 2026 17:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The pinned specification remains in blocked, unmerged PR #47205, leaving release validation and generated-model reproducibility unresolved.

1 open finding

🧠 Review effort: Balanced

@github-actions

Copy link
Copy Markdown
Contributor
[Pilot] PR Pipeline Failure Analysis

What failed

Pipeline build 6951821 — python - pullrequest (Build Test windows2022_312) failed, with two unrelated test failures on windows2022_312:

  1. sdk.ml.azure-ai-ml.tests.batch_services.e2etests.test_batch_endpoint.TestBatchEndpoint.test_batch_endpoint_create (whl artifact) — failed on setup with azure.core.exceptions.HttpResponseError: Operation returned an invalid status 'None' while the test-proxy attempted to start a playback session (PLAYBACK_START_URL returned a non-200 status).
  2. sdk.ml.azure-ai-ml.tests.utils.unittests.test_artifact_cache.test_concurrent_cache_writer_waits_for_checksum[set] (sdist artifact) — failed with FileExistsError: [WinError 183] Cannot create a file when that file already exists at azure/ai/ml/_utils/_artifact_utils.py:572 (os.rename(download_path, artifact_package_path)). This is a known Windows-specific os.rename semantics difference (POSIX overwrites the destination silently; Windows raises if it exists) surfacing in a concurrency test designed to race two cache writers.
Relevant pipeline output
E  azure.core.exceptions.HttpResponseError: Operation returned an invalid status 'None'
..\..\..\.venv\azure-ai-ml\.venv_whl\Lib\site-packages\devtools_testutils\proxy_testcase.py:117: HttpResponseError

E  FileExistsError: [WinError 183] Cannot create a file when that file already exists: 'C:\\Users\\cloudtest\\...\\package' -> 'C:\\Users\\cloudtest\\...\\1.2.3'
..\..\..\.venv\azure-ai-ml\.venv_sdist\Lib\site-packages\azure\ai\ml\_utils\_artifact_utils.py:572: FileExistsError

Both failing tests live in tests/batch_services/ and tests/utils/unittests/test_artifact_cache.py, and the second failure's underlying code is in azure/ai/ml/_utils/_artifact_utils.py — none of which this PR touches. The PR's diff is scoped to FabricSpark compute entity/schema code (azure/ai/ml/_schema/compute/, azure/ai/ml/entities/_compute/, azure/ai/ml/_restclient/arm_ml_service/, constants/_compute.py) and compute unit tests.

Recommended next steps

  • Re-run the pipeline — failure 1 looks like a transient test-proxy playback/infrastructure issue (invalid status 'None') unrelated to this PR's changes.
  • Failure 2 is a pre-existing Windows-platform race in test_concurrent_cache_writer_waits_for_checksum/_artifact_utils.py os.rename usage; track it separately as flaky/platform-specific test debt — it is not something this PR's diff caused or can fix.
  • See https://aka.ms/ci-fix

Copilot detected the failing pipeline and generated the analysis above. To have it attempt a
fix automatically, reply with @copilot please fix the failing pipeline on this PR.

Generated by Pipeline Analysis Next Steps · copilot · sonnet50 · 37.7 AIC · ⌖ 9 AIC · ⊞ 8.9K · ◷

"EstimatedVMPrice",
"EstimatedVMPrices",
"ExternalFQDNResponse",
"FabricSpark",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we are adding new type for fabric compute, can you add sample for this in samples folder: https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/ml/azure-ai-ml/samples

EstimatedVMPrice,
EstimatedVMPrices,
ExternalFQDNResponse,
FabricSpark,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that these changes "fabric compute" related do not create regression in existing flows by submitting the samples pipeline: https://dev.azure.com/azure-sdk/internal/_build?definitionId=5066

@pytest.mark.unittest
@pytest.mark.core_sdk_test
class TestComputeEntity:
def test_fabric_spark_yaml_and_attach_body(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appreciate that you added unit tests. can you also add e2e test cases and add the recording of those test cases in this PR?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants