Closed carlossanlop closed 1 week ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 68.90%. Comparing base (
f385b06
) to head (908acb5
). Report is 7 commits behind head on main.
@michaelgsharp there are several dead-lettering failures. Can you take a look? Maybe the VM queues need updating?
/azp run
System.IO.FileLoadException: Could not load file or assembly 'System.Memory, Version=4.0.1.2, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x8013104
The new version of System.Memory
is 4.0.2.0
. I would expect the test to auto-generate a bindingRedirect for this.
I found these two lines hardcoding the package version, which should now be 4.6.0: https://github.com/dotnet/machinelearning/blob/5c503195e8fb64ea5f73c6a46c76d87f4aa0503b/test/Microsoft.ML.CodeAnalyzer.Tests/Helpers/AdditionalMetadataReferences.cs#L17-L23
Those are just for testing the analyzer.
Here the problem is due to RemoteExecutor.
The .dll.config has the correct redirect. If you download the helix payload and examine workitems\Microsoft.ML.CpuMath.UnitTests\Microsoft.ML.CpuMath.UnitTests.dll.config
you can see
<dependentAssembly>
<assemblyIdentity name="System.Memory" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-4.0.2.0" newVersion="4.0.2.0" />
</dependentAssembly>
But that .dll.config isn't being reused with RemoteExecutor.
This same issue was hit here: https://github.com/dotnet/runtime/issues/104647 @ViktorHofer mentions that it should be handled with https://github.com/dotnet/arcade/blob/fc2f7ce8372a55725aab7b48c25bad7327a9769d/src/Microsoft.DotNet.RemoteExecutor/src/build/Microsoft.DotNet.RemoteExecutor.targets#L8-L33 -- it looks like ML.NET doesn't use that remote executor, but still uses it's own which may not have the fix. So that's the "right fix" here -- make the ML.NET remoteexecutor honor the tests redirects.
If I look at the assembly refs here, most are coming from the netstandard2.0
library which was pinned to 4.0.1.2
. In the 4.5.5 version of System.Memory
both the netstandard2.0
and net461
libraries had the same version. In the new package we kept netstandard2.0
pinned at 4.0.1.2
but bumped net4*
to 4.0.2.0
. I wonder if we should avoid doing this in servicing since it'll be introducing the need for a redirect in this scenario? I checked and the version that's inbox in netcoreapp2.1
is actually 4.1.0.0
so we don't need to pin the netstandard2.0
version -- so long as it remains lower than 4.1.0.0
.
We don't use the RemoteExecutor in arcade because it doens't support NetFX, so we still have our custom one. Let me take a look at @ViktorHofer's fix and see what it would take to adopt it as well.
@michaelgsharp RemoteExecutor in Arcade supports .NET Framework. A lot of the tests in runtime target framework and depend on RemoteExecutor.
Closing as #7295 takes care of it and the remote executor update.
dotnet/runtime depends on these packages that we are now publishing from dotnet/maintenance-packages:
Bumping their versions to consume the new preview versions, available in the dotnet-libraries feed: https://dnceng.visualstudio.com/public/_artifacts/feed/dotnet-libraries
Related PRs: