Skip to content
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

dotnet kernel for C# and F# #547

Merged
merged 30 commits into from
Nov 24, 2024
Merged

dotnet kernel for C# and F# #547

merged 30 commits into from
Nov 24, 2024

Conversation

anpin
Copy link
Contributor

@anpin anpin commented Nov 22, 2024

This PR adds dotnet kernel to C# and F# languages in jupyter notebooks.

This PR depends on refactorings made in #524

Closes #393

Copy link

netlify bot commented Nov 22, 2024

Deploy Preview for tweag-jupyterwith ready!

Name Link
🔨 Latest commit 51788ef
🔍 Latest deploy log https://app.netlify.com/sites/tweag-jupyterwith/deploys/6743031011969400087e2924
😎 Deploy Preview https://deploy-preview-547--tweag-jupyterwith.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@GTrunSec
Copy link
Collaborator

@anpin Thanks for your contributing. once you solve the CI to be green, I will merge your PR.

@GTrunSec
Copy link
Collaborator

After merging this PR, we should have a plan to migrate the poetry to the dream2nix. like https://github.com/hardenedlinux/AISecurity-Research-Template/blob/main/flake.nix

@anpin
Copy link
Contributor Author

anpin commented Nov 23, 2024

@anpin Thanks for your contributing. once you solve the CI to be green, I will merge your PR.

removed dotnet-sdk from requiredRuntimePackages which fixed nix flake check locally

@anpin
Copy link
Contributor Author

anpin commented Nov 23, 2024

that's weird. can't reproduce these failures locally.

@GTrunSec
Copy link
Collaborator

@anpin Did you run this commandnix build -L .#jupyterlab-kernel-example-dotnet-csharp locally? since I can test them tomorrow night, please check your test.py, and make sure everything goes well.

@anpin
Copy link
Contributor Author

anpin commented Nov 24, 2024

yes I do. There is no output in the case of a successful test, so it works fine. I don't have dotnet-sdk in my $PATH. And I can run the kernel and confirm it works from jupyter.

image

@anpin
Copy link
Contributor Author

anpin commented Nov 24, 2024

added openssl and zlib to runtimeDeps, maybe that will fix CI

pname = "Microsoft.dotnet-interactive";
version = "1.0.556801";

nugetSha256 = "sha256-tABt/DltggX85SZaaZK7ZP+L3EqxEh0fZ1pfB4MOtxk=";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually this should be upstreamed to nixpkgs and built from source

@anpin
Copy link
Contributor Author

anpin commented Nov 24, 2024 via email

@anpin
Copy link
Contributor Author

anpin commented Nov 24, 2024

I disabled nix-ld and reverted the last commit with runtimeDeps, but that did not make it fail. The kernel test executed successfully.

I managed to use lib.mapAttrs' on the example set to run test.py in nixos flake check VM for every kernel. It seems to work for per-existing kernels, but those added in this PR fail with a different error than CI build above.

@anpin
Copy link
Contributor Author

anpin commented Nov 24, 2024

If running test.py in nix falke check is desired by maintainers I can submit separate PR or include it in this one. Here is the gist of it

diff --git a/flake.nix b/flake.nix
index c5d35a5..3a4f4ed 100644
--- a/flake.nix
+++ b/flake.nix
@@ -133,6 +133,40 @@
 
         exampleJupyterlabAllKernelsNew =
           jupyterLib.mkJupyterlabNew (builtins.attrValues examples);
+
+        checkKernel = name: example:
+          with import (nixpkgs + "/nixos/lib/testing-python.nix") {inherit system;}; let
+            kernel = jupyterLib.mkJupyterlabNew example;
+            test_py = (builtins.dirOf example) + "/test.py";
+          in
+            makeTest {
+              inherit name;
+              nodes.machine = {config, ...}: {
+                config = {
+                  environment.systemPackages = [kernel];
+                };
+              };
+              testScript = ''
+                machine.execute("${kernel}/bin/python ${test_py}")
+              '';
+            };
+        exampleCheck = (
+          lib.mapAttrs'
+          (
+            name: value:
+              lib.nameValuePair
+              ("check-" + name)
+              (checkKernel name value)
+          )
+          examples
+        );
       in {
         lib =
           jupyterLib
@@ -163,9 +197,11 @@
             ${pre-commit.shellHook}
           '';
         };
-        checks = {
-          inherit pre-commit;
-        };
+        checks =
+          {
+            inherit pre-commit;
+          }
+          // exampleCheck;
         apps = {
           update-poetry-lock =
             flake-utils.lib.mkApp

Copy link
Collaborator

@GTrunSec GTrunSec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GTrunSec GTrunSec merged commit b9dcda0 into tweag:main Nov 24, 2024
30 of 31 checks passed
@anpin
Copy link
Contributor Author

anpin commented Nov 24, 2024

thanks for merging it @GTrunSec!

I've added another commit with above patch which allows to test any kernel via nix build .\#checks.x86_64-linux.check-example-dotnet-fsharp -L --show-trace. Would you consider such change in a new PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: F# kernel
2 participants