Add precommit

[SauravMaheshkar]

Configure pre-commit

Description

Fix #2743

• Specific releases have been mentioned instead of stable / master / main. For clarification please refer to this.
• The pytest hook is "local" instead of being a hook. For clarification please refer to this

Type of change

Please check the option that is related to your PR.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • In this case, we recommend to discuss your modification on GitHub issues before creating the PR
• Documentations (modification for documents)

Checklist

• My code follows the style guidelines of this project
  • Run yapf -i <modified file> and check no errors (yapf version must be 0.22.0)
  • Run mypy -p deepchem and check no errors
  • Run flake8 <modified file> --count and check no errors
  • Run python -m doctest <modified file> and check no errors
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New unit tests pass locally with my changes
• I have checked my code and corrected any misspellings

[rbharath]

@SauravMaheshkar Thanks for the contribution!

@arunppsg Could you take a quick look at this PR? It might dovetail nicely with your planned work on the yml infrastructure for the CI

[arunppsg]

Hey @SauravMaheshkar, thank you very much for the contribution. A couple of general thoughts and suggestions:

• The flake8, mypy and yapf pre-commit hooks are really good ones. They add a lot of usability. Maybe, is it possible to run the tests only in changed files instead of all the files? It will save more time.
• Adding pre-commit dependency in env.common.yml alone is sufficient. The other .yml files are generally used for installing backend frameworks associated with DeepChem.

A general doubt:

I feel that the requirement of pre-commit is another dependency addition to DeepChem. Git allows customizing of hooks via the .git/hooks directory. If we can set-up a pre-commit hook over there and make changes to DeepChem docs on how developers can set-up the hook for deepchem or add a script which developers can copy to .git/hooks directory, will it not be more simpler?

[SauravMaheshkar]

NOTE: I've removed pre-commit from the other environment files.

---

1. @arunppsg I'm not able to understand the "time" issue. AFAIK pre-commit will run only on the staged files after the initial setup. For reference, this is from the pre-commit docs. So for the initial setup (i.e. the first commit with pre-commit) it'll take time as it'll run against all files but after that it should run only on the staged files.

pre-commit run: this is what pre-commit runs by default when committing. This will run all hooks against currently staged files.

2. I agree with you on how adding pre-commit would add another dependency to deepchem and git-hooks might be a better alternative. I guess we must compare our options between the ease of using pre-commit, it's open source community and git hooks which although barebone and highly customizable might be restrictive for some of the newer devs and steepen the learning curve.

[arunppsg]

On 1, sorry I did not know it earlier that it only runs on staged files. On 2, I defer to @rbharath.

[rbharath]

Chatted offline with @arunppsg. I think that since pre-commit is not on the critical path, it would be fine to add on as an extra dependency. The main thing we will need to do in this PR before merge is add in more documentation.

Could you add a description of pre-commit installation/usage as a section to https://github.com/deepchem/deepchem/blob/master/docs/source/development_guide/coding.rst? New developers should be able to read the docs section and set up and run pre-commit locally afterwards

[SauravMaheshkar]

I've added some description in a2424d9. LMK if it looks good.

[rbharath]

Have a couple of minor comments on the docs

[rbharath]

We need to do pip install pre-commit first right?

[rbharath]

Could you also try these instructions out locally on a fresh environment/deepchem install to make sure they work as expected?

[SauravMaheshkar]

pre-commit is already present in env.common.yml so there shouldn't be any need to run pip install pre-commit right ?

[rbharath]

Hmm, I'm not sure actually. Could you try installing from source in a fresh environment following directions in the doc and see if it is present for you? The installation flow is a little complex so I'm not sure if the user will actually have pre-commit installed correctly on a fresh install

[SauravMaheshkar]

Yes, it's present by default. Steps I followed:

• Clone my branch (add-precommit) to my local system and create a new conda env named test using env.common.yml.
• activate the test environment and run pre-commit install. PFA the output

pre-commit installed at .git/hooks/pre-commit

• run pre-commit run --all-files this runs pre-commit on the codebase. Output is as follows :

[INFO] Installing environment for https://github.com/pre-commit/mirrors-yapf.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Check for merge conflicts................................................Passed

[SauravMaheshkar]

PS: There seems to be a lot of yapf and flake8 inconsistencies in the codebase.

[rbharath]

Ah good catch! If you're interested, a follow-up PR cleaning up yapf/flake8 inconsistencies in the codebase would be very useful

[rbharath]

I think if the CI looks good, we are clear on my end to merge in.

@arunppsg Could you do a review pass as well?

[arunppsg]

LGTM!

[arunppsg]

A personnel note: I have limited bandwidth and I always try to review pull request at the earliest. Nudging for review sometimes irritates.