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)

Most upvoted comments

Ok, so looks like “do nothing” is the choice for now. Closing.

I see @casperdcl suggested using init hook in #4146 (comment)

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:

  1. Remove pylint from pre-commit altogether. Add a guide to how-to run pylint in contributing docs.
  2. Create a wrapper script that’d be a no-op if pylint is not installed.
  3. Do nothing.
  4. Make it a pre-push hooks?

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 pylint is not installed and does not break pre-commit runs. 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-commit failure, 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.