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

Experimental python bindings #7735

Draft
wants to merge 42 commits into
base: master
Choose a base branch
from
Draft

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Feb 1, 2023

Motivation

Adds simple Python bindings for Nix evaluation, initialized from @Mic92's archived Pythonix (including the license), but then updated to work with the latest Nix version. This relates to #7271

Just like the existing Perl bindings under pkgs.nix.perl-bindings, these Python bindings are available as a separate derivation under pkgs.nix.python-bindings. This is a Python package that can be used as python.withPackages (p: [ pkgs.nix.python-bindings ]

The API currently is just a single function:

  • nix.eval(str, vars: dict)
    Evaluates the Nix expression str with variables vars in scope. vars is transparently converted from Python values to Nix values. The result of this function is transparently converted from a Nix value to a Python value.

An example:

flake.nix
{
  inputs.nix.url = "github:tweag/nix/python-bindings";
  inputs.nixpkgs.follows = "nix/nixpkgs";
  inputs.flake-utils.url = "github:numtide/flake-utils";

  outputs = { nix, nixpkgs, flake-utils, ... }: flake-utils.lib.eachDefaultSystem (system: {
    defaultPackage = nixpkgs.legacyPackages.${system}.pkgs.python3.withPackages (p: [
      nix.packages.${system}.nix.python-bindings
    ]);
  });
}
$ nix run
Python 3.10.8 (main, Oct 11 2022, 11:35:05) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import nix
>>> nix.eval("1 + a", vars=dict(a = 2))
3

This work is sponsored by Antithesis

TODO

Context

The motivation for this is better tests for library functions in nixpkgs. Previous efforts:

In particular NixOS/nixpkgs#212858 highlighted that property tests are currently either slow (if using the Nix CLI) or limited (if you work around this by cramming everything into a single Nix CLI call, which then means you can't test error messages anymore).

I then instead focused on Pythonix, third-party Python bindings for the Nix evaluator. By using Python for testing we can use the Python ecosystem (such as property testing using Hypothesis), and by it linking to the Nix library it's much faster and we can test error messages too. Unfortunately Pythonix was archived due to being too hard to maintain, since the library interface isn't stable. So the fix is to just upstream it instead.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

python/LICENSE.md Outdated Show resolved Hide resolved
Comment on lines 45 to 53
// TODO: Only do this once
// By default, Nix sets the build-hook to be "$(readlink /proc/self/exe) __build-remote", expecting the current binary to be Nix itself.
// But when we call the Nix library from Python this isn't the case, the current binary is Python then
// So we need to change this default, pointing it to the Nix binary instead
// Because we can't know the path to the Nix binary from here, we use a preprocessor macro,
// which we can then influence from Meson, which we can then influence from the Nix build definition
nix::settings.buildHook = NIX_BUILD_HOOK;
// And by setting buildHook before calling initNix, we can override the defaults without overriding the user-provided options from the config files
nix::initNix();
Copy link
Member Author

Choose a reason for hiding this comment

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

@roberth I am not yet fully sure how this relates to #7732, but this might interest you

Copy link
Member

Choose a reason for hiding this comment

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

initNix() can only be called once. You can use std::call_once or the static local initializer trick, e.g.

    [[maybe_unused]] static auto init = [&]() -> bool
    {
        initNix();
        return true;
    }();

Copy link
Member

Choose a reason for hiding this comment

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

Notice that nix also installs signal handler which will not play well with Python ones. I disabled those in my project like this: nix-community/harmonia@d8af988

Copy link
Member Author

Choose a reason for hiding this comment

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

It's now called only once by using the python module initialization code that was already used for initGC:

// By default, Nix sets the build-hook to be "$(readlink /proc/self/exe) __build-remote", expecting the current binary to be Nix itself.
// But when we call the Nix library from Python this isn't the case, the current binary is Python then
// So we need to change this default, pointing it to the Nix binary instead
nix::settings.buildHook = nix::settings.nixBinDir + "/nix";
// And by setting buildHook before calling initNix, we can override the defaults without overriding the user-provided options from the config files
nix::initNix();
nix::initGC();

@edolstra
Copy link
Member

edolstra commented Feb 2, 2023

The problem with upstreaming this is that I don't know anything about Python internals, yet changes to the C++ API would require me to update/fix the Python bindings. That's why it's probably better to keep this as an external project (or perhaps as a separate flake in the Nix repo).

flake.nix Outdated

export PYTHONPATH=$out/${python.sitePackages}

ninja test
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should use ninja here if we're not using it elsewhere yet. See NixOS/rfcs#132.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe, but since this is morally a separate package, just one versioned in lockstep, I don't think it is so bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out it's pretty much essential to not use a separate build system here, because otherwise it's really hard to iteratively work on these bindings, since the two build systems can't really call each other nicely, meaning a nix build is always required to actually test whether the bindings work whenever a change in Nix is done.

I'll consider a fast development cycle a requirement for this PR. I'll see if I can either make meson hackily call into autotools and depend on its built nix, or probably better, switching to autotools for the python bindings as well.

Copy link
Member

@Ericson2314 Ericson2314 Feb 2, 2023

Choose a reason for hiding this comment

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

@infinisil we currently do not have that for the Perl bindings, right? However if both projects were meson we would have that very nicely! https://mesonbuild.com/Subprojects.html

So I therefore consider it future work blocked on the RFC.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out it's pretty much essential to not use a separate build system here, because otherwise it's really hard to iteratively work on these bindings, since the two build systems can't really call each other nicely

Nix proper and the bindings should be loosely coupled projects. Of course this is funny for something called a binding, but hear me out.

Nix exposes an interface

  • A change in the bindings should generally be possible to iterate on without having to change Nix.
  • A change in Nix should generally be possible to iterate on without having to change the binding.

Nix is a unit, the bindings are a unit. Each come with their own tests.

Sometimes, interfaces need to change. Such a change should be fully implemented in Nix proper first, because the fewer units you test at a time, the shorter your iteration cycles.

With these rules in mind, the workflow for an interface change will look like

  1. Think of a change that affects both Nix and the binding
  2. Write a unit test for the Nix part that the binding will need
  3. Iterate using the unit test
  4. Run the integration tests by means of nix build
  5. Write a unit test for the binding
  6. Iterate on the binding test

It'd be rare to have to iterate through 4, because that would imply that either the test (2) insufficient, or because you're in a process of validating the idea (1). The prior is just a professional growth opportunity (arrogant as that sounds; sorry), whereas the latter is unlikely to be needed for something as conceptually simple as a language binding.

The alternative is to forego componentization and always build everything and run all tests. While this might lead to a slightly simpler process for the bindings, it leads to a scalability problem for Nix, as it has to always build its bindings in order to be tested during the human part of the development cycle.

Anyway, going back to the unified build system; yes, incremental builds across the nix package interface would be helpful, but are not necessary for a good workflow with short cycle times.

Copy link
Member Author

Choose a reason for hiding this comment

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

@roberth I think I get the idea, it would be similar to the plugins tests (which btw have been broken multiple times).

I don't think it would work very well here though, because they would have to essentially just duplicate what the bindings do, which then becomes a synchronization effort (every time you change the bindings you need to ensure the test covers the same behavior). And the only benefit I can see is a bit faster iteration times, I don't think that's worth it.

For the plugin tests this makes much more sense because that's supposed to be a stable third-party interface, meaning there's third-party code we can't test or change. The in-tree plugin test then is representative of those third-parties and (should) guard against breaking them.

Concretely for this PR, since the Nix team agreed to merge it once ready, the next best step imo is to just make sure the bindings can be iterated over quickly, somehow.

Copy link
Member

Choose a reason for hiding this comment

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

Concretely for this PR, since the Nix team agreed to merge it once ready, the next best step imo is to just make sure the bindings can be iterated over quickly, somehow.

Yeah as long as it's not a blocker for merging this, that's fine.

If this, Nix, and Hydra all use Meson, we can subproject everything and develop all 3 simultaneously which would be fantastic.

python/src/eval.cc Outdated Show resolved Hide resolved
const char *currentExceptionTypeName() {
int status;
Copy link
Member

Choose a reason for hiding this comment

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

If upstreamed, this should be formatted consistently with the other C++ code in Nix, e.g.

Suggested change
const char *currentExceptionTypeName() {
int status;
const char * currentExceptionTypeName()
{
int status;

See #6721 for a .clang-format that may help.

Copy link
Member

Choose a reason for hiding this comment

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

I could configure a pre-commit hook and ci check for a subset of files without much effort.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like @roberth since opened #7745 to only check a subset of files, but @edolstra thinks that #6721 would be better.

I'd rather avoid making this PR dependent on these decisions, so for now I just formatted all the files using the .clang-format config file from the above-linked PR's (it's the same for both).

Copy link
Member

Choose a reason for hiding this comment

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

Much appreciated, @infinisil.

This conversation would not have been needed if #7745 was merged.

Comment on lines 45 to 53
// TODO: Only do this once
// By default, Nix sets the build-hook to be "$(readlink /proc/self/exe) __build-remote", expecting the current binary to be Nix itself.
// But when we call the Nix library from Python this isn't the case, the current binary is Python then
// So we need to change this default, pointing it to the Nix binary instead
// Because we can't know the path to the Nix binary from here, we use a preprocessor macro,
// which we can then influence from Meson, which we can then influence from the Nix build definition
nix::settings.buildHook = NIX_BUILD_HOOK;
// And by setting buildHook before calling initNix, we can override the defaults without overriding the user-provided options from the config files
nix::initNix();
Copy link
Member

Choose a reason for hiding this comment

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

initNix() can only be called once. You can use std::call_once or the static local initializer trick, e.g.

    [[maybe_unused]] static auto init = [&]() -> bool
    {
        initNix();
        return true;
    }();

flake.nix Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member

I don't know anything about Python internals, yet changes to the C++ API would require me to update/fix the Python bindings.

I don't know anything about Perl internals, but I have been able to fix issues in the Perl bindings I've created. That is basically one generally just needs to fix the C++ part, and the more "infra" bits stay the same.

That makes me think the Python binds would also not be a maintenance problem upstream.

infinisil added a commit to tweag/nix that referenced this pull request Feb 2, 2023
Copies part of the tree from Pythonix at
Mic92/pythonix@fbc8490
into the ./python subdirectory.

These Python bindings don't build yet with the current Nix version,
which is why they're not built yet in this commit.

Mic92 confirmed that he's okay with the license being changed to the Nix
one: NixOS#7735 (comment)

Co-Authored-By: Jörg Thalheim <joerg@thalheim.io>
@infinisil
Copy link
Member Author

@Ericson2314 @edolstra Yeah, I don't think this is very hard. I'm neither experienced at C++ nor Python, but I was able to update the bindings from like 2.4 to the latest 2.13.1 in like an hour, you can see the necessary changes here.

And in fact, these python bindings don't even have any Python code, it's all just C++ code using the Python C API. Although this could change later, the API surface is very small right now, but it's the only subset I need.

@edolstra
Copy link
Member

edolstra commented Feb 3, 2023

Nix team discussion: In principle we're in agreement that we want to merge this. However, we would like some kind of stable C++ API for language bindings to use, to minimize the amount of language-specific breakage that the Nix maintainers would have to deal with, and also to help with creating bindings for other languages. We'll think about that this C++ API would look like.

@thufschmitt
Copy link
Member

Some more notes from the team's meeting:

  • Do we want python bindings here?
    • Helpful because an external thing binding to an unstable API might break at any time
    • Also would blesse it as official
  • Issue: Requires the maintainers to maintain this API
    • And we don't particularely know the python-C system
      • Not the end of the world because it's generally not too hard to change, but still an annoyance
    • Maybe we could have a stable c++ interface so that the python API doesn't have to be touched too much
      • Would increase the maintenance cost
      • Would allow for other bindings to build on top of it
  • Decision:
    • Merge it when ready
    • Ask for a follow-up refactor to have a stable C++ “middleware” interface
    • Make it explicitely experimental (as we can, probably package metadata or something)
    • Need to be explicit about the promises:
      • We accept python because it's nice and orthongonal to C++ as to the use-cases
      • We don't make any promise as to what else we want to support

@Mic92
Copy link
Member

Mic92 commented Feb 3, 2023

Just to give you some datapoints, when I was maintaining pythonix, I had a few times to fix the nix C++ api part but never the python API. The only main changes I can remember happened in the 2 to 3 transition and there it was still less bad compared to the python stdlib and language changes.. I think in fact that python developers are scared to break it because there are many native libraries in the ecosystem that are important but not so well maintained.

@infinisil
Copy link
Member Author

  • Merge it when ready

  • Ask for a follow-up refactor to have a stable C++ “middleware” interface

I think rather than a stable C++ API, it would be better to have a stable client RPC interface, e.g. using gRPC and protobuf. And arguably the daemon protocol should also use the same tech stack. This makes it much easier and safer for other languages.

This also paves the way for a future where Nix might be rewritten in another language, in which case a C++ binding would certainly not be viable anymore. Related to Tvix too of course, which could also be a server for such a stable interface.

This might also be a great step towards Flakes just being a CLI frontend, which then makes calls the stable client RPC interface to actually do any Nix evaluation. Also this sounds like a good step towards making the "Nix language" its own thing entirely

@roberth
Copy link
Member

roberth commented Feb 3, 2023

RPC interface

An RPC interface is going to suffer from a flexibility vs latency tradeoff that bindings just avoid.
I think any IPC interface for the evaluator is better to be layered atop the in-process API that we wish to move towards; a C api or stable C++ one.

This might also

Perhaps. We'll have to make some big changes in little more than one step at a time.

@Ericson2314
Copy link
Member

I do sort of think a nice rule is that in-tree bindings get deprecated once there is an out of tree separate implementation. For example, we might now haskell bindings in tree since hnix is very incomplete, but if it were finished, I would be happy switching over. Or another example is something like tvix could do do the language part, so any Rust bindings can just focus on the store layer.

I bring this up as a heuristic for the "bindings vs RPC" question.

@infinisil infinisil force-pushed the python-bindings branch 4 times, most recently from 098a31b to cc11189 Compare February 7, 2023 04:01
@infinisil
Copy link
Member Author

After a bunch of hacking I got incremental compilation to work! Yet to be documented, but it's now possible to build the bindings like this:

$ nix develop .#python
warning: Git tree '/home/tweagysil/src/nix' is dirty

[tweagysil@zion:~/src/nix]$ cd python

[tweagysil@zion:~/src/nix/python]$ make
make -C .. python/build
make[1]: Entering directory '/home/tweagysil/src/nix'
make[1]: 'python/build' is up to date.
make[1]: Leaving directory '/home/tweagysil/src/nix'
meson compile -C build
ninja: Entering directory `/home/tweagysil/src/nix/python/build'
ninja: no work to do.

This still uses meson, but it hooks into Nix's main Makefile to ensure that libexpr and libmain are built and up-to-date.

infinisil added a commit to tweag/nix that referenced this pull request Feb 7, 2023
Copies part of the tree from Pythonix at
Mic92/pythonix@fbc8490
into the ./python subdirectory.

These Python bindings don't build yet with the current Nix version,
which is why they're not built yet in this commit.

Mic92 confirmed that he's okay with the license being changed to the Nix
one: NixOS#7735 (comment)

Co-Authored-By: Jörg Thalheim <joerg@thalheim.io>
flake.nix Outdated
// { default = self.devShells.${system}.stdenv; }
// {
default = self.devShells.${system}.stdenv;
python = self.packages.${system}.nix.python-bindings.shell;
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to have a python/flake.nix, so one could just cd python; nix develop, but that doesn't work since the main Nix flake and the Python bindings one would depend on each other, and the subflake updating doesn't work very well.

I could create a shell.nix so that cd python; nix-shell would work (using builtins.getFlake on the parent), let me know if that's better.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's not independent, so not a subflake. Either subflakes could be made to be more useful, or we design devshells so that they can be declared for a specific subdirectory. While we don't have that, I think a shell.nix is nice.

@infinisil
Copy link
Member Author

Reminder for myself: @yorickvP pointed out that this CI Darwin failure:

ImportError: dlopen(/private/tmp/nix-build-python3.10-nix.drv-0/source/python/build/src/nix.so, 0x0002):
  symbol not found in flat namespace (__Py_FalseStruct)

Might need code like this to be fixed:

nix/flake.nix

Lines 320 to 325 in 0a7071e

${lib.optionalString currentStdenv.isDarwin ''
for LIB in $out/lib/*.dylib; do
chmod u+w $LIB
install_name_tool -id $LIB $LIB
install_name_tool -delete_rpath ${boost}/lib/ $LIB || true
done

@yorickvP
Copy link
Contributor

yorickvP commented Feb 10, 2023

Reminder for myself: @yorickvP pointed out that this CI Darwin failure:

ImportError: dlopen(/private/tmp/nix-build-python3.10-nix.drv-0/source/python/build/src/nix.so, 0x0002):
  symbol not found in flat namespace (__Py_FalseStruct)

Might need code like this to be fixed:

nix/flake.nix

Lines 320 to 325 in 0a7071e

${lib.optionalString currentStdenv.isDarwin ''
for LIB in $out/lib/*.dylib; do
chmod u+w $LIB
install_name_tool -id $LIB $LIB
install_name_tool -delete_rpath ${boost}/lib/ $LIB || true
done

Actually, this might be a case of https://stackoverflow.com/a/39519008

Which is a dependency lib version mismatch, somewhere

@infinisil
Copy link
Member Author

This section might be useful, we could also try building the bindings with distutils instead of meson

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-02-03-nix-team-meeting-minutes-29/25436/1

@infinisil
Copy link
Member Author

I cannot reproduce the Darwin failure locally on an x86_64-darwin macOS Catalina 10.15.7 machine, nor on an arm64 macOS Montery 12.6.2.

CI fails on x86_64-darwin macOS Monterey 12.6.3, so I might have to upgrade my local x86_64-darwin machine to reproduce this :/

infinisil and others added 21 commits April 19, 2023 14:53
It didn't cause a problem because it converted True to 1, which still
allowed 1 == True to succeed in Python (using assertEqual)
Using a symlink hacky and breaks in some ways
So that we don't have to rebuild Nix every time the python bindings
change
- BASH_SOURCE[0] doesn't exist for evaluations under `eval`
- NIX_STORE may not be set
The former is deprecated
Previously the python bindings derivation would use Nix as its source,
call configure on that, just to extract the two files tests/init.sh and
(the generated one) tests/common/vars-and-functions.sh

The new approach is to instead have a separate derivation extracting
these two files and having a small wrapper around them
Tests that the Nix bindings can be used as a Python dependency in Nix
builds

## Documentation

See [index.md](./doc/index.md), which is also rendered in the HTML manual.
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find where they are rendered in the manual.

```python
nix.callExprString(expression: str, arg)
```
Parse a nix expression, then call it as a nix function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Parse a nix expression, then call it as a nix function.
Parse a Nix expression from the given string, then call it as a Nix function.

This needs some more information, such as whether it writes to the store when evaluating derivations and such. Also, how to pass parameters to the evaluator?

Note that this function is experimental and subject to change based on known issues and feedback.

**Parameters:**,
`expression` (str): The string containing a nix expression.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`expression` (str): The string containing a nix expression.
`expression` (str): The string containing a Nix expression.

`arg`: the argument to pass to the function

**Returns:**
`result`: the result of the function invocation, converted to python datatypes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`result`: the result of the function invocation, converted to python datatypes.
`result`: the result of the function invocation, converted to Python data types.

This needs some more information on the data type mapping between Nix and Python.

Comment on lines +22 to +24
In the future these Python bindings will be available from Nixpkgs as `python3Packages.nix`.

Until then the Python bindings are only available from the Nix derivation via the `python-bindings` [passthru attribute](https://nixos.org/manual/nixpkgs/stable/#var-stdenv-passthru). Without any modifications, this derivation is built for the default Python 3 version from the Nixpkgs version used to build Nix. This Python version might not match the Python version of the project you're trying to use them in. Therefore it is recommended to override the bindings with the correct Python version using
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should make promises about the future in a manual.

Suggested change
In the future these Python bindings will be available from Nixpkgs as `python3Packages.nix`.
Until then the Python bindings are only available from the Nix derivation via the `python-bindings` [passthru attribute](https://nixos.org/manual/nixpkgs/stable/#var-stdenv-passthru). Without any modifications, this derivation is built for the default Python 3 version from the Nixpkgs version used to build Nix. This Python version might not match the Python version of the project you're trying to use them in. Therefore it is recommended to override the bindings with the correct Python version using
The Python bindings are only available from the Nix derivation via the `python-bindings` [passthru attribute](https://nixos.org/manual/nixpkgs/stable/#var-stdenv-passthru). Without any modifications, this derivation is built for the default Python 3 version from the Nixpkgs version used to build Nix. This Python version might not match the Python version of the project you're trying to use them in. Therefore it is recommended to override the bindings with the correct Python version using

@roberth roberth added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Jun 2, 2023
@roberth
Copy link
Member

roberth commented Jun 13, 2023

I just realised that the nixos-option program would make for a good use case to validate the C interface.

@yorickvP How's it going?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.