Skip to content

Define Fabric Spark compute ARM contract in TypeSpec - #47205

Open
vaibhj wants to merge 3 commits into
mainfrom
users/vaibhj/fabric-spark-compute-typespec
Open

vaibhj wants to merge 3 commits into
mainfrom
users/vaibhj/fabric-spark-compute-typespec

Conversation

@vaibhj

@vaibhj vaibhj commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Background

The MachineLearningServices ARM service already returns attached FabricSpark computes, but the TypeSpec source does not declare that compute discriminator or its Fabric resource IDs. The Python SDK currently has to add these models to generated REST files manually, which a future regeneration would erase.

Change

Add FabricSpark to ComputeType and define FabricSpark and FabricSparkProperties alongside the existing SynapseSpark variants. The nested contract carries the four required Fabric IDs and a read-only pool name. No endpoints, authentication behavior, or non-Fabric compute types change.

Dependency

The SDK change in Azure/azure-sdk-for-python#49501 depends on this source contract for durable generated models. Regeneration/pin update must be validated before the SDK change ships; its current generated models are provisional.

Validation

Compiled TypeSpec with pnpm 12.6.0 and committed all 11 versioned OpenAPI outputs. TypeSpec Validation and SDK Validation - Python pass. Swagger BreakingChange fails because the new FabricSpark discriminator extends ComputeType in already-published versions; API owner approval or an agreed version-gated contract is required before merge. The four Fabric IDs are GUID identifiers rather than ARM resource IDs, as clarified in the discussion. No live service mutation was performed.

Model the existing Fabric Spark attached compute contract alongside Synapse Spark, with the Fabric workspace, Lakehouse, Environment, and custom pool IDs and read-only pool name.

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
@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ This PR targets either the main branch of the public specs repo or the RPSaaSMaster branch of the private specs repo. These branches are not intended for iterative development. Therefore, you must acknowledge you understand that after this PR is merged, the APIs are considered shipped to Azure customers. Any further attempts at in-place modifications to the APIs will be subject to Azure's versioning and breaking change policies. Additionally, for control plane APIs, you must acknowledge that you are following all the best practices documented by ARM at aka.ms/armapibestpractices. If you do intend to release the APIs to your customers by merging this PR, add the PublishToCustomers label to your PR in acknowledgement of the above. Otherwise, retarget this PR onto a feature branch, i.e. with prefix release- (see aka.ms/azsdk/api-versions#release--branches).
  • ❌ This PR is in purview of the ARM review (label: ARMReview). This PR must get ARMSignedOff label from an ARM reviewer.
    This PR is not ready for ARM review (label: NotReadyForARMReview). This PR will not be reviewed by ARM until relevant problems are fixed. Consult the rest of this Next Steps to Merge comment for details.
    Once the blocking problems are addressed, add to the PR a comment with contents /azp run. Automation will re-evaluate this PR and if everything looks good, it will add WaitForARMFeedback label which will put this PR on the ARM review queue.
    For details of the ARM review, see aka.ms/azsdk/pr-arm-review
  • ❌ This PR is NotReadyForARMReview because it has the BreakingChangeReviewRequired label.
  • ❌ This PR has at least one breaking change (label: BreakingChangeReviewRequired).
    To unblock this PR, follow the process at aka.ms/brch.
  • ❌ The required check named Swagger BreakingChange has failed. To unblock this PR, follow the process at aka.ms/brch.


Comment generated by summarize-checks workflow run.

@github-actions

Copy link
Copy Markdown

Contributor readiness: required access

Warning

The access issues below can prevent PR approvals from counting or block Azure DevOps pipeline runs.

User / area Access issue PR impact
🔴 vaibhj Azure membership not public. Not determined.

Internal contributors: setup / renew required access. Recheck: /azsdk check-access.

Non-blocking report; external fork contributions are allowed. GitHub review rules still apply.

@github-actions github-actions Bot added ARMReview resource-manager TypeSpec Authored with TypeSpec WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required ARMAutoSignedOff-Trivial ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review ARMAutoSignedOff-Test BreakingChange-Go-Sdk and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Oct 12, 2026
@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

API Change Check

APIView identified API level changes in this PR and created the following API reviews

Language API Review for Package
Go sdk/resourcemanager/machinelearning/armmachinelearning
Java com.azure.resourcemanager:azure-resourcemanager-machinelearning
JavaScript @azure/arm-machinelearning
C# Azure.ResourceManager.MachineLearning
Swagger Microsoft.MachineLearningServices-MachineLearningServices

Comment generated by After APIView workflow run.

Compile the updated TypeSpec into all versioned OpenAPI snapshots so spec validation and downstream generators see the new discriminator.

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
@github-actions github-actions Bot added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required and removed ARMAutoSignedOff-Test ARMAutoSignedOff-Trivial ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review labels Oct 12, 2026
Match TypeSpec formatter output for the documented compute variant.

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
@github-actions github-actions Bot added BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required NotReadyForARMReview and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Oct 12, 2026
@github-actions

Copy link
Copy Markdown

ARM API Review Summary

Reviewed PR #47205 at head SHA 99b98e0bdb68f26290317a5b1ad5e7f6a31cc7e6 | Triggered by: pull_request

Caution

Independent Critic verification did not run. The findings below are unverified. Severity is unchanged, and no ARM review labels were modified.

Approval labels observed: none.

Category Count
🔴 Blocking 2
🟠 Warning 0
🔵 Suggestion 0

The change modifies existing published API-version outputs without version gating and models four ARM resource references as unconstrained strings; the mandatory independent Critic was unavailable, so both findings retain their original Blocking severity and review labels were left unchanged.

posted-by: arm-api-reviewer-agent | rule: summary | category: summary | severity: blocking | classification: new | critic: unknown | head-sha: 99b98e0

🔍 ARM API review by ARM API Review: Automated Workflow

@github-actions

Copy link
Copy Markdown

[NEW] 🔴 Blocking [TSP-ARM-RESOURCE-ID] specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/MachineLearningServices/models.tsp - line 11501 - The new fabricWorkspaceId, lakehouseId, environmentId, and poolId properties represent ARM resource references but are declared as unconstrained string values. This omits ARM resource-ID validation and resource-type semantics required by TSP-ARM-RESOURCE-ID.

Classification reasoning: Introduced in this PR because these four properties are newly added in the pinned head and are absent from the pinned base SHA.

Approval context: No label in the applicable suppression or approval family was observed.

Suggested fix: Declare each property with the constrained armResourceIdentifier type for its intended Fabric workspace, Lakehouse, Environment, and custom Spark pool resource type, preserving the existing wire property names.

posted-by: arm-api-reviewer-agent | rule: TSP-ARM-RESOURCE-ID | category: resource-modeling | severity: blocking | classification: new | critic: unknown | head-sha: 99b98e0

🔍 ARM API review by ARM API Review: Automated Workflow

@github-actions

Copy link
Copy Markdown

[NEW] 🔴 Blocking [Section 26.0] specification/machinelearningservices/resource-manager/Microsoft.MachineLearningServices/MachineLearningServices/preview/2024-10-01-preview/openapi.json - line 26029 - This change adds the FabricSpark discriminator value and FabricSpark/FabricSparkProperties definitions to existing published API-version outputs, including this preview version and stable versions, without introducing a later API version or TypeSpec version gate. The pinned base SHA has no such contract, so existing published API versions are being changed directly.

Classification reasoning: Introduced in this PR because the pinned base SHA lacks FabricSpark, while the head adds it to all 11 pre-existing API-version directories; main.tsp already lists those versions, and no new API-version directory was added.

Approval context: No label in the applicable API-version approval family was observed. If this contract is intended for a later API version, add that version and gate the model with TypeSpec versioning; otherwise obtain the required approval before changing published outputs.

JSON path: $.definitions.ComputeType.enum and $.definitions.FabricSpark

Suggested fix: Add a later API version, use the appropriate TypeSpec @added/versioning annotations so the new discriminator and models emit only in that version, and regenerate without modifying the existing published OpenAPI files.

posted-by: arm-api-reviewer-agent | rule: Section-26.0 | category: versioning | severity: blocking | classification: new | critic: unknown | head-sha: 99b98e0

🔍 ARM API review by ARM API Review: Automated Workflow

@vaibhj

vaibhj commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Clarification for the ARM API review: fabricWorkspaceId, lakehouseId, environmentId, and poolId are Fabric GUID identifiers (not ARM resource IDs); armResourceIdentifier would reject valid service payloads and change the wire contract. The published-version immutability / AddedEnumValue CI finding is legitimate and still needs API owner approval or an agreed version-gated contract. This PR is open for review but should not merge while that finding is outstanding.

@vaibhj

vaibhj commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Requesting an explicit API-owner / breaking-change review for the published-version change in this PR. The Swagger BreakingChange check reports AddedEnumValue because adding FabricSpark to ComputeType changes 11 previously published OpenAPI versions, including stable versions. This is a real change to the published specifications; I am not proposing to suppress the check or treat the ARM auto-review as approval.

The FabricSpark wire behavior is already deployed for these existing compute APIs, and the Python SDK PR Azure/azure-sdk-for-python#49501 uses stable API version 2025-12-01. Introducing FabricSpark only in a new preview would leave that existing SDK API version without a reproducible TypeSpec source. Can the API owners and breaking-change reviewers review whether documenting the already-shipped discriminator in the existing versions qualifies for an approved exception? If yes, please provide the required explicit approval and the correct waiver/label procedure for the failing check; if not, please confirm the required new-version migration path so we can update the SDK and service together. This PR should not merge until the decision and required checks are resolved.

The four Fabric IDs are GUIDs, not ARM resource IDs; changing them to armResourceIdentifier would not be wire-compatible.

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

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

Labels

ARMReview BreakingChange-Go-Sdk BreakingChange-JavaScript-Sdk BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required NotReadyForARMReview resource-manager TypeSpec Authored with TypeSpec

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant