Csi native #600

KevinRansom · 2019-11-06T05:50:05Z

Update @jonsequitur 's PR to probe for cross platform native .dlls

Other changes are Jon's.

KevinRansom · 2019-11-06T05:51:11Z

MLS.Agent/NativeAssemblyLoadHelper.cs

 {
    public class NativeAssemblyLoadHelper : INativeAssemblyLoadHelper
    {
+        private static readonly HashSet<DirectoryInfo> globalProbingPaths = new HashSet<DirectoryInfo>();


There are several instances of NativeAssemblyLoadHelper created. The globalProbingPaths is the union of all of the located probing paths.

KevinRansom · 2019-11-06T05:54:17Z

MLS.Agent/NativeAssemblyLoadHelper.cs

            }
        }

+        private IEnumerable<string> ProbingFilenames(string name)


the name passed in here is the value specified with the DllAttribute. by convention, the dll may have:

A full path

A relativepath

lib prepended

on windows .dll or .exe as a suffix.

on linux .so as a suffix

on macos .dynlib as a suffix

This function enumerates the possibilities.

KevinRansom · 2019-11-06T05:55:24Z

MLS.Agent/NativeAssemblyLoadHelper.cs

+            }
+        }
+
+        private IEnumerable<string> ProbingPaths(string probingPath, string name)


This function combines the probing root directory with the runtimes/rid/native to produce a list of dll's

KevinRansom · 2019-11-06T05:56:13Z

MLS.Agent/NativeAssemblyLoadHelper.cs

                (libraryName, assembly, searchPath) =>
                {
+                    var ptr = IntPtr.Zero;
                    if (_resolvers.TryGetValue(


Use calling assembly path to limit probes to _probepaths.

KevinRansom · 2019-11-06T05:57:12Z

MLS.Agent/NativeAssemblyLoadHelper.cs

+                    return ptr;
+                });
+
+            IntPtr nativeLoader(string dll)


If we can't load it, just return a Zero intptr

KevinRansom · 2019-11-06T05:57:45Z

MLS.Agent/NativeAssemblyLoadHelper.cs

+                    foreach (var pname in ProbingFilenames(name))
+                    {
+                        var p = Path.Combine(path, pname);
+                        if (File.Exists(p))


Since we are probing for a file, don't return it if it isn't actually there.

jonsequitur · 2019-11-06T14:03:14Z

NuGet.config

    <add key="dotnet-try" value="https://dotnet.myget.org/F/dotnet-try/api/v3/index.json" />
    <add key="roslyn" value="https://dotnet.myget.org/F/roslyn/api/v3/index.json" />
-    <add key="dotnet-corefxlab" value="https://dotnet.myget.org/F/dotnet-corefxlab/api/v3/index.json" />
+    <add key="dotnet-corefxlab" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/MachineLearning/nuget/v3/index.json" />


Did you mean to include this?

this file should not use azure package source

eerhardt · 2019-11-06T22:04:44Z

WorkspaceServer/Kernel/INativeAssemblyLoadHelper.cs

-        void Configure(string v);
+        void Handle(ResolvedNugetPackageReference reference);
+
+        void SetNativeDllProbingPaths(IReadOnlyList<DirectoryInfo> nativeDllProbingPaths);


(nit) NativeDll may not be the best name here since .dll is a Windows concept/extension.

How about NativeLibrary?

eerhardt · 2019-11-06T22:06:28Z

MLS.Agent/NativeAssemblyLoadHelper.cs

+
+            // Computer valid dotnet-rids for this environment: https://docs.microsoft.com/en-us/dotnet/core/rid-catalog
+            // Where rid is: win, win-x64, win-x86, osx-x64, linux-x64 etc ...
+            IEnumerable<string> ProbingRids()


Why are we doing this ourselves? This is exactly what AssemblyDependencyResolver is meant to solve. It reads the .deps.json files and handles any RID fallbacks.

We don't have a .deps.json. We're doing a restore but not a build in order to avoid copying so many files out of the NuGet caches.

Maybe we should be generating a .deps.json file then. The logic being added here is super fragile and will not work in many cases.

For example, the proposed code won't handle the same scenario I just fixed in Azure Functions - Azure/azure-functions-host#5187. In that case, the native library is located at runtimes\{RID}\nativeassets\{TFM}. You won't be able to guess at this path, unless you are planning on knowing how to fall back through TFMs. The current RID support here is also problematic because it won't allow for things like alpine to work.

The more we try to hard-code what the current system does, the more we are going to get it wrong. We should be using as much of the current systems as possible.

Maybe we should be generating a .deps.json file then.

Do you mean by doing a build or do you have another approach in mind?

You wouldn't need to do a full build to generate a .deps.json file. We have a few levels of places we can get at the current code.

Call into the MSBuild Task directly.

Invoke the MSBuild Target

Maybe @dsplaisted or @nguerrera have other ideas on how to generate a .deps.json file without doing a full build.

Another potential route would be to inspect NuGet's project.assets.json file and read all of the native asset paths from it. However, we would still need to know how to do RID fallback which the AssemblyDependencyResolver solves for us. But that is a solvable problem by reading the shared frameworks .deps.json file, which will give you the RID fallback graph for the current machine. (Assuming we are always running on a shared framework in dotnet-try.)

Also - another route is that if you just want to avoid "copying so many files out of the NuGet cache", you can set MSBuild properties to disable that. See dotnet/sdk@1936ebd.

The CopyLocalLockFileAssemblies property can be set to false to revert to
the previous behavior for dotnet build.

I agree with Eric that you should try to reuse as much of the existing build logic as possible. I would avoid calling the GenerateDepsFile task directly, as a lot of the logic of what flows into that happens in targets files. Running the GenerateDepsFile target is probably the right thing to do.

eerhardt · 2019-11-06T22:07:28Z

MLS.Agent/NativeAssemblyLoadHelper.cs

-                    NativeLibrary.SetDllImportResolver(args.LoadedAssembly, Resolve);
+                    ptr = NativeLibrary.Load(dll);
+                    Logger.Log.Info("NativeLibrary.Load({dll})", args.LoadedAssembly.Location);
+                    Console.WriteLine($"NativeLibrary.Load({dll})");


Is this going to show up in the user's notebook output?

Yes. The Console.WriteLine needs to be removed.

WorkspaceServer/Kernel/INativeAssemblyLoadHelper.cs

eerhardt · 2019-11-07T16:21:07Z

NotebookExamples/csharp/Samples/HousingML.ipynb

        "#r \"nuget:Microsoft.ML,1.4.0-preview\"\n",
        "#r \"nuget:Microsoft.ML.AutoML,0.16.0-preview\"\n",
-        "#r \"nuget:Microsoft.Data.DataFrame,0.1.1-e190920-1\""
+        "#r \"nuget:Microsoft.Data.DataFrame,1.0.0-e190910-1\""


Unfortunately we messed up some versions on our myget feed. The 1.0.0- versions are older than the 0.1.1- versions. If you are looking at the feed, note the date it was published.

The good news is that we have renamed the package to Microsoft.Data.Analysis and so all the versioning issues went away.

I can send a separate update to fix this to the latest package name and version, and fixing any code that needs updating.

@KevinRansom, please revert the notebook changes.

@jonsequitur, I had already reverted them on my machine. Eric, updated the version, and have updated it to the newer version.

The tests were failing because there was no way to specify the custom feed, with the new location for the project file. I have added support for #r "nuget:restoresources=http://blah".

Unlike the F# repo, this needs to be specified before the #r nugets that rely on it. So … first I suppose.

I am going to spend some time seeing if I can wire up the F# #r nuget processor, wich would enable me to rip out some of the code.

@eerhardt --- the primary benefit of reading the assets.json file, will be that there is less guesswork about where to find the file is, and so it should perform better in the resolution stage. My main gripe, is there is no decent way to find the nuget root directory.

I will try to incorporate asset.json probing in the F# probing algorithm and thus the C# probing if I can successfully incorporate it. However, using #I to specified probing roots for assemblies, has always been a thing in F# and we will want to continue with that including native probing, it's a nice developer scenario.

My main gripe, is there is no decent way to find the nuget root directory.

I believe that is what the FallbackPackagePathResolver in NuGet is for. It will locate where a NuGet package is for a given context.

See https://github.com/dotnet/sdk/blob/51a0655dea6d6e4f56c235191aec6c2f9d62c07b/src/Tasks/Microsoft.NET.Build.Tasks/NuGetPackageResolver.cs#L17 for the usage in the .NET SDK.

However, doesn't the Interactive Kernel control where the nuget root directory is?

It doesn't.

Per our discussions yesterday, I'd prefer to merge this PR and then revisit the approach, since there a number of different bugs being fixed here, not just the native dependency resolution.

KevinRansom commented Nov 6, 2019

View reviewed changes

jonsequitur reviewed Nov 6, 2019

View reviewed changes

jonsequitur and others added 10 commits November 6, 2019 06:58

wip

4438342

wip

9d30f34

use target to scan dependencies

764237f

add utility function to parse file

9ebac9e

wip

2c69c64

wip

a21816e

wip

89e317d

wip

56a1072

rename test

0bcdd01

native loader

7755032

KevinRansom force-pushed the csinative branch from 367b0ab to 7755032 Compare November 6, 2019 21:12

jonsequitur approved these changes Nov 6, 2019

View reviewed changes

jonsequitur mentioned this pull request Nov 6, 2019

[WIP] #r nuget improvements #586

Closed

eerhardt reviewed Nov 6, 2019

View reviewed changes

WorkspaceServer/Kernel/INativeAssemblyLoadHelper.cs Show resolved Hide resolved

KevinRansom added 4 commits November 6, 2019 14:33

Do it correctly this time

d628f58

Cleanup comments

ba24cc6

Update microsoft.data.dataframe version

c6e8cbb

revert package version

ad84479

eerhardt reviewed Nov 7, 2019

View reviewed changes

temp

0df8028

KevinRansom added 2 commits November 7, 2019 20:19

merge

638c091

cleanup

aace3ca

colombod merged commit 1f0e3e9 into dotnet:master Nov 8, 2019

eerhardt mentioned this pull request Feb 25, 2020

NuGet packages with native assets in a notebook dotnet/interactive#75

Closed

13 tasks

Csi native #600

Csi native #600

Uh oh!

Conversation

KevinRansom commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsequitur Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsequitur Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KevinRansom commented Nov 6, 2019 •

edited

Loading

jonsequitur Nov 6, 2019 •

edited

Loading

jonsequitur Nov 7, 2019 •

edited

Loading

eerhardt Nov 8, 2019 •

edited

Loading