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

CI skip not working correctly? #53

Closed
TylerYep opened this issue Apr 10, 2021 · 9 comments
Closed

CI skip not working correctly? #53

TylerYep opened this issue Apr 10, 2021 · 9 comments

Comments

@TylerYep
Copy link

@TylerYep TylerYep commented Apr 10, 2021

Even though I specified the skip flag, pre-commit-ci still tries to install the environment for mypy?
It works fine if I put the command in local, but not if I specify the repo url.

ci:
  skip: [mypy]
repos:
  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v0.812
    hooks:
      - id: mypy
        additional_dependencies: [torch]

(the config is truncated to only include mypy)

build attempt 1...
    => timeout after 60s
    [INFO] Installing environment for https://github.com/pre-commit/mirrors-mypy.
    [INFO] Once installed this environment will be reused.
    [INFO] This may take a few minutes...
    Interrupted (^C): KeyboardInterrupt: 
    Check the log at /pc/pre-commit.log
build attempt 2...
    => timeout after 60s
    [INFO] Installing environment for https://github.com/pre-commit/mirrors-mypy.
    [INFO] Once installed this environment will be reused.
    [INFO] This may take a few minutes...
    Interrupted (^C): KeyboardInterrupt: 
    Check the log at /pc/pre-commit.log
build attempt 3...
    => timeout after 60s
    [INFO] Installing environment for https://github.com/pre-commit/mirrors-mypy.
    [INFO] Once installed this environment will be reused.
    [INFO] This may take a few minutes...
    Interrupted (^C): KeyboardInterrupt: 
    Check the log at /pc/pre-commit.log
@asottile
Copy link
Member

@asottile asottile commented Apr 10, 2021

yeah, installation must still occur and succeed to eventually run pre-commit run

@asottile
Copy link
Member

@asottile asottile commented Apr 10, 2021

it's possible that pre-commit itself could be improved for this case:

$ git diff
diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py
index 05c3268..0fef50d 100644
--- a/pre_commit/commands/run.py
+++ b/pre_commit/commands/run.py
@@ -271,11 +271,11 @@ def _get_diff() -> bytes:
 def _run_hooks(
         config: Dict[str, Any],
         hooks: Sequence[Hook],
+        skips: Set[str],
         args: argparse.Namespace,
         environ: MutableMapping[str, str],
 ) -> int:
     """Actually run the hooks."""
-    skips = _get_skips(environ)
     cols = _compute_cols(hooks)
     classifier = Classifier.from_config(
         _all_filenames(args), config['files'], config['exclude'],
@@ -403,9 +403,11 @@ def run(
             )
             return 1
 
-        install_hook_envs(hooks, store)
+        skips = _get_skips(environ)
+        to_install = [hook for hook in hooks if hook.id not in skips]
+        install_hook_envs(to_install, store)
 
-        return _run_hooks(config, hooks, args, environ)
+        return _run_hooks(config, hooks, skips, args, environ)
 
     # https://github.com/python/mypy/issues/7726
     raise AssertionError('unreachable')

this would skip installation if a hookid is inside SKIP (though it still needs to "clone" to ensure the metadata is correct)

@asottile
Copy link
Member

@asottile asottile commented Apr 10, 2021

I'll see what I think of this tomorrow: pre-commit/pre-commit#1875

@lachmanfrantisek
Copy link

@lachmanfrantisek lachmanfrantisek commented May 21, 2021

Is this already deployed on pre-commit.ci?

Looks like I am still hitting #25 for ansible-lint that I've marked as skipped (https://github.com/packit/deployment/pull/211/files).

Thanks for working on this!

@asottile
Copy link
Member

@asottile asottile commented May 21, 2021

@lachmanfrantisek it is not, that is why this issue is still open

@lachmanfrantisek
Copy link

@lachmanfrantisek lachmanfrantisek commented May 21, 2021

Thanks for your response. I should have made that clear in my question, but do you plan to deploy that? I don't know, how (often) you update the pre-commit itself in the CI part. Just want to know if it makes sense to wait a bit or do something with that on our side. Nevertheless, thanks!

@asottile
Copy link
Member

@asottile asottile commented May 21, 2021

bumping the pre-commit version is trivial, teaching the distributed cache mechanism about this requires a code change

@asottile
Copy link
Member

@asottile asottile commented May 25, 2021

this is implemented now -- you should be able to use ci: skip: [...] to avoid problematic builds such as ansible or torch

thanks again for the issue !

@lachmanfrantisek
Copy link

@lachmanfrantisek lachmanfrantisek commented May 27, 2021

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants