Skip to content

Attempt to retarget tests to .NET 6.0 #6367

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 7 commits into from
Oct 18, 2022
Merged

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Oct 10, 2022

Trying out retargeting our tests to .NET 6.0.

@ghost ghost assigned ericstj Oct 10, 2022
@ericstj
Copy link
Member Author

ericstj commented Oct 11, 2022

I believe we're tripping up on the differences between CPUMath and Intrinsics. Either we're using the wrong data file, or we're loading the wrong CPUMath. Looking into it.

Ideally we should just remove the knowledge of TargetFramework from the
build, but that can happen in a follow up change.
@ericstj
Copy link
Member Author

ericstj commented Oct 11, 2022

I think I found and fixed the problems around test data. We weren't even using the .NETCore 3.1+ data since the way we were checking for .NETCore 3.1+ broke in 6.0. I fixed that. Also fixed a few other cases where we were conditioning our behavior based on .NETCore 3.1. I made all the chanages here "future proof" by avoiding hardcoding a version we'll have to change moving forward.

@ericstj
Copy link
Member Author

ericstj commented Oct 11, 2022

This seems to have fixed the majority of the issues, here are the two remaining, across all platforms:
MulticlassLRNonNegativeTest

�[m�[37m        *** Failure #1: Values to compare are 0.1270046062939375 and 0.1273098059294578
�[m�[37m        	 AllowedVariance: 0.0001
�[m�[37m        	 delta: -0.0003
�[m�[37m        	 delta2: -0.0003

�[m�[37m        *** Failure #2: Output and baseline mismatch at line 43, expected '41	0	0	0.12700460629393751	0.8807296	0.11926952	7.253493E-08	0	1	2' but got '41	0	0	0.12730980592945779	0.88046086	0.119539626	7.181209E-08	0	1	2' : 'MulticlassLogisticRegression/LogisticRegression-Non-Negative-TrainTest-iris.txt'

�[m�[37m        *** Failure #3: Values to compare are 0.30088424491217947 and 0.3007240049803194
�[m�[37m        	 AllowedVariance: 0.0001
�[m�[37m        	 delta: 0.0002
�[m�[37m        	 delta2: 0.0002

�[m�[37m        *** Failure #4: Output and baseline mismatch at line 32, expected '68	1	1	0.30088424491217947	0.740163445	0.258734822	0.00110106682	1	2	0' but got '68	1	1	0.30072400498031943	0.74028206	0.25861543	0.001101945	1	2	0' : 'MulticlassLogisticRegression/LogisticRegression-Non-Negative-CV-iris.txt'

BinaryClassifierLogisticRegressionBinNormTest

�[m�[37m        *** Failure #1: Values to compare are -5.641634 and -5.6416364
�[m�[37m        	 AllowedVariance: 1E-06
�[m�[37m        	 delta: 1E-05
�[m�[37m        	 delta2: 2E-06

�[m�[37m        *** Failure #2: Output and baseline mismatch at line 12, expected '10	0	-5.641634	0.00353453052	0.0051082826744376919	0' but got '10	0	-5.6416364	0.0035345221	0.0051082705390145811	0' : 'LogisticRegression/LogisticRegression-bin-norm-TrainTest-breast-cancer.txt'

�[m�[37m        *** Failure #3: Values to compare are 6.373105 and 6.3731103
�[m�[37m        	 AllowedVariance: 1E-06
�[m�[37m        	 delta: -1E-05
�[m�[37m        	 delta2: -5E-06

�[m�[37m        *** Failure #4: Output and baseline mismatch at line 2, expected '5	1	6.373105	0.9982961	0.0024603307167519865	1' but got '5	1	6.3731103	0.9982961	0.0024603307167519865	1' : 'LogisticRegression/LogisticRegression-bin-norm-CV-breast-cancer.txt'

These small changes seem like acceptable rounding error to me. I'm inclined to just update the .NET Core baseline files @michaelgsharp do you agree?

@ericstj
Copy link
Member Author

ericstj commented Oct 12, 2022

Baseline data updated to account for those failures. We also have CPUMath failures on ARM64. Looks like those are trying to load CpuMathNative which we don't expect.

It should only be included when TargetFramework is not compatible with
netcoreapp3.1.
@ericstj
Copy link
Member Author

ericstj commented Oct 12, 2022

Ubuntu failure is well known system.drawing issue.

Microsoft.ML.Tests.ImageTests.TestBackAndForthConversionWithAlphaInterleave [FAIL]�[31;1m�[m�[37m      System.ArgumentException : Parameter is not valid.
�[m�[30;1m      Stack Trace:
�[m�[37m           at System.Drawing.Bitmap.UnlockBits(BitmapData bitmapdata)

Rest of the legs are looking good 👍

Coverage is broken for some reason. I'll look into that next.

@ericstj
Copy link
Member Author

ericstj commented Oct 13, 2022

I thought the bug here was that our PathResolver for loading from the nuget package cache was busted, but actually it's that the runtime.config.dev.json is no longer produced when targeting net6.0. This is the piece that tells the runtime how to locate the nuget package folder. https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/6.0/runtimeconfigdev-file

We can fix this by setting GenerateRuntimeConfigDevFile to restore the old behavior.

We need this file to tell the runtime where the NuGet package cache is.
It uses that location when we disable copying of NuGet dependencies.
@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Merging #6367 (4c76f02) into main (8475fe0) will decrease coverage by 0.01%.
The diff coverage is 62.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6367      +/-   ##
==========================================
- Coverage   68.51%   68.49%   -0.02%     
==========================================
  Files        1164     1165       +1     
  Lines      246113   246202      +89     
  Branches    25554    25744     +190     
==========================================
+ Hits       168619   168646      +27     
- Misses      70821    70870      +49     
- Partials     6673     6686      +13     
Flag Coverage Δ
Debug 68.49% <62.50%> (-0.02%) ⬇️
production 62.92% <ø> (-0.02%) ⬇️
test 88.98% <62.50%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
test/Microsoft.ML.FSharp.Tests/SmokeTests.fs 87.23% <ø> (ø)
...t/Microsoft.ML.PerformanceTests/Harness/Configs.cs 0.00% <0.00%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs 70.11% <ø> (ø)
...crosoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs 99.37% <ø> (ø)
test/Microsoft.ML.CpuMath.UnitTests/UnitTests.cs 95.35% <100.00%> (ø)
...est/Microsoft.ML.TestFramework/BaseTestBaseline.cs 76.95% <100.00%> (-0.05%) ⬇️
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs 97.45% <100.00%> (ø)
src/Microsoft.ML.SearchSpace/Tuner/RandomTuner.cs 42.85% <0.00%> (-38.97%) ⬇️
...ansforms/PermutationFeatureImportanceExtensions.cs 94.01% <0.00%> (-1.71%) ⬇️
...icrosoft.ML.Core/Utilities/ResourceManagerUtils.cs 58.21% <0.00%> (-0.94%) ⬇️
... and 22 more

@ericstj
Copy link
Member Author

ericstj commented Oct 13, 2022

Looks very close. The one failure on ARM seems to be a transient test failure. I've opened #6377 to track it and reran that leg.

@ericstj ericstj marked this pull request as ready for review October 13, 2022 18:02
@ericstj
Copy link
Member Author

ericstj commented Oct 13, 2022

image

✋🎉🎆🚀

Also fix a couple places where ifdefs were hardcoded to the version of .NETCore.
@ericstj ericstj merged commit c8b3ca4 into dotnet:main Oct 18, 2022
ericstj added a commit to ericstj/machinelearning that referenced this pull request Oct 19, 2022
* Attempt to retarget tests to .NET 6.0

* Fix places where tests had hardcoded framework versions

* Update build naming

Ideally we should just remove the knowledge of TargetFramework from the
build, but that can happen in a follow up change.

* Update test data for rounding differences on net6.0

* Fix condition on CPUMathNative reference in tests

It should only be included when TargetFramework is not compatible with
netcoreapp3.1.

* Ensure we generate runtimeconfig.dev.json

We need this file to tell the runtime where the NuGet package cache is.
It uses that location when we disable copying of NuGet dependencies.

* Rename test baseline output directory.

Also fix a couple places where ifdefs were hardcoded to the version of .NETCore.
@ghost ghost locked as resolved and limited conversation to collaborators Nov 17, 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.

2 participants