dotnet-interactive assembly extension#517
Conversation
suhsteve
changed the title
suhsteve
force-pushed
the
dotnetinteractive_extension
branch
from
9bd0f0e to
d7960f9
Compare
| string assemblyName = | ||
| AssemblyLoader.NormalizeAssemblyName(preCompilation.AssemblyName); | ||
| string assemblyPath = Path.Combine(tempDir.FullName, $"{assemblyName}.dll"); | ||
| if (!File.Exists(assemblyPath)) |
There was a problem hiding this comment.
Should we handle the case where the assemblyPath already exists?
There was a problem hiding this comment.
I think it's safe to remove now that we have the command is SubmitCode check.
There was a problem hiding this comment.
If this is an unexpected case, should we just throw and fail early?
There was a problem hiding this comment.
Previously, other dotnet-interactive internal calls were hitting the middleware, and running the dll emitting logic and this caused it to try to emit duplicate assemblies. But I believe with the command is SubmitCode we should be okay. But to be safe we can readd the check and not run the middleware.
| if (!IsPathValid(nugetFile.FullName)) | ||
| { | ||
| // Copy the nuget file to a location that works with | ||
| // SparkContext.AddFiles(..) |
There was a problem hiding this comment.
why is this? (e.g., why wouldn't it work)?
There was a problem hiding this comment.
The original nupkg file can be in a location like, C:\Users\user with spaces\.nuget\packages\package.name\version\package.name.version.nupkg and that would not work with SparkContext.AddFiles(..) with a spark version lower than 3.
|
|
||
| foreach (FileInfo file in files) | ||
| { | ||
| if (!file.Exists || s_filesCopied.Contains(file.Name)) |
There was a problem hiding this comment.
Would it be easy to debug the !file.Exists case if we ever hit it?
There was a problem hiding this comment.
We can remove this check and trust DirectoryInfo.EnumerateFiles(..).
| public IEnumerable<ResolvedNuGetPackage> GetPackagesToCopy() | ||
| { | ||
| PackageRestoreContext restoreContext = | ||
| ((ISupportNuget)KernelInvocationContext.Current.HandlingKernel) |
There was a problem hiding this comment.
should ISupportNuget be passed into the constructor of this class? In that case, is this class also testable now (not sure how hard it is to mock ResolvedPackageReference)? And I think this function can move to PackageHelper? (I don't get the abstraction layers of PackageResolver vs PackageHelper)
There was a problem hiding this comment.
Originally moved out logic from PackageHelper into PackageResolver to do unit tests. Couldn't mock ISupportNuget, because PackageRestoreContext.ResolvedPackageReferences isn't a virtual property.
Moved logic back into PackageHelper and instead wrapping the ISupportNuget ... PackageRestoreContext.ResolvedPackageReferences call.
imback82
left a comment
imback82
left a comment
There was a problem hiding this comment.
LGTM except for few minor/nit comments.
|
|
||
| namespace Microsoft.Spark.Extensions.DotNet.Interactive | ||
| { | ||
| internal class PackageHelper |
There was a problem hiding this comment.
Is there a better name than Package'Helper'? :)
There was a problem hiding this comment.
hmm.. I'm bad with naming. How about Package'Resolver' ?
| string assemblyName = | ||
| AssemblyLoader.NormalizeAssemblyName(compilation.AssemblyName); | ||
| assemblyPath = Path.Combine(dstPath, $"{assemblyName}.dll"); | ||
| if (!File.Exists(assemblyPath)) |
There was a problem hiding this comment.
I thought this was a critical error scenario no?
There was a problem hiding this comment.
I don't think it's expected. Should we log an error or throw an exception ?
There was a problem hiding this comment.
let's throw an exception to fail fast
|
|
||
| private string GetPathRelativeToPackages(string file, DirectoryInfo directory) | ||
| { | ||
| string strippedRoot = file |
There was a problem hiding this comment.
Can you add one example for this function please?
There was a problem hiding this comment.
Should I move the examples that are at the callsite of GetPathRelativeToPackages(..) and add it to the method description instead ?
There was a problem hiding this comment.
oh, you can just add it here.
There was a problem hiding this comment.
just relevant example related to this function should be fine.
|
@imback82 addressed your comments. LMK if they look fine. |
| probingPaths) | ||
| }); | ||
|
|
||
| var packageResolver = |
There was a problem hiding this comment.
nit: no need to break now?
|
@imback82 actually don't merge yet. The |
|
Also, please be aware of and weigh in on: dotnet/interactive#512. |
|
@suhsteve the tests keep failing with timeouts (I retried 3 times). Can you check? |
I've seen this happen quite a few times with the recent PRs. I think something fishy is going on with the test pipeline. |
|
I don't see the failures from recent builds here? https://dev.azure.com/dnceng/public/_build?definitionId=459&_a=summary |
I think it's an issue with DeltaTable.Foreach , issue #333 . If we get lucky then it will use the same thread that has the activeThreadSession . |
|
Well, I saw multiple timeouts (>60 mins). Let's see what happens with the latest one. |
|
You can check attempt 3 and 4. |
|
I think we need to merge #524 soon. |
|
The latest failed with Delta error. But if you look at the runtime, it could not have finished within one hour. You can compare the runtimes with https://dev.azure.com/dnceng/public/_build/results?buildId=679547&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774. Any changes in this PR could cause the regression? |
|
Maybe build agent changed? I see that E2E for Spark 2.3.x is taking more than 2 minutes instead of <1 minute even in master: https://dev.azure.com/dnceng/public/_build/results?buildId=685678&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=c58e8551-6b20-5658-7a75-0283b3f97acb |
4 participants
This extension will add a a middleware to the dotnet-interactive session when this nuget is added via
#r "nuget: Microsoft.Spark.Extensions.DotNet.Interactive".For every code submission, the middleware will add the compiled assembly (before the current code has been processed) to the spark context by calling
SparkContext.AddFile(...).In addition to this, we also add any nuget package dependencies that the current session depends on to
SparkContext.AddFile(...)PackageHelperidentifies dependent nuget packages and generates the necessary files forMicrosoft.DotNet.DependencyManager.DependencyProviderPlease see #515 for additional details.