Resolve nuget dependencies for UDFs defined in dotnet-interactive #515

suhsteve · 2020-05-12T17:29:35Z

In a dotnet-interactive REPL session (driver), nuget dependencies will be systematically added to SparkContext.AddFiles(..).

<packagename>.<version>.nupkg
nuget packages
dependencyProviderMetadata
Contains native probing paths required by Microsoft.DotNet.DependencyManager.DependencyProvider
Contains the assembly probing paths required by Microsoft.DotNet.DependencyManager.DependencyProvider
Contains nuget metadata such as filename, package name, and version to properly unpackage nuget packages.

On the Microsoft.Spark.Worker, in order to resolve the nuget dependencies referenced by the dotnet-interactive session, we use the files added to the SparkContext.AddFiles(...). These files can be accessed from the Worker by checking the SparkFiles.GetRootDirectory() path.

There are two parts
deserialize dependencyProviderMetadata

Using the NuGetMetadata, we locate and unpack .nupkg files to /path/to/working/directory/.nuget/packages/<package name>/<package version>
We use the AssemblyProbingPaths and NativeProbingPaths to instantiate a Microsoft.DotNet.DependencyManager.DependencyProvider class. This class will register itself to the AssemblyLoadContext Resolver.

note: Because SparkContext.AddFiles(..) prevents overwriting/deleting files once they have been added to the SparkContext, numbered identifiers are added to relevant files:

dependencyProviderMetadata => dependencyProviderMetadata_<zero padded ulong>

src/csharp/Microsoft.Spark.Worker/Microsoft.Spark.Worker.csproj

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

src/csharp/Microsoft.Spark/Interop/SparkEnvironment.cs

imback82 · 2020-05-22T20:27:55Z

src/csharp/Microsoft.Spark/Utils/UdfUtils.cs

+
+            string replMode =
+                EnvironmentUtils.GetEnvironmentVariableAsBool("DOTNET_SPARK_REPL_MODE").ToString();
+            environmentVars.Put("DOTNET_SPARK_REPL_MODE", replMode);


nit: do we always to set this or only when it's running a repl? Also, I suggested this name, but mode may not be a good name. Maybe DOTNET_SPARK_RUNNING_REPL or a name that conveys a boolean may be better?

Only need to set it if it's running the REPL. Modified code a bit and renamed env variable.

Hmm, who do you think should be responsible for setting up this env variable? Should we set it from the extension or whatever is going to start the dotnet-interactive session (livy) ?

good question. I was thinking whoever is starting the dotnet process, but I guess extension can still set it as well to be sure?

imback82 · 2020-05-22T20:29:56Z

src/csharp/Microsoft.Spark.Worker/Processor/PayloadProcessor.cs

@@ -87,6 +74,15 @@ internal Payload Process(Stream stream)
            }

            payload.IncludeItems = ReadIncludeItems(stream);
+
+#if NETCOREAPP


so basically, .NET framework will not work right? If so, let's remember to put this in the release note.

I suppose we don't really need this #if. It should compile fine with either framework. The only issue I foresee is the dotnet-interactive session is only pulling in dotnet and netstandard nugets. The AssemblyProbingPaths would only have paths to these two frameworks, and if a Worker required a net461 specific nugets, then it wouldn't be available.

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

imback82 · 2020-05-22T20:42:05Z

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

+                UnpackPackages(sparkFilesPath, unpackPath, metadata.NuGets);
+
+                (s_dependencyProvider as IDisposable)?.Dispose();
+                s_dependencyProvider = new DependencyProvider(AssemblyProbingPaths, NativeProbingRoots);


I guess this wouldn't affect the running tasks? Note that stages can run in parallel.

I was under the impression that a Worker process would only be running parallel tasks for a specific stage. Can a Worker process run multiple stages in parallel ? If so, will we be locking too often ?

Changed the dispose order a bit. I now register a new DependencyProvider for the new set of Assembly/Native probing paths before disposing the previously registered one.

imback82 · 2020-05-22T20:43:38Z

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

+            if (files.Length > 0)
+            {
+                Array.Sort(files);
+                return files.Last();


The highest file should have accumulated info right? In that case should we try to remove the old files proactively or no?

Correct. The highest file should contain the super set of all nuget, assembly, and native probing paths.

We can remove them proactively, but I don't think we'll have that many files. There would only be a new file for every cell/codesubmission that introduces a new nuget package.

imback82 · 2020-05-22T20:45:41Z

src/csharp/Microsoft.Spark/Utils/AssemblyLoader.cs

@@ -153,7 +166,15 @@ internal static Assembly ResolveAssembly(string assemblyName)
                    }
                }

-                throw new FileNotFoundException($"Assembly '{assemblyName}' file not found '{simpleAsmName}[{string.Join(",", s_extensions)}]' in '{string.Join(",", s_searchPaths)}'");
+                s_logger.LogWarn(


Why the behavior change?

Because we threw an exception in AssemblyLoader, the AssemblyLoadContext.Resolving event would stop checking any other event handlers that were registered.

https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext.resolving?view=netcore-3.1
If more than one event handler is registered for this event, the event handlers are called in order until an event handler returns a value that isn't null. Subsequent event handlers are ignored.

If we wanted to keep the the same behavior, then we would constantly need to remove and readd our AssemblyLoader event handler with the AssemblyLoadContext every time we remove/readd the DependencyProvider.

If we see this message, what does the user need to do? Is this something that can be ignored by the user?

Hmm. In the batch scenario, if the user sees this warning, it should be proceeded by an exception that will be thrown by the assembly loading class that is using this event handler.

In the interactive scenario the following may happen. User sees this warning. The next event handler registered by the DependencyProvider is processed. Next, either of the following can occur:

Finds assembly in the probing paths defined in the DependencyProvider.

Unable to find assembly and an exception is thrown similar to the batch scenario.

So, is this a noise (since an exception will be always thrown) or will be this be helpful for further debugging?

src/csharp/Microsoft.Spark/Utils/UdfUtils.cs

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

src/csharp/Microsoft.Spark/Utils/DependencyProviderUtils.cs

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

src/csharp/Microsoft.Spark.Worker/Processor/PayloadProcessor.cs

…teve/spark into repl_nuget_dependencies

src/csharp/Microsoft.Spark/Utils/DependencyProviderUtils.cs

imback82 · 2020-05-28T04:33:01Z

src/csharp/Microsoft.Spark/Utils/AssemblyLoader.cs

@@ -153,7 +166,15 @@ internal static Assembly ResolveAssembly(string assemblyName)
                    }
                }

-                throw new FileNotFoundException($"Assembly '{assemblyName}' file not found '{simpleAsmName}[{string.Join(",", s_extensions)}]' in '{string.Join(",", s_searchPaths)}'");
+                s_logger.LogWarn(


If we see this message, what does the user need to do? Is this something that can be ignored by the user?

imback82 · 2020-05-28T04:54:16Z

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

+                {
+                    s_dependencyProvider?.Dispose();
+                    s_dependencyProvider = dependencyProvider;
+                }


How should we handle this if TryLoad fails? Should we log or fail early?

Fails as in returns false or throws an exception ?

If it returns false since we are not catching any exceptions here anyway.

It will only return false if there are no metadata files (ie, no nuget dependencies or if spark did not distribute the files to the worker) and if the cached s_lastFileRead matches the latest metadata file in the SparkFiles path. In either case I don't think we need logs here.

ie, no nuget dependencies or if spark did not distribute the files to the worker

This sounds like a critical error case, no?

How do we determine if it's the former or latter ?

It depends on how you implement TryLoad. If you think it's a critical error, returning false in that case may not work.

I looked at this one more time, and I think it should be ok with the current approach.

src/csharp/Microsoft.Spark.Worker.UnitTest/DependencyProviderTests.cs

src/csharp/Microsoft.Spark.UnitTest/DependencyProviderUtilsTests.cs

imback82 · 2020-05-28T05:03:28Z

Generally LGTM, few minor comments/questions.

imback82 · 2020-05-28T16:29:06Z

src/csharp/Microsoft.Spark.UnitTest/DependencyProviderUtilsTests.cs

+            }
+
+            string expectedFile = "dependencyProviderMetadata_00000000000000000020";
+            string highestFile =


nit: latest

imback82 · 2020-05-28T16:30:36Z

src/csharp/Microsoft.Spark.UnitTest/DependencyProviderUtilsTests.cs

+            DependencyProviderUtils.Metadata deserializedMetadata =
+                DependencyProviderUtils.Metadata.Deserialize(serializedFilePath);
+
+            Assert.Equal(metadata, deserializedMetadata);


Is it ok to depend on EqualTo (new code) to test Serialize/Deserialize?

Sorry i'm not sure what you're referring to.

This will invoke Metadata.Equals?

Yes it will call Metadata.Equals

Can you depend on the function to verify Ser/De since it can have a bug (not checking all fields, for example)?

imback82 · 2020-05-28T16:42:41Z

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

+                {
+                    s_dependencyProvider?.Dispose();
+                    s_dependencyProvider = dependencyProvider;
+                }


I looked at this one more time, and I think it should be ok with the current approach.

imback82 · 2020-05-28T16:45:10Z

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

+                    Directory.GetCurrentDirectory());
+                if (dependencyProvider.TryLoad())
+                {
+                    s_dependencyProvider?.Dispose();


What happens if another task is using this s_dependencyProvider and you Dispose at the same time?

Maybe we should make it thread local? What's the memory usage and perf to create/dispose this? Should we ask to implement incremental update instead of creating a new one every time?

Removed previous Dispose pattern. Instead we will update the Metadata files to only include incremental updates and load a new DependencyProvider for each metadata file.

src/csharp/Microsoft.Spark/Utils/DependencyProviderUtils.cs

imback82 · 2020-05-28T20:40:44Z

src/csharp/Microsoft.Spark/Utils/DependencyProviderUtils.cs

+                    Equals(nugetMetadata);
+            }
+
+            public bool Equals(NuGetMetadata other)


private here as well?

…teve/spark into repl_nuget_dependencies

imback82 · 2020-05-28T20:53:28Z

src/csharp/Microsoft.Spark.Worker/Utils/DependencyProvider.cs

+    {
+        private readonly DepManager.DependencyProvider _dependencyProvider;
+
+        internal DependencyProvider(string metadataFile, string src, string dst)


nit: metadataFilePath, srcPath, dstPath?

imback82 · 2020-05-28T20:56:23Z

src/csharp/Microsoft.Spark.Worker/Utils/AssemblyLoaderHelper.cs

+            LoggerServiceFactory.GetLogger(typeof(AssemblyLoaderHelper));
+
+        // A mapping between a metadata file's path to its respective DependencyProvider.
+        private static readonly ConcurrentDictionary<string, Lazy<DependencyProvider>> s_depProvs =


can we spell it out to s_dependencyProviders to be consistent with other names?

It will exceed the 100 char limit if you are okay with this.

imback82

LGTM, thanks @suhsteve!

suhsteve added 2 commits May 12, 2020 09:32

initial commit

33ce5e7

Merge branch 'master' into repl_nuget_dependencies

9ce25ca

suhsteve changed the title ~~Resolve nuget dependencies for UDFs defined in dotnet-interactive~~ [WIP] Resolve nuget dependencies for UDFs defined in dotnet-interactive May 12, 2020

get package name and version from nuget metadata file

ead9f1b

suhsteve changed the title ~~[WIP] Resolve nuget dependencies for UDFs defined in dotnet-interactive~~ Resolve nuget dependencies for UDFs defined in dotnet-interactive May 12, 2020

suhsteve added 3 commits May 12, 2020 23:45

use nuget metadata to find locate required files

ec982b3

rename var

30e6142

remove newline

7c120a9

suhsteve mentioned this pull request May 15, 2020

dotnet-interactive assembly extension #517

Merged

suhsteve added 9 commits May 18, 2020 18:46

serialize metadata

9ac5ff4

move to new file

4faa0d6

update env variable

7cfaf16

reload dependencymanager at every new stage.

ccb5dfc

AssemblyLoader to return null instead of throwing exception

af3cbc3

fixes

0ccce95

add comments

e537215

update comment

baedad4

return if no updates to metadata

1e541a5

imback82 assigned suhsteve May 22, 2020

imback82 added the enhancement New feature or request label May 22, 2020

imback82 reviewed May 22, 2020

View reviewed changes

imback82 mentioned this pull request May 23, 2020

Using with .NET Interactive notebooks locally #529

Closed

suhsteve added 2 commits May 25, 2020 19:34

Address PR comments.

4846ecb

Merge branch 'master' into repl_nuget_dependencies

a0b91c2

imback82 reviewed May 27, 2020

View reviewed changes

suhsteve added 3 commits May 26, 2020 22:26

PR Comments

3cdc98b

Merge branch 'repl_nuget_dependencies' of https://www.github.com/suhs…

30fc7e7

…teve/spark into repl_nuget_dependencies

add comments

b98e02c

imback82 reviewed May 28, 2020

View reviewed changes

PR comments.

f092f38

imback82 reviewed May 28, 2020

View reviewed changes

suhsteve and others added 2 commits May 28, 2020 13:30

PR Comments

0969188

Merge branch 'master' into repl_nuget_dependencies

7885fd0

imback82 reviewed May 28, 2020

View reviewed changes

suhsteve added 2 commits May 28, 2020 13:49

update access modifier.

59dc64b

Merge branch 'repl_nuget_dependencies' of https://www.github.com/suhs…

a0d3b95

…teve/spark into repl_nuget_dependencies

imback82 reviewed May 28, 2020

View reviewed changes

suhsteve added 6 commits May 28, 2020 13:56

rename parameters.

0e4e55b

rename variable.

a0e682f

pr comment.

7a09e8c

Add tests for Metadata equals

b1ad1bc

Add CollectionUtilsTests

419ae43

add header, clean usings

86b4348

imback82 approved these changes May 28, 2020

View reviewed changes

imback82 merged commit ce23177 into dotnet:master May 29, 2020

suhsteve deleted the repl_nuget_dependencies branch May 29, 2020 07:10

Niharikadutta mentioned this pull request Sep 9, 2020

[FEATURE REQUEST]: Ability to reference Nuget packages in UDFs in notebooks #409

Closed

Resolve nuget dependencies for UDFs defined in dotnet-interactive #515

Resolve nuget dependencies for UDFs defined in dotnet-interactive #515

Uh oh!

Conversation

suhsteve commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suhsteve May 26, 2020 • 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!

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!

suhsteve May 26, 2020 • 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!

suhsteve May 28, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

suhsteve May 28, 2020 • 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!

suhsteve commented May 12, 2020 •

edited

Loading

suhsteve May 26, 2020 •

edited

Loading

suhsteve May 26, 2020 •

edited

Loading

suhsteve May 28, 2020 •

edited

Loading

suhsteve May 28, 2020 •

edited

Loading