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

Python bindings #1036

Merged
merged 9 commits into from
Jan 16, 2023
Merged

Python bindings #1036

merged 9 commits into from
Jan 16, 2023

Conversation

GuillaumeDesforges
Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges commented Jan 9, 2023

This PR adds very simple Python bindings. Fixes #820.

It builds a package nickel-lang with a module nickel_lang containing a single function run which takes in a str of a Nickel expression and returns the result as str.

The Python module is built using maturin.
Note that building the Python modules part is Rust is optional, the binary shouldn't be bigger.

To test this PR:

  • go to pyckel folder
  • create a Python virtual environment and activate it
    python -m venv .venv
    source .venv/bin/activate
  • install maturin
    pip install "maturin>=0.14,<0.15"
  • build and install the nickel_lang Python module to the local venv
    maturin develop
  • test with Python
    $ python
    >>> import pyckel
    >>> pyckel.run("let x = 1 in x + 3")
    '4'

This PR does not

  • implement proper Python exceptions: all Nickel errors are just dumped as their Debug formatted string to a NickelException

Todo:

  • run should return JSON
  • run should evaluate fully

Cargo.toml Outdated Show resolved Hide resolved
src/python.rs Outdated Show resolved Hide resolved
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

LGMT overall 👍

The CI is failing, presumably because now building the dependencies of the workspace implies to build pyo3 which requires python. I guess it's fine to do so in the checks, but not when just building e.g. Nickel.

But that may not be totally straightforward to achieve right now, not because of any Nix or Crane limitations, but because of how the flake is organized. It seems we always builds the dependencies of the whole workspace before anything else.

Cargo.toml Outdated Show resolved Hide resolved
pyckel/README.md Outdated Show resolved Hide resolved
@GuillaumeDesforges
Copy link
Contributor Author

GuillaumeDesforges commented Jan 13, 2023

Trying to fix the error from the CI here: https://github.com/tweag/nickel/actions/runs/3876173138/jobs/6609682802

nickel-lang-deps> error: failed to run custom build command for `pyo3-build-config v0.17.3`
nickel-lang-deps> Caused by:
nickel-lang-deps>   process didn't exit successfully: `/build/source/target/release/build/pyo3-build-config-bd0e0341d2a36725/build-script-build` (exit status: 1)
nickel-lang-deps>   --- stdout
...
nickel-lang-deps>   --- stderr
nickel-lang-deps>   error: no Python 3.x interpreter found

I can fix this issue with a patch on flake.nix, adding Python when building the Rust deps:

diff --git a/flake.nix b/flake.nix
index 1c6fd331..580368cc 100644
--- a/flake.nix
+++ b/flake.nix
@@ -196,6 +196,7 @@
             inherit
               src
               cargoExtraArgs;
+            buildInputs = [ pkgs.python3 ];
           };

           nickel = craneLib.buildPackage {

but then building the nickel package with Nix fails with the error

$ nix flake checks
error: builder for '/nix/store/hz4gvqkjf22xlhd612amzic1a07jm3n6-nickel-lang-0.3.1.drv' failed with exit code 101;
       last 10 log lines:
       > ++ command cargo build --profile release --message-format json-render-diagnostics --frozen --offline --workspace
       >    Compiling pyo3-build-config v0.17.3
       > error: failed to run custom build command for `pyo3-build-config v0.17.3`
       >
       > Caused by:
       >   could not execute process `/build/source/target/release/build/pyo3-build-config-bd0e0341d2a36725/build-script-build` (never executed)
       >
       > Caused by:
       >   Permission denied (os error 13)

@GuillaumeDesforges
Copy link
Contributor Author

I think I fixed the Nix build. I removed --workspace from the cargo build extra arguments, and created a derivation per package.

Note that there is no derivation for the Python bindings (pyckel) since I have not yet managed to make it work. I suggest to tackle that in another PR.

@GuillaumeDesforges
Copy link
Contributor Author

I've requested reviews from @sir4ur0n and @matthew-healy since they have worked on the Nix flake not too long ago.

Copy link
Contributor

@sir4ur0n sir4ur0n left a comment

Choose a reason for hiding this comment

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

I haven't reviewed actual pyckel part, only the Nix files

flake.nix Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
lsp/client-extension/default.nix Outdated Show resolved Hide resolved
@matthew-healy
Copy link
Contributor

Thanks for tagging me, but I'm gonna remove myself from this PR since my experience editing the flake was pretty limited. I think Julien has a lot more context than I do on how it actually works.

@matthew-healy matthew-healy removed their request for review January 16, 2023 10:33
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. The CI running time is suspiciously high, but you've changed the nickel-lang-deps derivation, which is the cornerstone of the build, so it might just be that this PR invalidated all previous cached artifacts. I took another look at the crane doc, and the current PR, and I don't see why the artifacts wouldn't be cached again in the future.

Let's see in subsequent PRs if something looks wrong with respect to CI time.

Bringing python in is surprising for people who don't care about python bindings, suggesting that said bindings could also be in their own repo. But I guess Python is a well-cached, not-too-big derivation, and it's fine as it is for now.

flake.nix Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@sir4ur0n sir4ur0n left a comment

Choose a reason for hiding this comment

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

Approving as in "Nothing looks off in the Nix code". I haven't tried it, nor looked at the Python/Rust part

@yannham
Copy link
Member

yannham commented Jan 16, 2023

Ok, let's keep an eye on CI times, but otherwise LGTM.

@yannham yannham merged commit 407afdb into tweag:master Jan 16, 2023
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.

Python bindings
4 participants