Skip to content

Adds CodeGen to master (from features/automl) #4365

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
Merged
merged 1 commit into from
Oct 25, 2019
Merged

Adds CodeGen to master (from features/automl) #4365

merged 1 commit into from
Oct 25, 2019

Conversation

@maryamariyan
Copy link
Contributor

@maryamariyan maryamariyan commented Oct 22, 2019 •
edited
Loading

  • Add src project for CodeGen
  • Add test project for CodeGen
  • Add packaging for CodeGen

cc: @eerhardt @LittleLittleCloud
@maryamariyan maryamariyan self-assigned this Oct 22, 2019
maryamariyan
maryamariyan commented Oct 22, 2019
View reviewed changes
@maryamariyan maryamariyan changed the title [WIP] Adds CodeGen to master (from features/automl) Adds CodeGen to master (from features/automl) Oct 22, 2019
eerhardt
eerhardt reviewed Oct 22, 2019
View reviewed changes
eerhardt
eerhardt reviewed Oct 22, 2019
View reviewed changes
eerhardt
eerhardt reviewed Oct 22, 2019
View reviewed changes
eerhardt
eerhardt reviewed Oct 22, 2019
View reviewed changes
eerhardt
eerhardt reviewed Oct 22, 2019
View reviewed changes
@maryamariyan maryamariyan marked this pull request as ready for review October 22, 2019 23:55
@maryamariyan maryamariyan requested review from a team as code owners October 22, 2019 23:55
eerhardt
eerhardt reviewed Oct 23, 2019
View reviewed changes
eerhardt
eerhardt reviewed Oct 23, 2019
View reviewed changes
eerhardt
eerhardt reviewed Oct 23, 2019
View reviewed changes
@eerhardt
Copy link
Member

eerhardt commented Oct 23, 2019

<#@ template language="C#" #>

We should put a visibility="internal" on these, so the class that is generated is internal instead of public.

<#@ template language="C#" visibility="internal" #>

Refers to: src/Microsoft.ML.CodeGenerator/Templates/Console/ConsumeModel.tt:1 in a98fa15. [](commit_id = a98fa15, deletion_comment = False)
@maryamariyan
Copy link
Contributor Author

maryamariyan commented Oct 23, 2019 •
edited
Loading

I'm looking at this comment: #4365 (comment)

Update: addressed in 2f894f4
eerhardt
eerhardt reviewed Oct 23, 2019
View reviewed changes
eerhardt
eerhardt reviewed Oct 23, 2019
View reviewed changes
sharwell
sharwell suggested changes Oct 23, 2019
View reviewed changes
src/Microsoft.ML.CodeGenerator/Templates/Console/Annotation.tt
{#>
//*****************************************************************************************
//* *
//* This is an auto-generated file by Microsoft ML.NET CLI (Command-Line Interface) tool. *
Copy link
Contributor

@sharwell sharwell Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ This is missing the <auto-generated/> comment, which means code style analyzer diagnostics will be reported in the file even though it is generated.
Copy link
Member

@eerhardt eerhardt Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

My thinking is that these code files aren't true "auto generated" files. I expect users to update them. Especially considering that we are putting hard-coded paths in some of the files, and we are making public types, that I assume most users will want to make internal. Or, if they do want them to be public - they would want to put XML docs on them.

So what is the appropriate pattern here? I think it is similar to "scaffolding" in ASP.NET. Or even "Add -> new Item" in VS. Do they put any "this was generated" comments at the top there?
Copy link
Contributor

@sharwell sharwell Oct 24, 2019

Choose a reason for hiding this comment

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

For this case, the ideal header would be more like a template, with links explaining what the file is and where to find documentation of expected changes users would need to make. You would not need the <auto-generated/> comment for templates (one-time code generation).
Copy link
Member

@eerhardt eerhardt Oct 24, 2019

Choose a reason for hiding this comment

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

@maryamariyan - can you log an issue to address this? I think we should make that change separately and we will need @JakeRadMSFT @LittleLittleCloud and @rustd input as well.
Copy link
Contributor Author

@maryamariyan maryamariyan Oct 25, 2019

Choose a reason for hiding this comment

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

Created #4387
src/Microsoft.ML.CodeGenerator/Templates/Console/Annotation.tt
<#+
void MB_Annotation()
{#>
// This file was auto-generated by ML.NET Model Builder.
Copy link
Contributor

@sharwell sharwell Oct 23, 2019

Choose a reason for hiding this comment

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

⚠️ This is missing the <auto-generated/> comment, which means diagnostics will be reported in the file even though it is generated.
eerhardt
eerhardt reviewed Oct 24, 2019
View reviewed changes
eerhardt
eerhardt approved these changes Oct 24, 2019
View reviewed changes
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.

I think this change is ready to be merged.

We can address @sharwell's concern in a follow-up change. But let's not let that block us from moving this code from features/automl into the master branch. It is an existing issue in the code, and there isn't a clear direction we should take on it right now.
@LittleLittleCloud LittleLittleCloud self-requested a review October 24, 2019 18:52
@codecov
Copy link

codecov bot commented Oct 24, 2019 •
edited
Loading

Codecov Report

Merging #4365 into master will increase coverage by <.01%.
The diff coverage is 74.36%.

@@ Coverage Diff @@ ## master #4365 +/- ## ========================================== + Coverage 74.65% 74.65% +<.01%  ========================================== Files 883 903 +20 Lines 155117 158562 +3445 Branches 16931 17136 +205 ========================================== + Hits 115795 118378 +2583  - Misses 34572 35362 +790  - Partials 4750 4822 +72
Flag Coverage Δ
#Debug 74.65% <74.36%> (ø) ⬆️
#production 70.11% <64.23%> (-0.11%) ⬇️
#test 89.84% <100%> (+0.28%) ⬆️
Impacted Files Coverage Δ
...tor/CodeGenerator/CSharp/TransformGeneratorBase.cs 100% <100%> (ø)
...r.Tests/ApprovalTests/ConsoleCodeGeneratorTests.cs 100% <100%> (ø)
....ML.CodeGenerator.Tests/TransformGeneratorTests.cs 100% <100%> (ø)
...t/Microsoft.ML.CodeGenerator.Tests/CodeGenTests.cs 100% <100%> (ø)
...ator/CodeGenerator/CSharp/CodeGeneratorSettings.cs 100% <100%> (ø)
...t.ML.CodeGenerator/CodeGenerator/CSharp/Symbols.cs 100% <100%> (ø)
...ft.ML.CodeGenerator.Tests/TrainerGeneratorTests.cs 100% <100%> (ø)
src/Microsoft.ML.CodeGenerator/Utils.cs 18.3% <18.3%> (ø)
...ML.CodeGenerator/Templates/Console/ModelProject.cs 29.29% <29.29%> (ø)
...odeGenerator/Templates/Console/ModelOutputClass.cs 35.19% <35.19%> (ø)
... and 33 more
eerhardt
eerhardt reviewed Oct 24, 2019
View reviewed changes
@maryamariyan
Copy link
Contributor Author

maryamariyan commented Oct 25, 2019

NOTE:
@eerhardt @LittleLittleCloud since you both had already approved the PR I just squash rebased it with master, to get the latest changes from #4377.

The diff should be the same as you both reviewed.
@maryamariyan maryamariyan mentioned this pull request Oct 25, 2019
Closed
justinormont
justinormont approved these changes Oct 25, 2019
View reviewed changes
Copy link
Contributor

@justinormont justinormont left a comment

Choose a reason for hiding this comment

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

LGTM
@codemzs codemzs merged commit 4f6de52 into dotnet:master Oct 25, 2019
frank-dong-ms-zz pushed a commit to frank-dong-ms-zz/machinelearning that referenced this pull request Nov 4, 2019
@maryamariyan @frank-dong-ms
Brings CodeGen to master - Squash all changes to one commit (dotnet#4365
f04ead6 
) All legs are passing except for one which appears to be a random failure and likely will not break the CI. This PR is blocking a few other important PRs that need to be in today hence I'm merging it. Spoke with @eerhardt who also agrees is ok to merge it with one leg still building and all greens.
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

6 participants

@maryamariyan @eerhardt @sharwell @justinormont @LittleLittleCloud @codemzs