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

Support fail_fast on check level #1143

Closed
potiuk opened this issue Sep 22, 2019 · 28 comments · Fixed by #2097
Closed

Support fail_fast on check level #1143

potiuk opened this issue Sep 22, 2019 · 28 comments · Fixed by #2097

Comments

@potiuk
Copy link

@potiuk potiuk commented Sep 22, 2019

I would love to have fail_fast configuration option on check level not only at the level of the whole configuration file.

The use case I have in mind - i have one specific pre-commit check that performs docker builds of images that are used by later checks. This is fairly complex build and I have some optimisations in place to build it once and not re-run Docker build . again (because just running Docker build . later on will introduce some delays). I would like to mark that single first check only as "fail_fast". If it fails, none of the other checks should run. But if any other check fails, It should be perfectly OK to run all other tasks without failing fast.

@potiuk potiuk changed the title Support fail_fast on checck level Support fail_fast on check level Sep 22, 2019
@potiuk
Copy link
Author

@potiuk potiuk commented Sep 22, 2019

Also happy to add it if there is a will to accept the issue.

@asottile
Copy link
Member

@asottile asottile commented Sep 22, 2019

how is this different from fail_fast as it currently is?

@potiuk
Copy link
Author

@potiuk potiuk commented Sep 22, 2019

The current fail_fast can only be specified at the whole "yaml" level. What I want to do is to specify it differently for different checks so that failure of only some checks is "fails fast".

Example:

You can specify fail_fast today only at the top level (which means that both :

default_stages: [commit, push]
fail_fast: true . # <--- HERE IS WHAT WE CAN DO NOW
default_language_version:
  # force all unspecified python hooks to run python3
  python: python3
repos:
  - repo: local
    hooks:
      - id: build
        name: Check if image build is needed
        entry: ./scripts/ci/local_ci_build.sh
        language: system
        always_run: true
        pass_filenames: false
      - id: check-apache-license
        name: Check if licences are OK for Apache
        entry: "./scripts/ci/pre_commit_check_license.sh"
        language: system
        files: ^.*LICENSE.*$|^.*LICENCE.*$
        pass_filenames: false
        require_serial: true
  - repo: https://github.com/Lucas-C/pre-commit-hooks
    rev: v1.1.7
    hooks:
      - id: forbid-tabs
        exclude: ^airflow/_vendor/.*$|^docs/Makefile$
      - id: insert-license
        name: Add licence for all SQL files
        files: \.sql$
        exclude: ^\.github/.*$|^airflow/_vendor/.*$
        args:
          - --comment-style
          - "/*||*/"
          - --license-filepath
          - license-templates/LICENSE.txt
          - --fuzzy-match-generates-todo

In my scenario I would like the pre-commit fail fast only when build fails, but when check-apache-licence fail I would like pre-commit to continue.

default_stages: [commit, push]
default_language_version:
  # force all unspecified python hooks to run python3
  python: python3
repos:
  - repo: local
    hooks:
      - id: build
        fail_fast: true . # <--- HERE IS WHAT WE WOULD LIKE TO DO
        name: Check if image build is needed
        entry: ./scripts/ci/local_ci_build.sh
        language: system
        always_run: true
        pass_filenames: false
      - id: check-apache-license
        name: Check if licences are OK for Apache
        entry: "./scripts/ci/pre_commit_check_license.sh"
        language: system
        files: ^.*LICENSE.*$|^.*LICENCE.*$
        pass_filenames: false
        require_serial: true
  - repo: https://github.com/Lucas-C/pre-commit-hooks
    rev: v1.1.7
    hooks:
      - id: forbid-tabs
        exclude: ^airflow/_vendor/.*$|^docs/Makefile$
      - id: insert-license
        name: Add licence for all SQL files
        files: \.sql$
        exclude: ^\.github/.*$|^airflow/_vendor/.*$
        args:
          - --comment-style
          - "/*||*/"
          - --license-filepath
          - license-templates/LICENSE.txt
          - --fuzzy-match-generates-todo

@asottile
Copy link
Member

@asottile asottile commented Sep 22, 2019

why though, isn't the current behavior good enough?

@potiuk
Copy link
Author

@potiuk potiuk commented Sep 22, 2019

Nope. The first check - "build" is special because other checks depend on it. It's really a prerequisite. If it fails, it can have some undesired side effects. In my case, it might be that the other checks attempt to start builds the image in all 8 parallel processes run by pre-commit on my 8 processor machine (and fail) which might take a long time (and precious resources).

The other steps on the other hand, are not special and I am very happy if they do not fail fast. I have several checks there - for example we have pylint, mypy, flake8, and others. I want to see all of them failing and producing output rather than fail fast when first of them fail.

In the current solution it is "all-or-nothing". I cannot specify different behaviour for some of the tasks only.

Or maybe I am missing something and my case can be implemented currently ?

@asottile
Copy link
Member

@asottile asottile commented Sep 22, 2019

presumably the other steps are under your control as well and they could short circuit if the image is missing?

I don't think the proposal is unreasonable, I just want to make sure it's necessary before building something that needs to be supported indefinitely :)

@potiuk
Copy link
Author

@potiuk potiuk commented Sep 22, 2019

I understand. Short circuiting is what I try to do there -> I already protected in most cases against this, but it's really "workaround". The only way I found for short-circuitting it is to store results of the first step in specified folder and check that folder for failure. Variables are not passed between pre-commit checks, nor you cannot check status of previous checks. But this is clumsy and cumbersome way (and prone to errors).

There are multiple problems with it - for example what happens if I still want to run just one pre-commit using previously built images and the "build" step just failed and left this "marker file" in ? The logic to manage it becomes quickly a problem. Simple "fail_fast" when build step fails is really simple comparing to that.

@asottile
Copy link
Member

@asottile asottile commented Sep 22, 2019

does docker inspect whatever || exit 1 not work?

@potiuk
Copy link
Author

@potiuk potiuk commented Sep 22, 2019

Nope. It is more complex. We mount our sources during build in order to avoid full rebuild (and the time it takes). So we only trigger rebuild of docker when our dependencies change. In our case (in order to save precious 10s of seconds it takes to run docker build . with all the sources we compare MD5 of the files that should trigger rebuild (for example setup.py) and only trigger rebuild when needed.

This check takes few ms but then we also implemented interactive check - if the build needs to be done, we ask the developer whether to run the build - answer 'yes' - (this might take up to few minutes) or whether to use already built image - answer 'no' - (this in many cases is enough) or whether to quit the build entirely ('quit'). The last option makes the check to fail. And when it fails I would like it to "fail_fast" (i.e. quit). So that we do not run any more checks.

@asottile
Copy link
Member

@asottile asottile commented Sep 23, 2019

won't the image be absent in that case? I don't understand

@potiuk
Copy link
Author

@potiuk potiuk commented Sep 23, 2019

This is all for handling the case of continuously evolving project with big codebase and multiple dependencies (https://github.com/apache/airflow). And developer pulling in other's changes and rebasing the work on top of the other changes,

Imaging there is already an image build previously (say few days ago).

The case I am going to handle is to check if we need to run Docker build . at all.

Running Docker build . every time is out of the question really - because it takes too long to run it if just sources changed for very good reasons - one of the reason is that we have huge codebase, several technologies (especially npm) and we are using multi-staging dockerfile which takes long to build.

So we only want to attempt to make the build when one of the important files change (setup.py, Dockerfile, npm-packages.json etc.). If none of those change, we know that it is OK to do docker run -v airflow:airflow run_static_check.sh and we can use the previously built docker image to run our pre-commit checjs.

And we want the user to only check it once per git commit rather than for every pre-commit check.

Imagine this developer pulling and rebasing on top of a change where someone added dependency to setup.py. This dependency requires to rebuild the Docker image. But it's not needed by the developer because the developer works on different part of the codebase and pre-commit checks for those changes will run just fine on the previously build image.

Then if we find that one of those files changed, the user might decide if they want to wait few minutes more and build the new image, or try to still use the old image hoping that the pre-commits on old image will run fine (which in most cases is perfectly OK and is super fast).

I hope it explains the case.

@asottile
Copy link
Member

@asottile asottile commented Sep 23, 2019

Presumably you already have something in place to know whether the image is up to date or not right? either through building a deterministic tag based on your inputs (and checking existence) or through other means

it's very unclear to me why you can't use the same heuristic inside your dependent hooks to fail fast

I completely understand that branches change dependencies, etc. but I really think there's a way to solve this very-very-very-very special use case without adding complexity to the framework

@potiuk
Copy link
Author

@potiuk potiuk commented Oct 6, 2019

Yeah. I actually have something in place and it works fine . But when I choose "quit" I would love to quit the whole pre-commit check immediately - and currently I cannot do it :(.

Another - maybe simpler - use case might be as follows.

  • you have a quick check first to see if python compiles (pycompile). It runs under a second on a huge code base you have.
  • then you have a bunch of longer tests that are checking pylint, mypy, flake etc
  • running pre-commit --all-files might take (say) 2 minutes as your pylint/mypy checks take a long time to run.
  • however the first check (pycompile) fails under a second and you would like to quit quickly - you know your code does not even compile so you do not want to waste 2 more minutes to run mypy/pylint. You'd rather want to fix the pycompile problem first
  • but if . pycompile works fine, you want all of them (pylint, mypy, flake8) to run and show the errors - because each of them can show a different set of errors.

Currently you can either fail fast all of them, or none. But I would like to fail fast only when pycompile failes, not when mypy, pylint, flake8 fails

@asottile
Copy link
Member

@asottile asottile commented Oct 6, 2019

I mean if you want to submit a PR go for it, I already changed the tags to [feature]

I still don't think it has much of an advantage over just using fail_fast: true at the top level and I think the two examples are a bit contrived (CI is really the only one that is/should be running with --all-files)

@potiuk
Copy link
Author

@potiuk potiuk commented Oct 6, 2019

I will go for PR indeed.

There are some valid cases for --all-files to run locally. For example when you do a heavy refactor with multiple files involved, you might want to run --all-files. We do it quite often in Apache Airflow . Airflow is now heading towards 2.0 release - it includes a number of refactors that are being implemented by different people in parallel and backwards-incompatible changes that might have some undesirable side effects.

And quite often when you work locally you want to do rebase on top of someone else's changes and use --all-files locally to run all the file checks locally - because somebody's changes might actually modify code in ways that running on your change only will not cut it. We've had some cases of this recently in our project. This is something I do quite often recently.

@asottile
Copy link
Member

@asottile asottile commented Oct 21, 2019

@potiuk any updates on this :D

@potiuk
Copy link
Author

@potiuk potiuk commented Oct 22, 2019

Not yet. I am currently at the ApacheCon Berlin conference (and talking here also praising pre-commit framework at my talk!). I had very little time to take a look at that - likely I will take a look when I am back from the conference (next week).

@asottile
Copy link
Member

@asottile asottile commented Oct 25, 2019

no problem! good luck :D

@asottile
Copy link
Member

@asottile asottile commented Apr 22, 2020

@potiuk did you have a chance to circle back on this?

@potiuk
Copy link
Author

@potiuk potiuk commented Apr 24, 2020

Not yet :(. We kind of can live without this feature for now and I am now deep into migrating whole Apache Airflow to Gihub Actions. I am finishing this up as we speak (we rolled it out yesterday and we have some fixes on the way) so I might come back to pre-commit improvements. I have a few things that I miss that we might want to use:

  1. Fail-fast on check-level (as in this issue)

  2. list (or similar) command to show list of available pre-commit ids with explanation or with --short form only showing ids (I need it because currently we have to manually synchronise list of pre-commits with the documentation and we auto-generate such documentation usually).

  3. autocomplete support for bash/zsh

I haven't looked it up yet in existing issues (but I guess somebody already raised such issues) but those are the three most-needed things for us.

WDYT ?

@asottile
Copy link
Member

@asottile asottile commented Apr 24, 2020

there is an open issue for 3. already and I've thought a lot about 2. but haven't decided on ergonomics or opened an issue yet

@potiuk
Copy link
Author

@potiuk potiuk commented Apr 24, 2020

I promise I come back to it soon. We love pre-commit and as you might see we are a heavy-user https://github.com/apache/airflow/blob/master/.pre-commit-config.yaml: 51 checks and counting

@archoversight
Copy link

@archoversight archoversight commented Oct 8, 2020

This feature would also be great so that if the check-yaml hook fails, I can make sure that yamlfmt hook doesn't fire and destroy the file by writing garbage.

Sometimes hooks that come after other hooks are not safe unless they pass some sort of safety mechanism first, but at the same time for anything else, I'd like it to run all fixers automatically (think end-of-file/mixed-line-ending/trailing-white-space) since doing a git add + git commit cycle for each individual fixer to run due to fail_fast is a tad annoying.

@asottile
Copy link
Member

@asottile asottile commented Oct 8, 2020

tbf I'd consider it a bug that a formatter tries to touch a file that has syntax errors in it -- probably should report that to yamlfmt

@colens3
Copy link
Contributor

@colens3 colens3 commented Oct 19, 2021

Hi @asottile
Wanted to check if this one is available and still relevant? In case it is I would like to try to help :)

@asottile
Copy link
Member

@asottile asottile commented Oct 19, 2021

yep feel free to tackle this

@colens3
Copy link
Contributor

@colens3 colens3 commented Oct 19, 2021

Cool, just one quick check - is the idea to have the fail_fast within the configuration on "repo" level or for each individual hook on "hooks" level?

@asottile
Copy link
Member

@asottile asottile commented Oct 19, 2021

it would be per-hook

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.

4 participants