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.

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

Refactor official build to use arcade templates #6412

Merged
merged 25 commits into from
Oct 27, 2022

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Oct 27, 2022

This refactors the official build to use the arcade templates which automate a lot of validation tasks. It also removes some redundant signing we were doing before, opting for a single signing phase after all packages have been built.

Here's a successful build from this change https://dev.azure.com/dnceng/internal/_build/results?buildId=2030382&view=results

@@ -1,7 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<Import Project="$(RepoRoot)eng/pkg/Pack.props" />
<PropertyGroup>
<Authors>Intel</Authors>
Copy link
Member

Choose a reason for hiding this comment

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

Intel

I assume this is ok and no concern removing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was wrong for us to claim Intel is the Author of the package. The gallery doesn't even present it to our users.
https://www.nuget.org/packages/Microsoft.ML.Mkl.Redist/2.0.0-preview.22313.1
image
We are the ones who build this package and binary, it's a custom linked copy of MKL. The package itself has the Intel license.

<Target Name="Test" />

<ItemGroup>
<PackageDownload Include="MlNetMklDeps" Version="[$(MlNetMklDepsVersion)]" />
Copy link
Member

Choose a reason for hiding this comment

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

[

interesting, does the version included inside brackets?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, PackageDownload requires explicit version references. https://learn.microsoft.com/en-us/nuget/consume-packages/packagedownload-functionality

@tarekgh
Copy link
Member

tarekgh commented Oct 27, 2022

@LittleLittleCloud Any idea about this failure?

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-machinelearning-refs-pull-6412-merge-b739effdcc32498c89/Microsoft.ML.AutoML.Tests/1/console.8ab99b89.log?helixlogtype=result

 ---> System.ObjectDisposedException: The CancellationTokenSource has been disposed.
   at System.Threading.CancellationTokenSource.Cancel()
   at Microsoft.ML.AutoML.AutoMLExperiment.<>c__DisplayClass26_1.<RunAsync>g__handler|4(Object o, EventArgs e) in /__w/1/s/src/Microsoft.ML.AutoML/AutoMLExperiment/AutoMLExperiment.cs:line 270
   at Microsoft.ML.AutoML.AggregateTrainingStopManager.<.ctor>b__4_0(Object o, EventArgs e) in /__w/1/s/src/Microsoft.ML.AutoML/AutoMLExperiment/IStopTrainingManager.cs:line 129
   at Microsoft.ML.AutoML.TimeoutTrainingStopManager.<.ctor>b__5_0(Object o, EventArgs e) in /__w/1/s/src/Microsoft.ML.AutoML/AutoMLExperiment/IStopTrainingManager.cs:line 72
   at Microsoft.ML.AutoML.CancellationTokenStopTrainingManager.<.ctor>b__5_0() in /__w/1/s/src/Microsoft.ML.AutoML/AutoMLExperiment/IStopTrainingManager.cs:line 38
   at System.Threading.CancellationToken.<>c.<Register>b__12_0(Object obj)
   at System.Threading.CancellationTokenSource.CallbackNode.<>c.<ExecuteCallback>b__9_0(Object s)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

To my best knowledge, LGTM.

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #6412 (ec1f420) into main (c69acbe) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6412      +/-   ##
==========================================
- Coverage   68.54%   68.52%   -0.02%     
==========================================
  Files        1169     1170       +1     
  Lines      246867   246960      +93     
  Branches    25788    25790       +2     
==========================================
+ Hits       169206   169222      +16     
- Misses      70938    71022      +84     
+ Partials     6723     6716       -7     
Flag Coverage Δ
Debug 68.52% <ø> (-0.02%) ⬇️
production 62.91% <ø> (-0.04%) ⬇️
test 88.96% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Microsoft.ML.FastTree/Dataset/DenseIntArray.cs 26.73% <0.00%> (-17.67%) ⬇️
src/Microsoft.ML.FastTree/Dataset/IntArray.cs 12.10% <0.00%> (-0.90%) ⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs 86.38% <0.00%> (-0.15%) ⬇️
...rosoft.ML.AutoML.Tests/StopTrainingManagerTests.cs 100.00% <0.00%> (ø)
...Microsoft.ML.AutoML.Tests/AutoMLExperimentTests.cs 96.52% <0.00%> (+0.29%) ⬆️

Copy link
Member

@michaelgsharp michaelgsharp left a comment

Choose a reason for hiding this comment

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

:shipit:

@michaelgsharp michaelgsharp merged commit de79b8a into dotnet:main Oct 27, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 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.

3 participants