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: Remove test & runtime dependencies from build time closure tracking issue #272178

Open
1 of 3 tasks
adisbladis opened this issue Dec 5, 2023 · 9 comments
Open
1 of 3 tasks
Labels
0.kind: bug Something is broken 0.kind: enhancement Add something new 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: python

Comments

@adisbladis
Copy link
Member

adisbladis commented Dec 5, 2023

This is a meta tracking issue to aggregate & track issues & PR related to Python build time & runtime separation.

Related issues

Motivations

Stop propagation of Python dependencies from applications

When depending on applications that are written in Python in a nix-shell Python dependencies are propagated into $PYTHONPATH.
This is very problematic when developing Python applications, and a number of packages (notably Poetry & PDM) have hacks that remove $out/nix-support/propagated-build-inputs.
Those hacks could be entirely removed.

Reducing the number of rebuilds

By not using build time propagation we don't even need to add runtime dependencies to the build.
This will drastically cut down on the amount of rebuilds we have to do.

Reducing evaluation overhead

Both through smaller build time closures, but also through possibly getting rid of the packageOverrides idiom (at least internally in nixpkgs).
This idiom is problematic because it means we rebootstrap the Python package set every time it's used.

By delaying composition we can even allow for things which today are not possible because they would lead to file collisions:

python3Packages.buildPythonApplication {
  dependencies = [
    (python3Packages.requests.overridePythonAttrs(old: {
      # This can actually work when we don't enforce propagation at build time!
      # `requiredPythonModules` just needs to take the first dependency for a given name from the list.
      # By putting something earlier in the `dependencies` list they get higher precedence.
    }))
  ];
}

As per the table in my comment a nixpkgs evaluation allocates an attrset with 26755 members 2491 times.
I hunted this down and this is variants of pythonPackages being instantiated.
That's only the tip of the iceberg.

Circular dependencies

In Nix, build time propagation requires the build graph to be a DAG.
This is not true for Python dependencies which sometimes has circular dependencies.

By only depending on the minimal build time dependencies we can reduce the risk of build-time cycles happening to almost zero.
Ensuring no infinite recursion happens would probably be the job of requiredPythonModules.

Expected breakage

I've tried to keep breakage to a minimum, but something has to give for such a drastic semantic change.
The good thing is that most of these issues can be fixed ahead of merging the breaking PR.

python3Packages.foo

Depending on a Python package like this will break:

stdenv.mkDerivation {
  propagatedBuildInputs = [ python3Packages.requests ];
}

You will instead need to:

stdenv.mkDerivation {
  propagatedBuildInputs = python3Packages.requiredPythonModules [ python3Packages.requests ];
}

This is because dependencies are no longer build-time propagated, so the full dependency graph calculation needs to happen in the Nix evaluator.

Note that you don't have to call requiredPythonModules manually when using buildPythonPackage/buildPythonApplication.
This quirk only applies when depending on pythonPackages in non-python contexts.
There are a handful of those cases in nixpkgs but I suspect the most common case is in external development shells:

pkgs.mkShell {
  packages = [ 
    python3
    python3Packages.requests
  ];
}

In those cases you'll need to either call requiredPythonModules or use withPackages/buildEnv.

python3Packages.buildPythonPackage vs python3Packages.buildPythonApplication

In #272179 the distinction between these two functions becomes much more firm:

  • buildPythonPackage is for building a Python package without dependency propagation
  • buildPythonApplication is for building application bundles. This internally calls buildPythonPackage, and also creates a wrapper derivation that takes care of calling requiredPythonModules.

All Python applications which has runnable binaries now needs to either be:

  • Created using buildPythonApplication
  • Converted from a Python module using toPythonApplication

Pull requests

I'm trying my best to allow us to work towards this goal without breaking things unnecessarily.
The first two pull requests in this list doesn't break any current behaviour.
Those allow us to work towards the goal of splitting dependencies incrementally.

The third PR introduces breaking behaviour, but manages to stay mostly compatible, with the breaking changes documented above being the notable exceptions.


Add a 👍 reaction to issues you find important.

cc @FRidh @mweinelt
cc @DavHau @phaer

@roberth
Copy link
Member

roberth commented Dec 5, 2023

cc @FRidh

@FRidh
Copy link
Member

FRidh commented Dec 6, 2023

I've been rather inactive these last years, but I'm happy to see changes like these and I'll try to provide some feedback, though I am pretty sure these items are nothing new. While I prefer we had some Nixpkgs-wide RFC on how we handle package sets, that hasn't really happened, and so I blocking larger changes any longer does not make sense anymore either.

Separate tests

Running tests in a separate derivation we should definitely do, but we do need to ensure Hydra for Nixpkgs will pick this up. Users outside Nixpkgs can stay with separateChecks = false for the time being.

Avoid propagation

At the time I explored various ways to get rid of the current propagation. One way is as done here, using evaluation. The other, was to just write a different file to the store with the runtime dependencies, essentially implementing the propagation ourselves. At the time Eelco was of the opinion we should not do this via the evaluator but instead always use such a file during build-time for performance-reasons. I don't know have any strong preference anymore on whether we use a function or a hook, though I imagine the hook would be easier for polyglot projects.

Circular dependencies

The circular dependencies issue could be resolved by building wheels in a separate derivation. Installation then becomes a very lightweight step.

Interface change

I've been for a while of the opinion that we should aim to get rid of builders such as buildPython* and express everything with just hooks. At the same time, when we split testing out, our users would want some "support" for that, which buildPython* would provide. In my opinion the Nixpkgs Python builder is relatively "low-level" and should stay "close" to semantics used in Nixpkgs, even if that may make it more confusing for users. Convenient interfaces could be provided by third-parties, e.g. using the module system.

@adisbladis
Copy link
Member Author

At the time I explored various ways to get rid of the current propagation. One way is as done here, using evaluation. The other, was to just write a different file to the store with the runtime dependencies, essentially implementing the propagation ourselves. At the time Eelco was of the opinion we should not do this via the evaluator but instead always use such a file during build-time for performance-reasons.

Actually performance was the main reason I got curious to look into this, and I think it's easy to have the wrong intuition about what's causing performance issues in nixpkgs.

I think writing a file to the store with runtime dependencies still wouldn't allow us to get rid of the packageOverrides idiom.
rg -tnix 'python.?\\.override' | wc -l indicates that we override the Python interpreter 60 times in Nixpkgs, meaning that we're computing the Python set from scratch 60 times.
That requirement is a massive performance hog.

@adisbladis adisbladis added the 0.kind: enhancement Add something new label Dec 7, 2023
@FRidh
Copy link
Member

FRidh commented Dec 7, 2023

Where would you do the patching/wrapping of executables in this case? Still in the installed derivation minus the Python run-time dependencies?

@adisbladis
Copy link
Member Author

Where would you do the patching/wrapping of executables in this case? Still in the installed derivation minus the Python run-time dependencies?

Exactly. $PATH would still be patched in the buildPythonPackage build, and the wrapping of the Python environment would happen in the derivation that aggregates the Python environment (buildPythonApplication/python3.withPackages/python3Packages.requiredPythonModules).

@samueldr samueldr added the 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems label Apr 23, 2024
yajo added a commit to moduon/dream2nix that referenced this issue Apr 30, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
yajo added a commit to moduon/dream2nix that referenced this issue Apr 30, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
yajo added a commit to moduon/dream2nix that referenced this issue May 13, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
phaer pushed a commit to phaer/dream2nix that referenced this issue Jun 11, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
@adisbladis
Copy link
Member Author

Open PRs have been rebased. Please take a look, especially at the one separating tests as I think we can merge that before tackling removing dependency propagation.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/several-comments-about-priorities-and-new-policies-in-the-python-ecosystem/51790/1

chaoflow pushed a commit to chaoflow/dream2nix that referenced this issue Nov 16, 2024
NixOS/nixpkgs#271597 implemented a new format for python packages that is starting to get traction in nixpkgs/unstable (soon to become NixOS 24.05).

That is a progress towards NixOS/nixpkgs#272178, which would be a massive improvement for dream2nix once completed.

This first step just makes sure that `buildPythonPackage` supports the new arguments properly. Without this change, many dream2nix python derivations fail to build on nixos-unstable right now.

Probably other python auto-builders should leverage these new options when available for the d2n user. That can be added later.

@moduon MT-1075
@adisbladis
Copy link
Member Author

As I've indicated in #272177 (comment):
I've burnt out on trying to achieve meaningful change within nixpkgs and will not pursue this issue further.

I don't know if I should close this issue or not. The problems with nixpkgs Python still remains.

@ShamrockLee
Copy link
Contributor

As I've indicated in #272177 (comment):
I've burnt out on trying to achieve meaningful change within nixpkgs and will not pursue this issue further.

I'm sorry to hear that.

I don't know if I should close this issue or not. The problems with nixpkgs Python still remains.

I think we should leave this issue open, as people are still looking forward to it being addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug Something is broken 0.kind: enhancement Add something new 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: python
Projects
None yet
Development

No branches or pull requests

7 participants