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

bash-kernel: fix and add smoke test #311507

Merged
merged 1 commit into from
May 15, 2024
Merged

Conversation

thomasjm
Copy link
Contributor

Description of changes

The bash-kernel in Nixpkgs is currently broken due to takluyver/bash_kernel#142.

The underlying issue is that bash-kernel uses invisible characters in its prompt, which are currently not being output.

This PR fixes it by using bashInteractive instead of bash. I don't totally understand why this fix works--AFAICT, the main difference between the two is that bashInteractive has the readline library included. In any case, using bashInteractive is probably closer to how most people use bash-kernel, as it just runs whatever bash is on the PATH.

This PR also adds a smoke test, in the form of a test notebook that gets evaluated by Papermill. If this succeeds, we know basic kernel evaluation works.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

CC @zimbatm @teto

Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

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

Haven't tested.


# Evaluate a test notebook with papermill
cd $(mktemp -d)
${python.withPackages (ps: [ps.papermill])}/bin/papermill --kernel bash ${./test.ipynb} out.ipynb
Copy link
Member

Choose a reason for hiding this comment

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

so this would fail without bashInteractive ?
I wonder if it should go through passtru.tests or in checkPhase since papermill becomes a new dependency ? without your fixes, the kernel still works but with a funky prompt ? (sry I read a bit fast)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The kernel is totally broken without bashInteractive. It crashes right away with an error about being unable to parse the prompt.

I don't know about the current best practices regarding passthru.tests. It seems like a good idea to always run a smoke check to prevent regressions like the one we currently have.

If we do the checkPhase option, Papermill would only be a build-time dependency right? So users getting it from the Nixpkgs cache wouldn't experience any change.

@github-actions github-actions bot removed the 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab label May 14, 2024
@thomasjm thomasjm requested a review from teto May 14, 2024 22:48
@teto teto merged commit 1a53d62 into NixOS:master May 15, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants