Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[AutoML] Pull out Code Gen as separate library plus some changes in CodeGen #4043

Merged
merged 23 commits into from
Aug 25, 2019
Merged

[AutoML] Pull out Code Gen as separate library plus some changes in CodeGen #4043

merged 23 commits into from
Aug 25, 2019

Conversation

LittleLittleCloud
Copy link
Contributor

@LittleLittleCloud LittleLittleCloud commented Jul 26, 2019 •
edited
Loading

We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

Related Issue

@LittleLittleCloud LittleLittleCloud changed the title Pull out Code Gen as separate library WIP - Pull out Code Gen as separate library Jul 26, 2019
@LittleLittleCloud LittleLittleCloud changed the title WIP - Pull out Code Gen as separate library [AutoML] WIP - Pull out Code Gen as separate library Jul 26, 2019
@LittleLittleCloud
Copy link
Contributor Author

LittleLittleCloud commented Jul 26, 2019 •
edited
Loading

  • pull out CodeGen
  • use CodeGen in MLNet cli
  • create private feed for Model Builder on CodeGen

@Dmitry-A Dmitry-A added the AutoML.NET Automating various steps of the machine learning process label Jul 26, 2019
@justinormont justinormont requested review from srsaggam, Dmitry-A and daholste and removed request for srsaggam July 29, 2019 06:28
@LittleLittleCloud LittleLittleCloud force-pushed the features/automl branch 4 times, most recently from 5a705ba to 6189d72 Compare July 31, 2019 00:14
Dmitry-A
Dmitry-A suggested changes Aug 1, 2019
View reviewed changes
src/Microsoft.ML.CodeGen/Assembly.cs Show resolved Hide resolved
src/Microsoft.ML.CodeGen/CodeGenerator/CSharp/CodeGenerator.cs Outdated Show resolved Hide resolved
src/Microsoft.ML.CodeGen/Microsoft.ML.CodeGen.csproj Outdated Show resolved Hide resolved
src/Microsoft.ML.CodeGen/mlnet.csproj Outdated Show resolved Hide resolved
Microsoft.ML.AutoML.sln Outdated Show resolved Hide resolved
@LittleLittleCloud LittleLittleCloud changed the title [AutoML] WIP - Pull out Code Gen as separate library [AutoML] Pull out Code Gen as separate library Aug 2, 2019
@LittleLittleCloud LittleLittleCloud changed the title [AutoML] Pull out Code Gen as separate library [AutoML] Pull out Code Gen as separate library plus some changes in CodeGen Aug 8, 2019
@dotnet dotnet deleted a comment from LittleLittleCloud Aug 19, 2019
@eerhardt
Copy link
Member

I don't think you need an NLog.config file for a library. That should only be at the app level, I think.

Refers to: src/Microsoft.ML.CodeGen/NLog.config:1 in a847f05. [](commit_id = a847f05, deletion_comment = False)

@srsaggam
Copy link
Member

srsaggam commented Aug 19, 2019 •
edited
Loading

Could you run a smoke test with different datasets per task type. binary, multi and regression. and check the generated projects can compile and run and give meaningful results before closing in on this PR

@Dmitry-A Dmitry-A self-requested a review August 19, 2019 21:17
@LittleLittleCloud
Copy link
Contributor Author

Could you run a smoke test with different datasets per task type. binary, multi and regression. and check the generated projects can compile and run and give meaningful results before closing in on this PR

  • binary
  • multi
  • regression

justinormont
justinormont reviewed Aug 23, 2019
View reviewed changes
src/Microsoft.ML.CodeGen/CodeGenerator/CSharp/CodeGeneratorSettings.cs Outdated
{
public CodeGeneratorSettings()
{
// set default value
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment style in ML.NET is to begin comment with an uppercase character

Suggested change
// set default value
// Set default value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#resolved

justinormont
justinormont reviewed Aug 23, 2019
View reviewed changes
src/Microsoft.ML.CodeGen/CodeGenerator/CSharp/CodeGeneratorSettings.cs

}

public enum GenerateTarget
Copy link
Contributor

Choose a reason for hiding this comment

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

This seem to be used only to print the right annotation for the platform. I might recommend just having a string field here for the annotation text.

This would address my other comment: https://github.com/dotnet/machinelearning/pull/4043/files#r317240647

justinormont
justinormont reviewed Aug 23, 2019
View reviewed changes
src/Microsoft.ML.CodeGen/Templates/Console/Annotation.txt
@@ -0,0 +1 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this empty file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it's generated by T4... I will remove that

justinormont
justinormont reviewed Aug 23, 2019
View reviewed changes
src/Microsoft.ML.CodeGen/Templates/Console/ModelProject.tt
@@ -6,7 +6,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netcoreapp2.1</TargetFramework>
<TargetFramework>netstandard2.0</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this moved from core to standard? Does this allow the generated project to exist in a wider set of projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because ML.Net target netstandard 2.0 and we want to keep in pace with that.
@JakeRadMSFT is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this makes it so .NET Framework and .NET Core projects can reference the model project.

@LittleLittleCloud LittleLittleCloud merged commit 6f3d26c into dotnet:features/automl Aug 25, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@justinormont justinormont justinormont left review comments

@eerhardt eerhardt eerhardt left review comments

@briacht briacht briacht left review comments

@srsaggam srsaggam srsaggam left review comments

@JakeRadMSFT JakeRadMSFT JakeRadMSFT approved these changes

@Dmitry-A Dmitry-A Dmitry-A approved these changes

@daholste daholste Awaiting requested review from daholste

Assignees
No one assigned
Labels
AutoML.NET Automating various steps of the machine learning process
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@LittleLittleCloud @JakeRadMSFT @eerhardt @srsaggam @justinormont @briacht @Dmitry-A