pre-commit: fixing import bug for built-in hooks #251418

MatrixManAtYrService · 2023-08-25T17:49:35Z

Context

Pre-commit has built-in "meta" hooks. If they are isolated from pre-commit itself, they don't work.

#235123 prevented pre-commit's dependencies from propagating into devshells. This was necessary because those dependencies were clobbering others in the devshell definition.

This increased isolation broke the "meta" hooks. A workaround emerged (see comments below) which involved configuring the pre-commit wrapper script to always run with its PYTHONPATH set such that the pre-commit internals are accessible.

The workaround clutters up the devshell definition, and it needlessly exposes system type hooks to pre-commit internals. Really, only the meta type hooks need to see pre-commit internals.

Description of changes

This PR patches pre-commit so that it treats meta and system hook types separately, ensuring that pre-commit itself (via PYTHONPATH) is available only to the meta hooks. This allows users to just add pkgs.pre-commit to their devshell without thinking about PYTHONPATH at all.

Things done

MatrixManAtYrService · 2023-08-25T17:53:18Z

Where I broke this: #235123
A previous attempt to fix it: #241698
Fixes: #238345

MatrixManAtYrService · 2023-08-26T18:03:43Z

Pre-commit has a "language" called system (docs) which lets you bypass pre-commit's encapsulation and just run commands in your "system".

By setting the pythonpath in the wrapper, we've fixed what we set out to fix. But we're clobbering pre-commit's idea of what's available in "system". In my case, I've got python311 in the calling shell, but my system hooks are using python310 because that's the version that pre-commit is packaged with.

Perhaps the wrapper needs to either:

merge the calling environment with the packaged values instead of clobbering the former with the latter
make note of what's in the env before clobbering it so that system hooks can un-clobber

To reproduce:

# flake.nix
{
  inputs = {
    flake-utils.url = "github:numtide/flake-utils";
    nixpkgs.url = "github:MatrixManAtYrService/nixpkgs?ref=fix-pre-commit";
  };

  outputs = { self, nixpkgs, flake-utils,}:
    flake-utils.lib.eachDefaultSystem (system:
      let
        pkgs = import nixpkgs {
          inherit system;
        };
      in rec
      {

        devShells.default = pkgs.mkShell {
          packages = [
            pkgs.python311
            pkgs.pre-commit
          ];
        };
      });
}

# .pre-commit-hooks.yaml
---
repos:
  - repo: local
    hooks:
      - id: print-python-version
        name: print-python-version
        language: system
        entry: python --version
        pass_filenames: false

default_language_version:
  python: python3.11

❯ python --version
Python 3.11.4

❯ pre-commit run --verbose
print-python-version.....................................................Passed
- hook id: print-python-version
- duration: 0.02s

Python 3.10.12

MatrixManAtYrService · 2023-08-26T22:29:16Z

Ok, I fixed it by patching pre-commit to only expose the calling environment's PATH and PYTHONPATH to the hook runner if they're written in the system language. This broke the internal hooks (which are also system), so I created a new language (meta) for these.

meta -> sees PATH and PYTHONPATH as prepared by nix
system -> sees the calling environment's PATH and PYTHONPATH
others -> pre-commit manages its environment so that its is isolated from both (no change here)

meta hooks are hard-coded into pre-commit. So far as I know, nobody is extending them. So this distinction need not be thought about by users (:crossed_fingers:).

MatrixManAtYrService · 2023-09-04T15:28:56Z

pandas has a nontrivial .pre-commit-config.yaml. I've been testing this by adding the following flake to that repo and running its pre-commit hooks.

{
  inputs = {
    flake-utils.url = "github:numtide/flake-utils";

    # switch the comment below to activate the other codebase
    nixpkgs.url = "github:MatrixManAtYrService/nixpkgs?ref=fix-pre-commit";
    # nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";

  };

  outputs = { self, nixpkgs, flake-utils }:
    flake-utils.lib.eachDefaultSystem (system:
      let
        pkgs = import nixpkgs {
          inherit system;
        };
      in
      {

        devShells.default = pkgs.mkShell {
          packages = [
            pkgs.pre-commit
            pkgs.nodejs
          ];
        };
      });
}

Ruff is still a problem, but it's not a problem for this PR. As for everything else, this PR fixes a wide swath of hooks, all of which previously failed with errors like:

Unwanted patterns.......................................................................................Failed
- hook id: unwanted-patterns
- exit code: 1

/nix/store/pkj7cgmz66assy7l18zc7j992npb41nx-python3-3.10.12/bin/python3.10: Error while finding module specification for 'pre_commit.languages.pygrep' (ModuleNotFoundError: No module named 'pre_commit')

Also, it introduces no new failures.

It seems pretty clear that it's much better than it was before. I think it can be merged.

MatrixManAtYrService · 2023-09-04T15:41:20Z

@FRidh you made a very helpful comment on my previous attempt to address this issue, thanks for that. Unfortunately I introduced problems with that PR. It's a little more invasive, but I think this actually does trick while also preventing those problems. Can I get your thoughts and/or review on it?

MatrixManAtYrService · 2023-09-04T15:49:58Z

Thanks to @anund for pointing me at the wrapper script as a place to address this kind of problem. I've had to go a bit further than you recommended but thanks to that help I think I'm on the right path here. If you have any thoughts about this approach, they'd be valued.

anund · 2023-09-10T07:06:56Z

@MatrixManAtYrService I think you may be able to just define the python version your dev shell uses rather than patching pre-commit. This comes from thinking about what the 'system' is for your example. if pre-commit is using the wrong python version and you're setting that version in nix develop the question that came to mind was "how do you tell pre-commit how to use a different python?". First I look at override, and overrideAttrs which cover swapping things around at a slight cost on not getting upstream caching for free anymore.

To reproduce I had to adjust somethings in the example flake.nix you provided. I need a version of nixpkgs with the bug and to use overridePythonAttrs on pre-commit to add in just the makeWrapperArgs change.

#flake.nix
{
  inputs = {
    flake-utils.url = "github:numtide/flake-utils";
    nixpkgs.url = "github:nixos/nixpkgs?rev=a4832e490d62bdb1e60cab751a9ae38b4431934e";
  };

  outputs = { self, nixpkgs, flake-utils }:
    flake-utils.lib.eachDefaultSystem (system:
      let
        pkgs = import nixpkgs { inherit system; };
      in rec
      {
        devShells.default = pkgs.mkShell {
          packages = [
            pkgs.python311
            (pkgs.pre-commit.overrideAttrs (previousAttrs: {
              makeWrapperArgs = ''--set PYTHONPATH $PYTHONPATH'';
            }))
          ];
        };
      });
}

$nix develop and pre-commit install && pre-commit run --all --verbose

This gets us to the mismatch behaviour if we create .pre-commit-config.yaml the way you helpfully define above. Note: there is a overridePythonAttrs to interact with the buildPythonPackage call you may need to use in some other cases.

Updating the python version is ~easy since pre-commit takes python3Packages as an input. (python3Packages has the reference to python buildPythonPackage depends on)

#flake.nix
{
  inputs = {
    flake-utils.url = "github:numtide/flake-utils";
    nixpkgs.url = "github:nixos/nixpkgs?rev=a4832e490d62bdb1e60cab751a9ae38b4431934e";
  };

  outputs = { self, nixpkgs, flake-utils }:
    flake-utils.lib.eachDefaultSystem (system:
      let
        pkgs = import nixpkgs { inherit system; };
      in rec
      {
        devShells.default = pkgs.mkShell {
          packages = [
            pkgs.python311
            # example getting the parens right, you should probably define pre-commit above in the let
            ((pkgs.pre-commit.override{ python3Packages = pkgs.python311Packages; }).overrideAttrs (previousAttrs: {
              makeWrapperArgs = ''--set PYTHONPATH $PYTHONPATH'';
            }))
          ];
        };
      });
}

Note you'll probably use python311Packages.<package-name> to keep any nix supplied python deps in line with your python3 version. (propogatedBuildInputs would probably bring in a valid python version for other python libs the way you had to work around for pre-commit)

anund · 2023-09-10T07:40:24Z

Hmm, looking a bit closer at the suggested fix the way the python path is built is a bit wrong. It captures the pythonPath generated during build that includes more than just a reference to python. A path like the following might be closer to what is needed. There is also --prefix or --suffix to consider see make-wrapper.sh. pre-commit lib would be $out/${pkgs.python311Packages.python.sitePackages}.

--prefix PYTHONPATH ${pkgs.python311Packages.python}/${pkgs.python311Packages.python.sitePackages}`

MatrixManAtYrService · 2023-10-26T05:31:40Z

I finally got a chance to meditate on @anund's comments. For the particular repo I'm most worried about (work stuff), this seems pretty close:

        devShells.default = pkgs.mkShell {
          packages = [
            pkgs.poetry
            pkgs.python311
            # pkgs.pre-commit
            (pkgs.pre-commit.overrideAttrs (previousAttrs: {
              makeWrapperArgs = ''
                --set PYTHONPATH $PYTHONPATH
                --suffix PYTHONPATH : ${pkgs.python311Packages.ruamel-yaml}/lib/python3.11/site-packages
                --suffix PYTHONPATH : .
              '';
            }))

The above overrides won't work for everyone. Your hooks may need different paths, but the --set line ought to get most people pretty far.

I'm now considering just closing this PR and writing a blog post about how to make pre-commit work inside of nix.

f1rstlady · 2024-02-21T22:15:06Z

@anund's approach almost solves the problem. The last hurdle for me was that the propagated build inputs needed to be injected to the environment:

pkgs.pre-commit.overrideAttrs(prevAttrs:
  with pkgs.python3Packages;
  let depsPath = makePythonPath prevAttrs.propagatedBuildInputs;
  in {
    makeWrapperArgs = (prevAttrs.makeWrapperArgs or []) ++ [
      "--prefix PYTHONPATH : ${depsPath}"
      "--prefix PYTHONPATH : $out/${python.sitePackages}"
    ];
  }
);

I'm not sure whether a difference between python3Packages.python and the python version used by the pre-commit derivation might occur and pose a problem, though.

l0b0 · 2024-03-19T07:05:39Z

Thanks for the fix! I've written a test for a different issue which might've been caused by the same underlying problem. Feel free to cherry-pick it.

MatrixManAtYrService · 2024-04-24T18:22:52Z

Thanks for the test! I'm unaccustomed to nixpkgs development so it'll be good for me to look that over.

It's been a little while since I've thought about this PR, but it's very nice to see it pop back up. I was feeling a little bit alone in my struggles against pre-commit.

imincik · 2024-04-30T13:27:58Z

Result of nixpkgs-review pr 251418 run on x86_64-linux 1

4 packages failed to build:

python311Packages.asteroid-filterbanks
python311Packages.asteroid-filterbanks.dist
python311Packages.pyannote-audio
python311Packages.pyannote-audio.dist

2 packages built:

pre-commit
pre-commit.dist

MatrixManAtYrService · 2024-05-07T15:52:00Z

I didn't know about that tool, that's pretty cool. I'll investigate how those packages are using pre-commit so I can avoid breaking them.

fidgetingbits · 2024-06-19T03:27:02Z

Just ran into this while working on a flake. I used the override from #251418 (comment) for now and it works, but think it would be nice when this gets in rather than having to manually update every flake that uses pre-commit.

I've never used nixpkgs-review before, but just gave it a run on x86_64-linux as well and didn't get any of the build errors mentioned above (maybe I did something wrong):

nix-shell -p nixpkgs-review.out --run 'nixpkgs-review pr 251418'
this path will be fetched (0.06 MiB download, 0.32 MiB unpacked):
  /nix/store/rx6laqr63fhb15sysy8z4magbijb8g0i-nixpkgs-review-2.10.4
copying path '/nix/store/rx6laqr63fhb15sysy8z4magbijb8g0i-nixpkgs-review-2.10.4' from 'https://cache.nixos.org'...
$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/251418/head:refs/nixpkgs-review/1
remote: Enumerating objects: 183036, done.
remote: Counting objects: 100% (29281/29281), done.
remote: Compressing objects: 100% (1251/1251), done.
remote: Total 183036 (delta 28476), reused 28580 (delta 27952), pack-reused 153755
Receiving objects: 100% (183036/183036), 109.76 MiB | 6.52 MiB/s, done.
Resolving deltas: 100% (108115/108115), completed with 7618 local objects.
From ssh://github.com/NixOS/nixpkgs
 * [new branch]                master                -> refs/nixpkgs-review/0
 * [new ref]                   refs/pull/251418/head -> refs/nixpkgs-review/1
$ git worktree add /home/aa/.cache/nixpkgs-review/pr-251418/nixpkgs 971ea0a28ad94b23d221c8377120340a303fc736
Preparing worktree (detached HEAD 971ea0a28ad9)
Updating files: 100% (41726/41726), done.
HEAD is now at 971ea0a28ad9 Merge pull request #320833 from matteo-pacini/easytag-resurrection
$ git merge --no-commit --no-ff ed76e4ddcfde2f8b2891455e7e61b71cfc6da502
Auto-merging pkgs/tools/misc/pre-commit/default.nix
Automatic merge went well; stopped before committing as requested
$ nix build --file /nix/store/rx6laqr63fhb15sysy8z4magbijb8g0i-nixpkgs-review-2.10.4/lib/python3.11/site-packages/nixpkgs_review/nix/review-shell.nix --nix-path nixpkgs=/home/aa/.cache/nixpkgs-review/pr-251418/nixpkgs nixpkg
s-overlays=/run/user/1000/tmpbsohbw02 --extra-experimental-features nix-command no-url-literals --no-link --keep-going --no-allow-import-from-derivation --option build-use-sandbox relaxed --argstr system x86_64-linux --argst
r nixpkgs-path /home/aa/.cache/nixpkgs-review/pr-251418/nixpkgs --argstr nixpkgs-config-path /run/user/1000/tmpufvqw8fw.nix --argstr attrs-path /home/aa/.cache/nixpkgs-review/pr-251418/attrs.nix

Link to currently reviewing PR:
https://github.com/NixOS/nixpkgs/pull/251418

6 packages built:
pre-commit pre-commit.dist python311Packages.asteroid-filterbanks python311Packages.asteroid-filterbanks.dist python311Packages.pyannote-audio python311Packages.pyannote-audio.dist

$ /nix/store/j7rp0y3ii1w3dlbflbxlv4g7hbaaz3bs-nix-2.18.2/bin/nix-shell --argstr system x86_64-linux --argstr nixpkgs-path /home/aa/.cache/nixpkgs-review/pr-251418/nixpkgs --argstr nixpkgs-config-path /run/user/1000/tmpufvqw8
fw.nix --argstr attrs-path /home/aa/.cache/nixpkgs-review/pr-251418/attrs.nix --nix-path nixpkgs=/home/aa/.cache/nixpkgs-review/pr-251418/nixpkgs nixpkgs-overlays=/run/user/1000/tmpbsohbw02 /nix/store/rx6laqr63fhb15sysy8z4ma
gbijb8g0i-nixpkgs-review-2.10.4/lib/python3.11/site-packages/nixpkgs_review/nix/review-shell.nix

Pre-commit's "system" language is a sort of escape hach from the encapsulation which pre-commit typically provides. It just runs commands without thinking too hard about setting up or tearing down environments. But it is used for two things: user-provided hooks which reference things in calling environment, and pre-commit-provided hooks which live in the pre-commit package (I'm calling these "meta" hooks). The challenge becomes: which PATH and PYTHONPATH variables do we expose to pre-commit? If it's the calling environment, the meta hooks break, and if it's the packaged environment, the external-facing system hooks break. In order to make both of these work at the same time, this commit separates 'system' into two languages: 'system', and 'meta'. System hooks get access to the calling environment, and meta hooks get access to the packaged environment.

MatrixManAtYrService · 2024-07-07T16:55:42Z

@imincik, I wonder if something has changed, because like @fidgetingbits, I also find that nixpkgs-review pr 251418 succeeds.

I spent a bit of time with asteroid-filterbanks and had a hard time getting the pre-commit hooks to succeed with or without this change (see ☝️ for details about the failure).

I also tested this change with pyannote-audio. It fails with the same error in all three cases:

pre-commit installed via pip
pre-commit installed via nix (without this change)
pre-commit installed via nix (with this change)

pyannote/audio/core/io.py:37:1: F401 'numpy as np' imported but unused

I'm not sure if these are cases where pre-commit is failing to be as deterministic as it aims to, or whether they're cases where maintainers have pushed commits without first running their pre-commit hooks, but as far as I can see, there's no indication that this change makes things any worse in those repos.

l0b0 · 2024-10-09T23:59:50Z

I'm getting errors from nixpkgs-review pr 251418 after rebasing onto master.

Closes NixOS#270805 by verifying that NixOS#251418 fixes the issue.

l0b0 · 2024-11-22T02:44:19Z

Rebased & fixed for pre-commit v4

ofborg bot requested a review from borisbabic August 25, 2023 18:30

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 25, 2023

MatrixManAtYrService marked this pull request as draft August 26, 2023 18:04

MatrixManAtYrService force-pushed the fix-pre-commit branch from f094a29 to 8b29a67 Compare August 26, 2023 22:20

MatrixManAtYrService marked this pull request as ready for review August 29, 2023 22:03

scalavision mentioned this pull request Oct 17, 2023

deeptools interferes with jupyter lab (problem with propagated inputs?) #261552

Open

l0b0 mentioned this pull request Mar 19, 2024

pre-commit: test meta hooks MatrixManAtYrService/nixpkgs#1

Merged

12 tasks

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024

l0b0 mentioned this pull request Apr 24, 2024

pre-commit: fix meta hooks #306444

Merged

13 tasks

l0b0 approved these changes Apr 24, 2024

View reviewed changes

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 24, 2024

ofborg bot removed 10.rebuild-darwin: 1 10.rebuild-linux: 1 labels Apr 24, 2024

fidgetingbits mentioned this pull request Jun 18, 2024

Add nix flake cursorless-dev/cursorless#1908

Merged

MatrixManAtYrService mentioned this pull request Jul 7, 2024

Updating black 20.8b1 -> 24.4.2 asteroid-team/asteroid-filterbanks#20

Open

MatrixManAtYrService force-pushed the fix-pre-commit branch from ed76e4d to 423b0dd Compare July 7, 2024 16:43

l0b0 mentioned this pull request Sep 17, 2024

pre-commit: fix import bug for built-in hooks #342436

Closed

13 tasks

l0b0 added a commit to l0b0/nixpkgs that referenced this pull request Oct 10, 2024

pre-commit: test meta hooks

fc7cf6f

Closes NixOS#270805 by verifying that NixOS#251418 fixes the issue.

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024

l0b0 added a commit to l0b0/nixpkgs that referenced this pull request Nov 22, 2024

pre-commit: test meta hooks

b989227

Closes NixOS#270805 by verifying that NixOS#251418 fixes the issue.

SuperSandro2000 closed this Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pre-commit: fixing import bug for built-in hooks #251418

pre-commit: fixing import bug for built-in hooks #251418

MatrixManAtYrService commented Aug 25, 2023 •

edited

Loading

MatrixManAtYrService commented Aug 25, 2023

MatrixManAtYrService commented Aug 26, 2023 •

edited

Loading

MatrixManAtYrService commented Aug 26, 2023 •

edited

Loading

MatrixManAtYrService commented Sep 4, 2023 •

edited

Loading

MatrixManAtYrService commented Sep 4, 2023

MatrixManAtYrService commented Sep 4, 2023

anund commented Sep 10, 2023

anund commented Sep 10, 2023 •

edited

Loading

MatrixManAtYrService commented Oct 26, 2023 •

edited

Loading

f1rstlady commented Feb 21, 2024 •

edited

Loading

l0b0 commented Mar 19, 2024 •

edited

Loading

MatrixManAtYrService commented Apr 24, 2024 •

edited

Loading

imincik commented Apr 30, 2024

MatrixManAtYrService commented May 7, 2024

fidgetingbits commented Jun 19, 2024

MatrixManAtYrService commented Jul 7, 2024 •

edited

Loading

l0b0 commented Oct 9, 2024

l0b0 commented Nov 22, 2024

pre-commit: fixing import bug for built-in hooks #251418

pre-commit: fixing import bug for built-in hooks #251418

Conversation

MatrixManAtYrService commented Aug 25, 2023 • edited Loading

Context

Description of changes

Things done

MatrixManAtYrService commented Aug 25, 2023

MatrixManAtYrService commented Aug 26, 2023 • edited Loading

MatrixManAtYrService commented Aug 26, 2023 • edited Loading

MatrixManAtYrService commented Sep 4, 2023 • edited Loading

MatrixManAtYrService commented Sep 4, 2023

MatrixManAtYrService commented Sep 4, 2023

anund commented Sep 10, 2023

anund commented Sep 10, 2023 • edited Loading

MatrixManAtYrService commented Oct 26, 2023 • edited Loading

f1rstlady commented Feb 21, 2024 • edited Loading

l0b0 commented Mar 19, 2024 • edited Loading

MatrixManAtYrService commented Apr 24, 2024 • edited Loading

imincik commented Apr 30, 2024

MatrixManAtYrService commented May 7, 2024

fidgetingbits commented Jun 19, 2024

MatrixManAtYrService commented Jul 7, 2024 • edited Loading

l0b0 commented Oct 9, 2024

l0b0 commented Nov 22, 2024

MatrixManAtYrService commented Aug 25, 2023 •

edited

Loading

MatrixManAtYrService commented Aug 26, 2023 •

edited

Loading

MatrixManAtYrService commented Aug 26, 2023 •

edited

Loading

MatrixManAtYrService commented Sep 4, 2023 •

edited

Loading

anund commented Sep 10, 2023 •

edited

Loading

MatrixManAtYrService commented Oct 26, 2023 •

edited

Loading

f1rstlady commented Feb 21, 2024 •

edited

Loading

l0b0 commented Mar 19, 2024 •

edited

Loading

MatrixManAtYrService commented Apr 24, 2024 •

edited

Loading

MatrixManAtYrService commented Jul 7, 2024 •

edited

Loading