dvc: hooks: pylint: pre-commit failure
.pre-commit-config.yaml states that the pylint hook is defined locally, yet .pre-commit-hooks.yaml fails to define it. As a result, attempting to commit will result in an error:
~/gits/dvc (master)$ git commit
black....................................................................Passed
seed isort known_third_party.............................................Passed
isort....................................................................Passed
flake8...................................................................Passed
pylint...................................................................Failed
- hook id: pylint
- exit code: 1
Executable `pylint` not found
beautysh.............................................(no files to check)Skipped
DVC pre-commit...........................................................Passed
- hook id: dvc-pre-commit
- duration: 0.5s
Data and pipelines are up to date.
Defining the hook’s language as system and asking devs to manually run pip install .[tests] goes against fundamental principles of pre-commit hooks. None of the other hooks require this.
In any case pylint itself defines .pre-commit-hooks.yaml so we should just use theirs.
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 19 (19 by maintainers)
Ok, so looks like “do nothing” is the choice for now. Closing.
I don’t like that approach/hack, it’s far easier to pre-activate virtualenv. And, it does not solve the issue, it still needs all of the tests dependencies to be installed.
The possible ways to solve this are:
pylintfrom pre-commit altogether. Add a guide to how-to runpylintin contributing docs.pylintis not installed.pre-pushhooks?I don’t have any strong opinions on either of the above. If we cannot commit to anything, I’d suggest we close the issue and defer it for later.
cc @casperdcl @efiop
@efiop, I’m still looking for answer for “Is it too difficult to pip install .[tests]?” from @casperdcl. Ideally, I’d also love to have it isolated, but in practice, the benefits outweigh the cons.
If it’s too hard to install our test dependencies, I could create a custom script that just throws a message to the user complaining that
pylintis not installed and does not breakpre-commitruns. But, there should be a very strong argument to do so.I think no-op would be frustrating for contributors -> silent pass just to fail on PR, each time we have new python env.
I feel like we are mixing up topics here.
The original issue is
pre-commitfailure, which, as described above, is not nice, but an intended way of using it. I see @casperdcl suggested using init hook in https://github.com/iterative/dvc/pull/4146#issuecomment-652552793 , which looks like the proper way to solve it. @skshetry could you take a look, please?The second issue that came up is pylint not being very fast (and, well, the whole pre-commit set of hooks). I can’t see any way of making it significantly less intrusive without defeating the purpose of having pre-commit hooks in the first place. I would say that the slowest in the pack is our dvc hook itself, so probarly would start there (it was the intention so we eat our own food).
@skshetry gotcha, makes sense! I would vote probably for providing some happy path in simple scenarios (warn but do not fail if env is not fully setup), assuming we always run it on CI/CD.