Documentation for pre-commit hook should clearly call out dependency

[vfazio]

Our pipelines started failing after the bump to v2.4:

Run import-linter........................................................Failed
- hook id: import-linter
- exit code: 1

Executable `lint-imports` not found

I might be lacking on coffee right now, but the directions: https://import-linter.readthedocs.io/en/stable/usage.html#running-using-pre-commit

Do not work without also amending a project's dependencies (likely the dev dependencies) to include import-linter since it's not spinning up a venv to run the utility out of and expecting it to be in whatever project venv is available...

I think the example hook also implies that the venv is either already activated or that import-linter is installed globally in order for things to work, so running git commit without an activated environment will fail. Both situations, committing without an activated environment and not having packages installed globally, are common workflows

I was not successful using:

Or, if you prefer pre-commit to install Import Linter separately, you can do this (replacing with the version number of Import Linter you wish to use):

Is the above expected to work? If so, how?

For now, the best I can get away with is:

• Pin import-linter in my dev dependencies and relocking
• deviate from the documentation to explicitly activate the environment when running the hook
  • Since we use poetry, this looks like poetry run lint-imports, uv should have similar syntax

  - repo: local
    hooks:
      - id: lint_imports
        name: "Lint imports"
        entry: poetry run lint-imports
        language: system
        pass_filenames: false

I realize not all plugins can be kept isolated, we also have to run mypy via system hooks, so this may be a necessary change, but I think we can be a bit clearer in the documentation so users know how to fix their setup. A lot of pre-commit setup is copy pasta so I wouldn't be surprised if someone else ends up stumbling on this.

I appreciate the work you do on this package!

[seddonym]

Thanks for the issue.

Is the above expected to work? If so, how?

It should work if you're running pre-commit from within your virtual environment - it says that a couple of lines up but maybe it could be clearer. Perhaps because you're using poetry, it's not suited to your use case.

Personally I use the first approach, where Import Linter is installed into my virtual environment.

Happy to review a PR if you think you can improve on the wording!

[vfazio]

It should work if you're running pre-commit from within your virtual environment - it says that a couple of lines up but maybe it could be clearer. Perhaps because you're using poetry, it's not suited to your use case.

I'll have to look into this more.

This should be happening by default since the git hook is invoked like so:

INSTALL_PYTHON=/home/vfazio/development/xtf/.venv/bin/python
if [ -x "$INSTALL_PYTHON" ]; then
    exec "$INSTALL_PYTHON" -mpre_commit "${ARGS[@]}"

pre-commit is in the virtualenv (venv 1) along with other dev dependencies like mypy

The documentation mentions

- repo: https://github.com/seddonym/import-linter
  rev: <import linter version>
  hooks:
  - id: import-linter

However, with the above import-linter uses the upstream repo's .pre-commit-hooks.yaml so the lint-imports command is now defaulting to coming from "system" and is unreachable from venv 1 if import-linter is not also specified as a dev dependency in the project.

If the hook was working for people previously, I believe behavior can be restored by specifying language: python to override language: system a la:

  - repo: https://github.com/seddonym/import-linter
    rev: v2.4
    hooks:
      - id: import-linter
        language: python
        name: Run import-linter
        pass_filenames: false

The caveats that drove the change to the defaults still applies so pinning import-linter in the dev dependencies and using the default hook may be required in those cases

[seddonym]

Hmm, to be honest I'm not sure there is an ideal way to configure pre-commit. Import Linter is designed to work with with access to any analyzed packages on the python path, which is a bit different from pre-commit's intent, which is more for isolated environments for each tool. For me it's easiest just to do it as a custom command (first example in the docs) rather than bothering with the provided one. Maybe we can figure out a better approach but you should probably familiarise yourself with the conversation on https://github.com/seddonym/import-linter/pull/216 and the approach we came up recently in https://github.com/seddonym/import-linter/pull/268.

[vfazio]

I'll try to take some time to review those issues. I think I read over them before but didn't fully grok what was going on.

I haven't fully researched how this utility works (a peer of mine implemented it and they're not available at the moment) so I'm probably missing some nuance about why the installation path of the utility, be it in a pre-commit venv vs a global venv, affects how it's working.

I'm not sure if this requires having the package that's being analyzed "installed" or at the very least being able to import relative to a directory in order to analyze imports which is why flat layouts may not have an issue but src layouts do.

Anyway, for now I don't think this should be treated with any urgency, just as documentation for those that run into this problem.

Once I've had time to look into this and fully understand what's going on and why, I will close the issue or start up a discussion about approaches.

Thanks again for your work!

[Remi-Gau]

Seems that this may be an issue for those of us using pre-commit CI to run pre-commit:

Got an update from pre-commit CI to update our hooks including import-linter:

https://github.com/nilearn/nilearn/pull/5659

updating to v2.4 fails

• commit: https://github.com/nilearn/nilearn/pull/5659/commits/107e0987046892ef64d2203000bfa4bd46631f95
• precommit-ci failure: https://results.pre-commit.ci/run/github/1235740/1757364479.iCG12IdMRsqamv19nVPzYw

Adding picture in case the logs get purged:

trying other options recommended in the doc also fails

• commit: https://github.com/nilearn/nilearn/pull/5659/commits/7ac4d1095478c2a73bb9314c216185e575584e43
• precommit-ci failure: https://results.pre-commit.ci/run/github/1235740/1757398774.9nu5Fx4lQ2CjWLPoJQGCFw

current fix

stick to v2.3

[seddonym]

Hi Remi - apologies for the breaking change - it seems it's a bit awkward to get the installable precommit to work out of the box in all environments.

I checked out your project locally and Import Linter precommit seems to work in that environment, so presumably it's just a problem in CI.

The key is to make sure that (i) Import Linter is installed and (ii) your project is importable. Here are a couple of things you could try:

1. Add language: python to your config like this (v2.4 changed it to language: system, so that will just change it back to what it was before).

-   repo: https://github.com/seddonym/import-linter
    rev: v2.4
    hooks:
    -   id: import-linter
        args: [--verbose]
        language: python

or:

2. Find a way of installing Import Linter in CI, before running the pre-commit hooks (I looked at the precommit CI's documentation page and I couldn't immediately see a way to do this).

Let me know how you get on!

[Remi-Gau]

apologies for the breaking change

no worries! I'd be a big hypocrite if I said none of my changes ever broke one of my user's workflow. 😉

1. Add `language: python` to your config like this (v2.4 changed it to `language: system`, so that will just change it back to what it was before).

-   repo: https://github.com/seddonym/import-linter
    rev: v2.4
    hooks:
    -   id: import-linter
        args: [--verbose]
        language: python

that seems to work:

https://github.com/nilearn/nilearn/pull/5664

pre-commit CI is happy: https://results.pre-commit.ci/run/github/1235740/1757408715.lmq7Fd1XTKKEcvFdBCn8WQ

not sure if that edge case should be mentioned in the documentation, but at least it's now in the issue tracker!

thanks for the quick reply!!!

[seddonym]

Great, glad you got it working.