Use pre-commit for Git hooks #3406

andrewhare · 2020-02-25T19:45:13Z

NOTE: This is blocked by a pending 2.2.0 release of pre-commit to support post-checkout hooks.

This introduces a new flag to the dvc install command:

$ dvc install --use-pre-commit-tool

Running this command will do several things:

Verify that pre-commit is installed on the system
Write the existing DVC git hooks bash files to .dvc/tmp/hooks instead of to .git/hooks
Build a pre-commit configuration that calls the hook files in .dvc/tmp/hooks.
Check the current directory for an existing .pre-commit-config.yaml, if one is found, merge the configuration into it, otherwise dump the configuration to a new .pre-commit-config.yaml.

efiop · 2020-02-26T05:09:36Z

@andrewhare Looks really good! Thank you so much for the PR!

For the record: We've discussed during the meeting with @andrewwhare, that it might be worth creating those hook files somewhere (e.g. in .dvc/ or .dvc/tmp/) and triggering those from pre-commit config, instead of writing shell wrapper directly into them.

andrewhare · 2020-02-26T05:48:31Z

@efiop Thanks for the feedback!

I like the idea of writing the hook files to disk (possibly reusing/refactoring _install_hook to write to the correct directory) and calling them there.

efiop · 2020-03-09T11:33:41Z

dvc/scm/git/__init__.py

+        if not which("pre-commit"):
+            raise DvcException("pre-commit is not installed")
+


I wonder if we really need to run pre-commit for the user, maybe config is enough + a hint now run 'pre-commit install' to install git hooks? Just a question.

efiop · 2020-03-09T11:37:05Z

dvc/scm/git/__init__.py

+        conf_yaml = os.path.join(self.root_dir, ".pre-commit-config.yaml")
+        if not os.path.isfile(conf_yaml):
+            check_call(["pre-commit", "install"])


This one is needed in case user has .pre-commit-hooks.yaml, but no config? Or am I missing something?

pared

I would consider adding test asserting that hooks work.
I tried to check their behavior and upon running this script:

#!/bin/bash

rm -rf repo storage git_repo
mkdir repo git_repo

main=$(pwd)
pushd git_repo
git init --quiet --bare
popd

pushd repo

git init --quiet
dvc init -q
git remote add origin $main/git_repo
dvc remote add -d str $main/storage

dvc install --use-pre-commit-tool

echo data >> data

dvc add data -q

git add -A
git commit -am "something"

I get the following error:

An error has occurred: InvalidConfigError: 
==> File .pre-commit-config.yaml
==> At Config()
==> At key: repos
==> At Repository(repo='local')
==> At key: hooks
==> At Hook(id='dvc-post-checkout')
==> At key: stages
==> At index 0
=====> Expected one of commit, commit-msg, manual, merge-commit, prepare-
commit-msg, push but got: 'checkout'
Check the log at /home/prd/.cache/pre-commit/pre-commit.log

pared · 2020-03-10T10:03:40Z

tests/unit/scm/test_pre_commit_tool.py

+class TestPreCommitTool(TestCase):
+    def setUp(self):
+        self.conf = pre_commit_tool_conf("a", "b", "c")
+
+    def test_merge_pre_commit_tool_confs_empty(self):
+        existing_conf = None
+        merged_conf = merge_pre_commit_tool_confs(existing_conf, self.conf)
+        self.assertEqual(self.conf, merged_conf)
+
+    def test_merge_pre_commit_tool_confs_invalid_yaml(self):
+        existing_conf = "some invalid yaml"
+        merged_conf = merge_pre_commit_tool_confs(existing_conf, self.conf)
+        self.assertEqual(self.conf, merged_conf)
+
+    def test_merge_pre_commit_tool_confs_no_repos(self):
+        existing_conf = {"foo": [1, 2, 3]}
+        merged_conf = merge_pre_commit_tool_confs(existing_conf, self.conf)
+        self.assertEqual(self.conf, merged_conf)
+
+    def test_merge_pre_commit_tool_confs(self):
+        existing_conf = {"repos": [{}]}
+        merged_conf = merge_pre_commit_tool_confs(existing_conf, self.conf)
+        # Merging the new conf in should append the new repo to the end of
+        # the existing repos array on the existing conf.
+        self.assertEqual(self.conf["repos"][0], merged_conf["repos"][1])


Is there some particular reason to use unittest? We re moving towards using pytest, I think it would be good to make it test also pytest-based.

efiop · 2020-03-16T11:09:38Z

@pared Did you install the latest pre-commit version? As mentioned in the header, it requires 2.2.0

EDIT: can reproduce, looking into it.

efiop · 2020-03-16T11:40:49Z

For the record: taking over this PR.

efiop · 2020-03-16T12:23:35Z

Currently you need to dvc install --use-pre-commit-tool on each clone, as pre-commit won't have .dvc/tmp/hook/* scripts to run without it. Looks like it would be better to run hooks directly through dvc. Looking into it.

efiop · 2020-03-16T17:20:41Z

.pre-commit-config.yaml

+  repo: https://github.com/andrewhare/dvc
+  rev: WIP-pre-commit-tool


Will be fixed after merge.

efiop · 2020-03-16T18:33:57Z

Ok, merging to adjust on master. So far works good for me, but it is tied to github repo, so need to try out on our master.

Docs will come separately after.

shcheklein · 2020-03-16T19:13:46Z

@efiop do we need adjust any docs because of this change?

efiop · 2020-03-16T19:52:15Z

@shcheklein Yes, hence "Docs will come separately after."

Related to iterative/dvc#3406

skshetry · 2020-03-17T08:15:44Z

dvc/command/install.py

@@ -27,4 +27,11 @@ def add_parser(subparsers, parent_parser):
        help=INSTALL_HELP,
        formatter_class=argparse.RawDescriptionHelpFormatter,
    )
+    install_parser.add_argument(
+        "--use-pre-commit-tool",


It'd be good to have --use-pre-commit or just --pre-commit. Tool is kind of redundant here.

@skshetry Im worried about it being confused with pre-commit hooks.

andrewhare requested review from efiop and shcheklein February 25, 2020 19:45

weekly-digest bot mentioned this pull request Mar 1, 2020

Weekly Digest (23 February, 2020 - 1 March, 2020) #3425

Closed

andrewhare force-pushed the WIP-pre-commit-tool branch from 23dae14 to 7c0792f Compare March 5, 2020 22:50

restyled-io bot mentioned this pull request Mar 5, 2020

Restyle WIP: Use pre-commit for Git hooks #3448

Closed

andrewhare changed the title ~~WIP: Use pre-commit for Git hooks~~ Use pre-commit for Git hooks Mar 5, 2020

skshetry self-requested a review March 6, 2020 06:36

efiop requested review from pared, Suor and gurobokum March 9, 2020 11:29

efiop reviewed Mar 9, 2020

View reviewed changes

pared suggested changes Mar 10, 2020

View reviewed changes

efiop force-pushed the WIP-pre-commit-tool branch from 89fd2a5 to 24967a8 Compare March 16, 2020 11:14

efiop mentioned this pull request Mar 16, 2020

Allow git hooks to be installed individually #3479

Closed

efiop changed the title ~~Use pre-commit for Git hooks~~ [WIP] Use pre-commit for Git hooks Mar 16, 2020

efiop force-pushed the WIP-pre-commit-tool branch from b226826 to cf2fa6d Compare March 16, 2020 12:09

Andrew Hare and others added 6 commits March 16, 2020 16:34

Initial design of using pre-commit for Git hooks

06ebcae

Refactor to reuse existing code and provide merge capabilities

0327d63

Linter fixes

0834213

More cleanup

62651d1

scm: make use_precommit_tool optional

880f51a

dvc: install our own hooks

13d4ac5

install: fix bugs

7aec2fe

efiop force-pushed the WIP-pre-commit-tool branch 2 times, most recently from 9891552 to df29cd2 Compare March 16, 2020 15:04

install: use python hooks

0a79495

efiop force-pushed the WIP-pre-commit-tool branch 12 times, most recently from ac7780d to 1490809 Compare March 16, 2020 16:59

add .pre-commit-hooks.yaml

63fcc66

efiop force-pushed the WIP-pre-commit-tool branch from 1490809 to 63fcc66 Compare March 16, 2020 17:12

efiop reviewed Mar 16, 2020

View reviewed changes

efiop changed the title ~~[WIP] Use pre-commit for Git hooks~~ Use pre-commit for Git hooks Mar 16, 2020

efiop merged commit c4164b3 into iterative:master Mar 16, 2020

efiop added a commit to iterative/dvc.org that referenced this pull request Mar 16, 2020

install: add pre-commit config

41d9bab

Related to iterative/dvc#3406

efiop mentioned this pull request Mar 16, 2020

install: add pre-commit config iterative/dvc.org#1059

Merged

efiop added a commit to iterative/dvc.org that referenced this pull request Mar 16, 2020

install: add pre-commit config

618fe8b

Related to iterative/dvc#3406

shcheklein pushed a commit to iterative/dvc.org that referenced this pull request Mar 16, 2020

install: add pre-commit config (#1059)

52ab37d

Related to iterative/dvc#3406

skshetry reviewed Mar 17, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pre-commit for Git hooks #3406

Use pre-commit for Git hooks #3406

andrewhare commented Feb 25, 2020 •

edited

Loading

efiop commented Feb 26, 2020 •

edited

Loading

andrewhare commented Feb 26, 2020

efiop Mar 9, 2020 •

edited

Loading

efiop Mar 9, 2020

pared left a comment

pared Mar 10, 2020

efiop commented Mar 16, 2020 •

edited

Loading

efiop commented Mar 16, 2020

efiop commented Mar 16, 2020

efiop Mar 16, 2020

efiop commented Mar 16, 2020 •

edited

Loading

shcheklein commented Mar 16, 2020

efiop commented Mar 16, 2020

skshetry Mar 17, 2020

efiop Mar 17, 2020

		if not which("pre-commit"):
		raise DvcException("pre-commit is not installed")

		repo: https://github.com/andrewhare/dvc
		rev: WIP-pre-commit-tool

Use pre-commit for Git hooks #3406

Use pre-commit for Git hooks #3406

Conversation

andrewhare commented Feb 25, 2020 • edited Loading

efiop commented Feb 26, 2020 • edited Loading

andrewhare commented Feb 26, 2020

efiop Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

efiop Mar 9, 2020

Choose a reason for hiding this comment

pared left a comment

Choose a reason for hiding this comment

pared Mar 10, 2020

Choose a reason for hiding this comment

efiop commented Mar 16, 2020 • edited Loading

efiop commented Mar 16, 2020

efiop commented Mar 16, 2020

efiop Mar 16, 2020

Choose a reason for hiding this comment

efiop commented Mar 16, 2020 • edited Loading

shcheklein commented Mar 16, 2020

efiop commented Mar 16, 2020

skshetry Mar 17, 2020

Choose a reason for hiding this comment

efiop Mar 17, 2020

Choose a reason for hiding this comment

andrewhare commented Feb 25, 2020 •

edited

Loading

efiop commented Feb 26, 2020 •

edited

Loading

efiop Mar 9, 2020 •

edited

Loading

efiop commented Mar 16, 2020 •

edited

Loading

efiop commented Mar 16, 2020 •

edited

Loading