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

SKIP skips invocation by alias but doesn't skip "Installing environment" #2478

Closed
kmARC opened this issue Aug 10, 2022 · 4 comments · Fixed by #2480
Closed

SKIP skips invocation by alias but doesn't skip "Installing environment" #2478

kmARC opened this issue Aug 10, 2022 · 4 comments · Fixed by #2480

Comments

@kmARC
Copy link
Contributor

kmARC commented Aug 10, 2022

search tried in the issue tracker

SKIP alias

describe your issue

When I run

SKIP=pylint-alias2 git commit

the hook aliased as pylint-alias2 is properly skipped, however, its environment is still initialized. I would expect that it also skips the lengthy initialization of the environment.

Indeed, running a single hook (run.py#153) checks against hook.id and hook.alias, however install_hook_envs is invoked with only filtering against hook.id (run.py#423

Is this inconsistency deliberate? I couldn't find a use case for it. This quick and dirty patch works for us:

-        to_install = [hook for hook in hooks if hook.id not in skips]
+        to_install = [hook for hook in hooks if hook.id not in skips and hook.alias not in skips]

In our use case Initialization is not needed for everyone (monorepo where some non-python devs also do non-python work) and also very expensive unfortunately (6minutes currently to install already cached pip packages)

(An even better solution would be if initialization wouldn't happen at all if the hook run would we skipped in general (because of SKIP variable or because there is no files changed, however, that would be a but bigger patch I suppose))

pre-commit --version

pre-commit

.pre-commit-config.yaml

repos:
  - repo: https://github.com/PyCQA/pylint
    rev: v2.14.5
    hooks:
      - name: pylint-alias1
        alias: pylint-alias1
        id: pylint
        additional_dependencies:
          - arrow
      - name: pylint-alias2
        alias: pylint-alias2
        id: pylint
        additional_dependencies:
          - hdfs

~/.cache/pre-commit/pre-commit.log (if present)

No response

@kmARC
Copy link
Contributor Author

kmARC commented Aug 10, 2022

Additional info: we are very well aware of the caching setups (and quite much like it!) and in our CI builds we do cache everything whenever an additional_dependencies changes.

The most painful use case is really when a developer wants to commit files which has nothing to do with one python project, but since the additional_dependencies in another hook, which would not be invoked, changes, then the developer still has to wait for the long initialization step. Respecting SKIP would already be a big improvement

@asottile
Copy link
Member

initialization must happen to resolve the alias -- but it shouldn't be installed -- please don't skip the prompts in the issue template. show the command you ran and the output

@kmARC
Copy link
Contributor Author

kmARC commented Aug 10, 2022

Sorry, my vocabulary was confused. Initialization should happen yes. Installing the environment should not.

Here is the output (with cleared ~/.cache before):

± SKIP=pylint-alias2 pre-commit run --all-files
[INFO] Initializing environment for https://github.com/PyCQA/pylint.
[INFO] Initializing environment for https://github.com/PyCQA/pylint:arrow.
[INFO] Initializing environment for https://github.com/PyCQA/pylint:hdfs.
[INFO] Installing environment for https://github.com/PyCQA/pylint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/PyCQA/pylint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
pylint-alias1........................................(no files to check)Skipped
pylint-alias2...........................................................Skipped

@asottile
Copy link
Member

yep seems like an oversight then, send a tested patch please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants