Keep online endpoint/deployment operations pinned to workspace scope for registry-backed clients - #48325
Open
Chakradhar886 with Copilot wants to merge 16 commits into
Open
Keep online endpoint/deployment operations pinned to workspace scope for registry-backed clients#48325Chakradhar886 with Copilot wants to merge 16 commits into
Chakradhar886 with Copilot wants to merge 16 commits into
Conversation
…d clients Co-authored-by: Chakradhar886 <259224138+Chakradhar886@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
Chakradhar886
July 29, 2026 06:16
View session
|
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. |
…0 chars) Co-authored-by: Chakradhar886 <259224138+Chakradhar886@users.noreply.github.com>
Chakradhar886
marked this pull request as ready for review
July 29, 2026 11:07
Chakradhar886
requested review from
Justin Firsching (JustinFirsching),
Klein Hu (NonStatic2014),
Amit Chauhan (achauhan-scc),
Arun (arunsu),
Jayesh Tanna (jayesh-tanna),
kingernupur,
Nikolay Rovinskiy (nick863),
Ayush Mishra (novaturient95),
Razvan Tanase (rtanase),
Riti Sharma (sharma-riti) and
Vivek Ramaswamy (vivram)
as code owners
July 29, 2026 11:08
|
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. |
Contributor
There was a problem hiding this comment.
Pull request overview
Pins online endpoint and deployment operations to workspace scope while preserving registry scope for assets.
Changes:
- Adds workspace-scoped online operation clients and scopes.
- Adds regression coverage for mixed workspace/registry configuration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
azure/ai/ml/_ml_client.py |
Routes online operations through workspace-scoped clients. |
tests/internal_utils/unittests/test_ml_client.py |
Tests workspace and registry scope isolation. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
sdk/ml/azure-ai-ml/tests/conftest.py:496
- The end-to-end regression likewise exercises
registry_reference+workspace_name, rather than theregistry_name+workspace_referenceconfiguration described by this fix. These forms take different scope-selection branches, so the test can pass while the reported scenario is broken.
workspace_name=e2e_ws_scope.workspace_name,
registry_reference="sdkv2-testFeed",
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:546
- This test does not exercise the reported
registry_name+workspace_referenceconstruction path. It instead usesregistry_reference+workspace_name, which follows different conditionals for asset operation scopes (for example,DataOperationsremains workspace-scoped in that path). Use the customer-facing configuration so a regression in the branch fixed by this PR is detected.
workspace_name=workspace_name,
registry_reference="test-registry",
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:565
- The regression criteria say both model and data operations must retain registry scope, but this test only asserts the model scope. Capture and verify
DataOperationstoo; this also protects the asset-scope half of the isolation contract.
model_scope = captured["ModelOperations"]["scope"]
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:546
- This test reverses the configuration from the reported regression. Passing
registry_referencemade the pre-change online scope select_ws_operation_scope, so it does not reproduce the wrong-resource-group path caused byregistry_namewithworkspace_reference. Construct the client with those arguments so the test fails on the old implementation and validates the intended fix.
workspace_name=workspace_name,
registry_reference="test-registry",
sdk/ml/azure-ai-ml/tests/conftest.py:496
- The end-to-end fixture also uses the inverse configuration from the failing scenario. With
registry_reference, the old code already selected the workspace operation scope; useregistry_nameplusworkspace_referenceto exercise the cross-resource-group regression described by this PR.
workspace_name=e2e_ws_scope.workspace_name,
registry_reference="sdkv2-testFeed",
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:564
- The regression coverage described for this PR also requires verifying that data operations remain registry-scoped, but the test only checks models. Add corresponding data-scope assertions; this will also guard the scope isolation for both asset operation types.
assert model_scope.subscription_id == registry_sub
assert model_scope.resource_group_name == registry_rg
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:565
- The advertised regression configuration is
registry_nameplusworkspace_reference, but this test uses the inverse pairing; those paths initialize asset scopes differently, and the promisedDataOperationsregistry-scope assertion is also absent. Exercise the reported configuration directly and assert that data operations remain registry-scoped.
MLClient(
credential=mock_credential,
subscription_id=workspace_sub,
resource_group_name=workspace_rg,
workspace_name=workspace_name,
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:546
- This setup exercises the
registry_referencepath, whose online operation scope was already selected from_ws_operation_scopebefore this change. The reported configuration isregistry_namewithworkspace_reference, so this test would still pass if the newregistry_namebranch in_online_operation_scoperegressed. Construct the client with that argument pair and omitworkspace_name, which is mutually exclusive withregistry_name.
workspace_name=workspace_name,
registry_reference="test-registry",
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:564
- The advertised regression coverage also requires data operations to remain registry-scoped, but
DataOperationsis captured and never asserted. Add its subscription/resource-group checks; with the intendedregistry_name+workspace_referencesetup, these assertions also guard against accidentally shifting asset operations back to workspace scope.
assert model_scope.subscription_id == registry_sub
assert model_scope.resource_group_name == registry_rg
sdk/ml/azure-ai-ml/tests/conftest.py:496
- This E2E fixture uses
registry_reference, for which the endpoint/deployment operation scope was already workspace-scoped before this PR. It therefore does not reproduce the reported wrong-resource-group path, which is reached by combiningregistry_namewithworkspace_reference. Configure that pair here so the E2E test actually validates the newregistry_namescope isolation.
workspace_name=e2e_ws_scope.workspace_name,
registry_reference="sdkv2-testFeed",
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:564
- The PR states that both model and data operations remain registry-scoped, but only the model scope is asserted.
DataOperationsis wired through a separate conditional inMLClient, so add explicit assertions to prevent its scope from silently changing. These assertions should be used with theregistry_name+workspace_referencesetup above.
assert model_scope.subscription_id == registry_sub
assert model_scope.resource_group_name == registry_rg
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:546
- This regression test exercises
workspace_name+ internalregistry_reference, not the reportedregistry_name+workspace_referenceconfiguration. Those paths behaved differently before this change:registry_referencealready selected_ws_operation_scope, so this test does not reproduce the wrong-resource-group bug. Instantiate the client with the exact public configuration so the test fails if that branch regresses.
workspace_name=workspace_name,
registry_reference="test-registry",
Contributor
[Pilot] PR Pipeline Failure AnalysisA CI pipeline failed on this pull request. Here is an automated analysis of what went wrong and how to get the build green. What failedThe This is a test failure — the end-to-end test Recommended next steps
Raw pipeline analysis (azsdk ci analyze)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
sdk/ml/azure-ai-ml/tests/internal_utils/unittests/test_ml_client.py:565
- This regression test constructs the alternate
workspace_name+registry_referencepath, not the reportedregistry_name+workspace_referenceconfiguration. Those paths differ here: only the latter must copyworkspace_referenceinto the online scope, and it also keepsDataOperationsregistry-scoped. As written, the test would still pass if that exact path regressed, and it omits the promised data-scope assertion. Construct the reported configuration and assert the captured data scope as well.
MLClient(
credential=mock_credential,
subscription_id=workspace_sub,
resource_group_name=workspace_rg,
workspace_name=workspace_name,
sdk/ml/azure-ai-ml/tests/conftest.py:506
- This E2E fixture also uses
workspace_name+registry_reference, for which online operations were already given the workspace resource-group scope before this PR. It therefore does not exercise the reported wrong-resource-group path (registry_name+workspace_reference) through the real online calls. Build the client with the affected argument combination so this test regresses if workspace scope isolation is removed.
subscription_id=e2e_ws_scope.subscription_id,
resource_group_name=e2e_ws_scope.resource_group_name,
workspace_name=e2e_ws_scope.workspace_name,
registry_reference="sdkv2-testFeed",
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The documented client construction is rejected by the API, and the claimed data-operation registry scope is neither implemented nor tested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
| online_deployment_scope = captured["OnlineDeploymentOperations"]["scope"] | ||
| online_endpoint_client = captured["OnlineEndpointOperations"]["args"][2] | ||
| online_deployment_client = captured["OnlineDeploymentOperations"]["args"][2] | ||
| model_scope = captured["ModelOperations"]["scope"] |
Comment on lines
+284
to
+285
| if registry_name or registry_reference | ||
| else self._operation_scope |
This branch has not been deployed
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.
Online deployment requests could be sent to the registry resource group when a client was initialized to consume registry assets and target a workspace in a different resource group. In that configuration, endpoint/deployment operations looked up the workspace endpoint under the wrong RG and failed with
ResourceNotFound.Scope isolation for online operations
OperationScopefor online endpoint and online deployment operations.Dedicated service clients for online calls
OnlineEndpointOperationsandOnlineDeploymentOperationsthrough those workspace-scoped clients instead of the registry-shifted shared scope.Regression coverage
MLClientwith a registry plusworkspace_referenceand asserts:Behavioral effect
Testing
Samples validations: Azure/azureml-examples#4110