Skip to content

Conversation

@amoghmc
Copy link
Contributor

@amoghmc amoghmc commented Apr 11, 2025

Add missing C# and F# kernels.
These are needed to create a default kernel else it will return an empty kernel.

Add missing C# and F# kernels.
@amoghmc amoghmc changed the title Update DotnetInteractiveKernelBuilder.cs Add missing C# and F# kernels Apr 11, 2025
@ekzhu ekzhu requested a review from LittleLittleCloud April 14, 2025 18:36
@ekzhu ekzhu added the dotnet issues related to AutoGen.Net label Apr 14, 2025
@BigBigMiao
Copy link

@amoghmc I thought the C# and F# kernel already there by default?

@amoghmc
Copy link
Contributor Author

amoghmc commented Apr 14, 2025

@amoghmc I thought the C# and F# kernel already there by default?

They were but a recent commit removed them so I am adding them back again.

@BigBigMiao
Copy link

@amoghmc Sorry for the confusion, what I meant was wouldn't the C# and F# kernel already added to CompositeKernel when creating by default. And in that case, would explicitly adding C# and F# kernel in InProccessDotnetInteractiveKernelBuilder been unnecessary

What error did you see when you create InProccessDotnetInteractiveKernelBuilder without explicitly adding C# and F# kernel

@amoghmc
Copy link
Contributor Author

amoghmc commented Apr 14, 2025

@BigBigMiao

Error: Microsoft.Dotnet.Interactive.NoSuitableKernelException: No handler registered on kernel csharp for command : Submit code: Console.WriteLine("Hello World");

@amoghmc
Copy link
Contributor Author

amoghmc commented Apr 14, 2025

Although the actual fix was this:

var kernel = DotnetInteractiveKernelBuilder
                .CreateDefaultInProcessKernelBuilder() // add C# and F# kernels
                .AddCSharpKernel(["c#", "C#"])
                // .AddCSharpKernel([]) or this will do as well but then u cannot use c# or C#
                .Build();
var result = await kernel.RunSubmitCodeCommandAsync("Console.WriteLine(\"Hello World\")", "csharp");

This is because .AddCSharpKernel() results in this error below:

Unhandled exception. System.ArgumentException: The kernel name or alias 'csharp' is already in use.

As the default value would end up as ["c#", "C#", "csharp"]
I thought of doing it as a separate pull request. You can use any set of values for the kernel aliases as long as it does not contain "csharp" or "fsharp" incase of AddFSharpKernel()

@LittleLittleCloud LittleLittleCloud enabled auto-merge (squash) April 15, 2025 20:08
@LittleLittleCloud LittleLittleCloud merged commit 71b7429 into microsoft:main Apr 15, 2025
52 checks passed
@codecov
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.02%. Comparing base (88dda88) to head (9d35703).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6283      +/-   ##
==========================================
- Coverage   77.24%   73.02%   -4.23%     
==========================================
  Files         200      306     +106     
  Lines       14473    17591    +3118     
  Branches        0      406     +406     
==========================================
+ Hits        11180    12845    +1665     
- Misses       3293     4473    +1180     
- Partials        0      273     +273     
Flag Coverage Δ
unittests 73.02% <ø> (-4.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@LittleLittleCloud
Copy link
Collaborator

@amoghmc Thanks for the PR, merging now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dotnet issues related to AutoGen.Net

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants