Refactor Azure authentication and service calls #164
Conversation
Co-authored-by: Chris Granade <chgranad@microsoft.com>
Co-authored-by: Chris Granade <chgranad@microsoft.com>
Co-authored-by: Chris Granade <chgranad@microsoft.com>
| { | ||
| Logger?.LogError(e, $"Failed to download providers list from Azure Quantum workspace: {e.Message}"); | ||
| return AzureClientError.WorkspaceNotFound.ToExecutionResult(); | ||
| } |
There was a problem hiding this comment.
This should be a different error, as there is a workspace...
There was a problem hiding this comment.
Yes, we have created the "workspace" object on the client, but downloading the providers list is the first time we actually attempt to communicate with Azure Quantum. For example, if the user has mistyped the workspace name, this is the error they will see. The friendly string displayed is: "No Azure Quantum workspace was found that matches the specified criteria."
| { | ||
| if (QuantumClient == null || AvailableProviders == null) | ||
| if (ActiveWorkspace == null || AvailableProviders == null) | ||
| { |
There was a problem hiding this comment.
I wonder if it's the right message that you can't connect to a workspace unless it has Q# providers.
There was a problem hiding this comment.
The behavior in this case is that the connection to the workspace will succeed, but there will simply be no valid targets to choose from. But it's a good point, and I'll add an output message to make this clear. It would still be possible, for example, to query the jobs list in this case.
| using Microsoft.Azure.Quantum.Client; | ||
| using Microsoft.Identity.Client; | ||
| using Microsoft.Identity.Client.Extensions.Msal; | ||
| using Microsoft.Jupyter.Core; |
There was a problem hiding this comment.
This code looks great.
Wondering if we should move it to be part of the Azure.Quantum.Client package itself. Pretty much the same should be done wherever we try to call Azure Quantum.
There was a problem hiding this comment.
Agreed, I had the same thought while I was writing it. There are a few Jupyter dependencies sprinkled in, and I want some of these abstractions in IQ# anyway in order to easily mock them, but a lot of this can and probably should be moved into Azure.Quantum.Client. I'll create a task for this.
This PR contains no functional changes. It is purely refactoring of authentication and service calls out of the
AzureClientclass into the existingAzureEnvironmentclass and a newAzureWorkspaceclass that implements a newIAzureWorkspaceinterface.In a subsequent PR, I plan to add
AzureEnvironmentType.Mock, which will use a mock implementation of this newIAzureWorkspaceinterface. This should enable end-to-end integration testing of most of the functionality without any Azure dependency.Fixes AB#15335, fixes AB#15316.