Skip to content

Add a DNN Image Featurizer Transform Estimator #1447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 56 commits into from
Nov 27, 2018

Conversation

vaeksare
Copy link
Member

@vaeksare vaeksare commented Oct 30, 2018

Adds a new estimator for doing pretrained DNN model image featurization as a transform. This estimator calls 2 ONNX transforms in a chain in order to achieve the desired result. The first ONNX model does the preprocessing while the second one is the actual pretrained DNN. A total of 4 DNN models are available:

  • AlexNet
  • ResNet18
  • ResNet50
  • ResNet101

Each model is available through its own project and a related extension method. Building that project automatically downloads the model from a CDN. The model is included with the NuGet package produced by that project when the code is distributed. Fixes #1232

@vaeksare vaeksare self-assigned this Oct 30, 2018
@justinormont
Copy link
Contributor

In addition to the estimator, would it be possible make the transform callable from the command-line and GUIs? Perhaps the shell of code could be copied from the internal repo and your new implementation replaces its innards?

@vaeksare
Copy link
Member Author

vaeksare commented Nov 1, 2018

I believe command-line is expected to be changed to use the estimators as a base in the near future. And ML.NET itself currently does not have a GUI. So adding an actual transform is certainly possible, but I don't think it is actually very useful.


In reply to: 434928031 [](ancestors = 434928031)

while (cursor.MoveNext())
{
getter(ref buffer);
Assert.Equal(512, buffer.Length);
Copy link
Member

@wschin wschin Nov 6, 2018

Choose a reason for hiding this comment

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

Need more strict asserts like checking the prediction values. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

See the new OldSavingAndLoading test which contains some exact output value checks.


In reply to: 231254842 [](ancestors = 231254842)

/// This is an extension method to be used with the <see cref="DnnImageFeaturizerEstimator"/> in order to use a pretrained AlexNet model.
/// The NuGet containing this extension is also guaranteed to include the binary model file.
/// </summary>
public static class AlexNetExtension
Copy link
Member

@eerhardt eerhardt Nov 26, 2018

Choose a reason for hiding this comment

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

AlexNetExtension [](start = 24, length = 16)

(nit) it is nice to keep the class name and the containing file name in sync.

AlexNetExtension vs AlexNet.cs #Resolved

<UsingTask TaskName="DownloadFilesFromUrl" AssemblyFile="$(ToolsDir)Microsoft.DotNet.Build.Tasks.dll"/>

<PropertyGroup>
<ModelPlacementDir>$(RepoRoot)bin\obj\DnnImageModels</ModelPlacementDir>
Copy link
Member

@eerhardt eerhardt Nov 26, 2018

Choose a reason for hiding this comment

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

Use $(ObjDir) instead of $(RepoRoot)bin\obj #Closed

/// </summary>
public sealed class DnnImageFeaturizerInput
{
public IHostEnvironment Env { get; }
Copy link
Member

@eerhardt eerhardt Nov 26, 2018

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, renamed it!


In reply to: 236390424 [](ancestors = 236390424)

<Content Include="$(MSBuildThisFileDirectory)..\..\tools\DnnImageFeaturizer\**\*.*">
<Link>DnnImageModels\%(RecursiveDir)%(Filename)%(Extension)</Link>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<CopyToPublishDirectory>PreserveNewest</CopyToPublishDirectory>
Copy link
Member

@eerhardt eerhardt Nov 27, 2018

Choose a reason for hiding this comment

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

You don't need CopyToPublishDirectory. It gets defaulted to CopyToOutputDirectory when it isn't set. #Resolved

@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Member

@eerhardt eerhardt Nov 27, 2018

Choose a reason for hiding this comment

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

I think this should be a .props file instead.

The difference is that a .props file gets imported BEFORE an importing .csproj gets executed. Which means that if a user wanted to modify the Content item below, they can in their .csproj without writing a <Target> #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, changed it, thanks!


In reply to: 236760593 [](ancestors = 236760593)

<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<ItemGroup>
<Content Include="$(MSBuildThisFileDirectory)..\..\tools\DnnImageFeaturizer\**\*.*">
Copy link
Member

@eerhardt eerhardt Nov 27, 2018

Choose a reason for hiding this comment

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

Why is this folder named DnnImageFeaturizer, but the Link uses DnnImageModels? I think they should be consistent. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed for consistency.


In reply to: 236760874 [](ancestors = 236760874)

DestinationDir="$(ModelPlacementDir)\ResNet101Onnx" />
</ItemGroup>

<Target Name="DownloadDnnModelFiles"
Copy link
Member

Choose a reason for hiding this comment

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

This looks much cleaner. Nice work.

@@ -0,0 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
Copy link
Member

Choose a reason for hiding this comment

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

(nit) you can drop DefaultTargets="Build" it doesn't make sense here. (and doesn't do anything)

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for the great work!

@vaeksare vaeksare merged commit f92c90d into dotnet:master Nov 27, 2018
@vaeksare vaeksare deleted the dnn-image-feat branch November 27, 2018 18:36
@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants