Adding Machine Learning Compute cmdlets #4681

shutchings · 2017-09-28T16:58:00Z

Description

This is an approved PR for the 4.5.0 release. These cmdlet's were supposed to make it in to the 4.4.0 release but didn't quite make it. I've been working closely with @markcowl and @cormacpayne for my reviews and feedback. This was the original PR. A lot of feedback was incorporated into it but @cormacpayne closed it in favor of this PR where he continued to merge some of the recent changes from preview.

The .NET SDK was merged to the public repo in this PR. The nuget package has been published and these cmdlets are not using a local package.

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

shutchings · 2017-09-28T16:59:44Z

Looks like @cormacpayne and @markcowl are OOF until the end of the week. Is someone stepping in for them?

azuresdkci · 2017-09-28T17:00:08Z

Can one of the admins verify this patch?

cormacpayne · 2017-09-28T20:53:36Z

@azuresdkci add to whitelist

cormacpayne

@shutchings a few minor comments.

Unsigned packing job to ensure your module has been included in AzureRM and that your cmdlets appear: http://azuresdkci.cloudapp.net/job/posh-pack/5/

cormacpayne · 2017-10-03T00:36:46Z

ChangeLog.md

@@ -1,4 +1,13 @@
-## 2017.09.25 - Version 4.4.0


@shutchings please revert the changes made to this file, we will handle updating the ChangeLog in the root of the repository during the release

Sounds good!

cormacpayne · 2017-10-03T00:38:02Z

src/ResourceManager/MachineLearningCompute/Commands.MachineLearningCOmpute/app.config

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="utf-8"?>


@shutchings there should be no app.config file for the Commands folder

Got it. I think I had it because of some dependency issues but it looks like those got cleaned up in past revisions. Probably from the changes you made.

cormacpayne · 2017-10-03T00:40:56Z

...achineLearningCompute/Commands.MachineLearningCompute/Commands.MachineLearningCompute.csproj

+      <Link>AzureRM.MachineLearningCompute.psd1</Link>
+      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
+    </None>
+    <None Include="app.config" />


@shutchings please remove the reference to app.config

shutchings · 2017-10-03T18:13:30Z

@cormacpayne I installed the package and tested the cmdlets and things look good.

shutchings · 2017-10-04T16:45:19Z

@cormacpayne @markcowl Looks like today is when it's supposed to be code complete. Any additional changes?

cormacpayne

@shutchings a few minor comments that need looking at. Code complete has been moved to Friday, so you have a little bit of time to get these changes in.

cormacpayne · 2017-10-04T21:51:09Z

...achineLearningCompute/Commands.MachineLearningCompute/Commands.MachineLearningCompute.csproj

+    <Reference Include="Microsoft.Azure.Management.MachineLearningCompute, Version=0.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
+      <HintPath>..\..\..\packages\Microsoft.Azure.Management.MachineLearningCompute.0.1.0\lib\net452\Microsoft.Azure.Management.MachineLearningCompute.dll</HintPath>
+    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL" />


@shutchings Microsoft.Rest.ClientRuntime.* and Newtonsoft.Json are already being imported from Common.Dependencies.targets, so these references are not needed in your csproj.

cormacpayne · 2017-10-04T22:00:03Z

...r/MachineLearningCompute/Commands.MachineLearningCompute/Cmdlets/GetAzureRmMlOpClusterKey.cs

+namespace Microsoft.Azure.Commands.MachineLearningCompute.Cmdlets
+{
+    [Cmdlet(VerbsCommon.Get, CmdletSuffix + "Key")]
+    [OutputType(typeof(OperationalizationClusterCredentials))]


@shutchings can we create a new class that is PSOperationalizationClusterCredentials and return that instead? This will prevent issues with potential breaking changes in the future.

cormacpayne · 2017-10-04T22:01:03Z

...r/MachineLearningCompute/Commands.MachineLearningCompute/Cmdlets/GetAzureRmMlOpClusterKey.cs

+
+            try
+            {
+                WriteObject(MachineLearningComputeManagementClient.OperationalizationClusters.ListKeys(ResourceGroupName, Name));


@shutchings if this is returning more than one object, make sure to use the WriteObject overload with enumerateCollection:true

This method only ever returns one object that wraps up multiple keys.

cormacpayne · 2017-10-04T22:03:44Z

...eLearningCompute/Commands.MachineLearningCompute/Cmdlets/MachineLearningComputeCmdletBase.cs

+        {
+            var exceptionDetails = e.Message + ".";
+
+            foreach (var err in e.Body.Details)


@shutchings is the body ever null for an error? I want to make sure we avoid the scenario where e.Body.Details throws an error because e.Body is null.

Also, would we need to do any null-checking for e.Body.Details?

Good points! I have to do this extra handling for the different types of errors that the SDK returns so that the error message to the user isn't just "Bad Request" or "Not Found". I've settled on trying to unwrap the errors as best as I can but I'll throw the original error if I have issues doing that. That way the user will always still get the original error, or a more descriptive one, and not an error due to the unwrapping.

In my experimentation the body isn't null but the details can be at times. I've added the additional checks.

cormacpayne · 2017-10-04T22:07:14Z

...nds.MachineLearningCompute/Cmdlets/TestAzureRmMlOpClusterSystemServicesUpdateAvailability.cs

+namespace Microsoft.Azure.Commands.MachineLearningCompute.Cmdlets
+{
+    [Cmdlet(VerbsDiagnostic.Test, CmdletSuffix + "SystemServicesUpdateAvailability")]
+    [OutputType(typeof(CheckSystemServicesUpdatesAvailableResponse))]


@shutchings same comment about wrapping this in a PS class

cormacpayne · 2017-10-04T22:07:56Z

...ningCompute/Commands.MachineLearningCompute/Cmdlets/UpdateAzureRmMlOpClusterSystemService.cs

+{
+
+    [Cmdlet(VerbsData.Update, CmdletSuffix + "SystemService", SupportsShouldProcess = true)]
+    [OutputType(typeof(UpdateSystemServicesResponse))]


@shutchings same comment about wrapping this in a PS class

cormacpayne · 2017-10-04T22:13:20Z

src/ResourceManager/MachineLearningCompute/Commands.MachineLearningCompute/packages.config

+<?xml version="1.0" encoding="utf-8"?>
+<packages>
+  <package id="Microsoft.Azure.Management.MachineLearningCompute" version="0.1.0" targetFramework="net452" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.10" targetFramework="net452" />


@shutchings remove same packages outlined in csproj

cormacpayne · 2017-10-06T16:29:07Z

On-demand: http://azuresdkci.cloudapp.net/job/powershell-demand/1899
Packaging job: http://azuresdkci.cloudapp.net/job/posh-pack/21/

shutchings · 2017-10-06T18:28:42Z

@cormac The cmdlets in the package job look good.

shutchings added 2 commits September 27, 2017 13:27

Added machine learning compute.

28d0b9a

Updated wxi file.

371c3f9

shutchings requested review from markcowl and cormacpayne September 28, 2017 16:58

azuresdkci added the needs-review label Sep 28, 2017

shutchings force-pushed the preview branch from 9ad7055 to 86e0301 Compare September 29, 2017 17:32

cormacpayne suggested changes Oct 3, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Oct 3, 2017

cormacpayne assigned shutchings Oct 3, 2017

Removed local nuget package.

faec0ca

shutchings force-pushed the preview branch from 86e0301 to faec0ca Compare October 3, 2017 16:49

cormacpayne suggested changes Oct 4, 2017

View reviewed changes

shutchings added 4 commits October 5, 2017 12:02

Made exception unwrapping more robust.

d88cd12

Added more PS output object types.

74123cc

Removed sandbox test and extra packages.

7689e3f

Updated help documentation.

cbbe7bf

cormacpayne assigned cormacpayne and unassigned shutchings Oct 6, 2017

cormacpayne added needs-review and removed needs-revision labels Oct 6, 2017

cormacpayne approved these changes Oct 6, 2017

View reviewed changes

cormacpayne merged commit ad4b8f3 into Azure:preview Oct 6, 2017

Adding Machine Learning Compute cmdlets #4681

Adding Machine Learning Compute cmdlets #4681

Uh oh!

Conversation

shutchings commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

shutchings commented Sep 28, 2017

Uh oh!

azuresdkci commented Sep 28, 2017

Uh oh!

cormacpayne commented Sep 28, 2017

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shutchings commented Oct 3, 2017

Uh oh!

shutchings commented Oct 4, 2017

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shutchings Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne commented Oct 6, 2017

Uh oh!

shutchings commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shutchings commented Sep 28, 2017 •

edited

Loading

shutchings Oct 5, 2017 •

edited

Loading

shutchings commented Oct 6, 2017 •

edited

Loading