-
Notifications
You must be signed in to change notification settings - Fork 329
Fixed UDFs in dotnet.interactive environments #1217
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
base: main
Are you sure you want to change the base?
Conversation
Added a random string and to the metadata name a fallback for retrieving spark session to allow kernel restarting
Thanks @jonsequitur for your help! |
Do you have a stable repro for this? This sounds like a bug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes User-Defined Functions (UDFs) support in .NET Interactive environments by addressing three main issues: replacing the removed DisposableDirectory API, fixing command failures after core restarts, and handling the absence of ResolvedPackageReferences in current kernel contexts.
Key changes include:
- Adding random GUID prefixes to metadata filenames to prevent conflicts after restarts
- Implementing reflection-based package reference extraction as a workaround
- Updating to newer .NET Interactive package versions
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
DependencyProviderUtils.cs |
Modified filename generation to include GUID prefix for uniqueness |
AssemblyLoaderHelper.cs |
Updated comment reference to reflect new CreateFileName signature |
Test files | Updated tests to accommodate new GUID-based filename pattern |
SupportNugetWrapper.cs |
Removed obsolete wrapper class |
ReferencedPackagesExtractor.cs |
Added new reflection-based approach to extract package references |
PackageResolver.cs |
Updated to use new extractor and GUID-based filenames |
AssemblyKernelExtension.cs |
Enhanced SparkSession retrieval logic and temp directory management |
Project files | Updated .NET Interactive package versions |
s_filePattern.Replace("*", $"{number:D19}"); | ||
// 20 => dependencyProviderMetadata_f1a2b3c400000000020 | ||
internal static string CreateFileName(Guid runId, long number) => | ||
s_filePattern.Replace("*", $"{runId.ToString("N").Substring(0, 8)}{number:D11}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Substring(0, 8) creates a new string allocation. Consider using runId.ToString("N")[..8] (range operator) or AsSpan().Slice(0, 8) for better memory efficiency.
s_filePattern.Replace("*", $"{runId.ToString("N").Substring(0, 8)}{number:D11}"); | |
s_filePattern.Replace("*", $"{runId.ToString("N")[..8]}{number:D11}"); |
Copilot uses AI. Check for mistakes.
|
||
if (disposables is null) | ||
{ | ||
throw new Exception("Failed to retrieve referenced packages from kernel, try using older version of Dotnet.Interactive"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message suggests using an older version but doesn't provide guidance on which version to use. Consider making the message more specific about compatible versions or troubleshooting steps.
throw new Exception("Failed to retrieve referenced packages from kernel, try using older version of Dotnet.Interactive"); | |
throw new Exception("Failed to retrieve referenced packages from kernel. Ensure you are using a compatible version of Dotnet.Interactive (e.g., 1.0.0 to 1.2.0). For more details, visit https://github.com/dotnet/spark/discussions/1179"); |
Copilot uses AI. Check for mistakes.
|
||
if (restoreContext is null) | ||
{ | ||
throw new Exception("PackageRestoreContext was not found in the kernel, try using older version of Dotnet.Interactive"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous error message, this doesn't provide actionable guidance about which version to use or alternative solutions.
throw new Exception("PackageRestoreContext was not found in the kernel, try using older version of Dotnet.Interactive"); | |
throw new InvalidOperationException("PackageRestoreContext was not found in the kernel. Ensure you are using a compatible version of the Microsoft.DotNet.Interactive library (e.g., version 1.0.0). Refer to the library's documentation or release notes for more details."); |
Copilot uses AI. Check for mistakes.
var disposablesField = typeof(Microsoft.DotNet.Interactive.Kernel).GetField("_disposables", BindingFlags.Instance | BindingFlags.NonPublic); | ||
var disposables = disposablesField?.GetValue(_kernel) as IEnumerable<IDisposable>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reflection-based approach accesses private fields which is fragile. Consider caching the FieldInfo and MethodInfo objects to avoid repeated reflection calls and improve performance.
var disposablesField = typeof(Microsoft.DotNet.Interactive.Kernel).GetField("_disposables", BindingFlags.Instance | BindingFlags.NonPublic); | |
var disposables = disposablesField?.GetValue(_kernel) as IEnumerable<IDisposable>; | |
var disposables = DisposablesField?.GetValue(_kernel) as IEnumerable<IDisposable>; |
Copilot uses AI. Check for mistakes.
catch (Exception ex) when (ex.InnerException is JvmException) | ||
{ | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching all exceptions and only checking InnerException type is too broad. Consider catching more specific exception types or logging the exception for debugging purposes.
catch (Exception ex) when (ex.InnerException is JvmException) | |
{ | |
return false; | |
} | |
catch (JvmException) | |
{ | |
// Handle known JvmException | |
return false; | |
} | |
catch (Exception ex) | |
{ | |
// Log unexpected exceptions for debugging purposes | |
Console.Error.WriteLine($"Unexpected exception in TryGetSparkSession: {ex}"); | |
throw; | |
} |
Copilot uses AI. Check for mistakes.
Assert.True(File.Exists(metadataFilePath)); | ||
Assert.Equal(nugetFile.FullName, actualNugetPath); | ||
Assert.StartsWith(tempDir.Path, actualMetadataPath); | ||
Assert.Matches("dependencyProviderMetadata_[a-f\\d]{8}00000000001", actualMetadataFilename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern uses double backslashes which may be confusing. Consider using a verbatim string (@"dependencyProviderMetadata_[a-f\d]{8}00000000001") for better readability.
Assert.Matches("dependencyProviderMetadata_[a-f\\d]{8}00000000001", actualMetadataFilename); | |
Assert.Matches(@"dependencyProviderMetadata_[a-f\d]{8}00000000001", actualMetadataFilename); |
Copilot uses AI. Check for mistakes.
@@ -54,20 +55,18 @@ public void TestPackageResolver() | |||
}); | |||
|
|||
var packageResolver = new PackageResolver(mockSupportNugetWrapper.Object); | |||
IEnumerable<string> actualFiles = packageResolver.GetFiles(tempDir.Path); | |||
string[] actualFiles = packageResolver.GetFiles(tempDir.Path).ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that actualFiles has length of 2.
|
||
compositeKernel.AddMiddleware(async (command, context, next) => | ||
{ | ||
await next(command, context); | ||
|
||
if ((context.HandlingKernel is CSharpKernel kernel) && | ||
(command is SubmitCode) && | ||
TryGetSparkSession(out SparkSession sparkSession) && | ||
TryGetSparkSession(out SparkSession sparkSession) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format this line.
@@ -39,22 +39,31 @@ public Task OnLoadAsync(Kernel kernel) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that OnLoadAsync is called more than once?
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
This PR fixes support for User-Defined Functions (UDFs) in .NET Interactive environments, including Jupyter and Polyglot Notebooks.
Fixed issues:
DisposableDirectory API was removed:
Switched to own simple implementation, all it does - clears the directory before the core shutdown.
Running any command failed after the dotnet.interactive core restart
(If spark itself was not restarted). Fixed by:
* Adding a random string to the generated metadata filename
* Using
Spark.GetDefaultSession
as a fallback whenSpark.GetActiveSession
returns null. This implementation is identical to the SparkSession.Active, but doesn't throw exceptions when spark is not runningCurrent kernel context doesn't contain ResolvedPackageReferences anymore.
Dotnet.Interactive will consider exposing missing API. As of now, the only way to reach all referenced nuget packages is to extract it from dispose queue from the core. This approach is highly-coupled with the internal implementation of dotnet.interactive, and might break in future. We need to migrate to the dedicated API as soon as it's released. Considered alternative options:
csharpkernel .ScriptState.Script.Options.MetadataReferences
, and extract package paths from there. Build probingPaths and NativeProbingPaths manually. This approach doesn't work, because this collection is of a base type,JustReference
, but File path is only available in a derived class,MetadataReference
, which is also internal and can be accessed only via reflection. And then we need to implement fragile logic to re-create a package structure dotnet.interactive has already built.CommandSucceeded
events instead ofAddMiddleware
. However, I refrained from migrating to this approach as ocasionally it didn't receive events on the successfull command execution, resulting in missing DLL errors.Related issues and discussions
Links: