Create style_docstr.yml

[tkoyama010]

Create style_docstr.yml to run pre-commit

[codecov]

Codecov Report

Merging #221 (00f899f) into main (1ad53e9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #221   +/-   ##
=======================================
  Coverage   97.22%   97.22%           
=======================================
  Files           8        8           
  Lines         648      648           
  Branches       81       81           
=======================================
  Hits          630      630           
  Partials       18       18           

[tkoyama010]

@adam-grant-hendry Can you guess why mypy and pylint is not installed in https://github.com/pyvista/pyvistaqt/runs/7636558143?check_suite_focus=true ?

[adam-grant-hendry]

@tkoyama010 Yes, I believe I see the problem. Our .pre-commit-config.yaml is using language: system for these packages, so

1. They must be pip installed in the CI and passed their configuration files to work
2. mypy should be put with the other repo: local hooks

Also, we should specify require_serial: true for these hooks.

Detailed Explanation

Typically, a repo url is passed to repo and pre-commit creates an isolated virtual environment in which it clones necessary files, installs them, and sets things up nicely (using the repo's .pre-commit-hooks.yaml). This is nice so that

• you don't have to require developers to install the dependency (pip install x for Python)
• the testing is independent of your system, which reduces gotcha's like "it works on my machine, but not yours" (forces you to be deliberate about passing configuration arguments to the hook)

However, since the above runs in an isolated environment, it does not have access to your locally installed dependencies and locally installed configuration files. Some hooks have special environment requirements that don't allow them to be run in isolation. Two that are known are pylint and pytest, and the mypy issue referenced in our .pre-commit-config.yaml file adds mypy to this list. These must be run on the local system, which is why you pass the argument 'local' to repo without a rev to specify that it will be run on the current machine (this includes the CI machine when running CI). pre-commit will not run these in an isolated virtual environment, so you must set up the virtual environment and install dependencies manually for them to run (typically from a maintained requirements.txt or pyproject.toml file).

Note that for local hooks, all the following properties must be specified for the hook to run:

• id
• name
• language
• entry
• files/types

In addition, by default, pre-commit runs all hooks sequentially in the order they are listed in the .pre-commit-config.yaml. However, since version v1.13.0, pre-commit runs each individual hook operation on all the files passed to it in parallel. This
beneficial for the majority of cases, since it means each hook will execute faster, but there are some scenarios where this may present a problem:

• if a hook reads from other files not passed to it (e.g. a configuration file), or
• if a hook writes to other files not passed to it (e.g. a log file)

In the above cases, this may cause a fork bomb. See

• Parallel execution of individual hooks? pre-commit/pre-commit#510
• Fork bomb - Wikipedia

In these cases, it is recommended to

1. to go to the repo
2. look at their .pre-commit-hooks.yaml and/or .pre-commit-config.yaml file(s)
3. if they have specified require_serial: true, add it to your local hook

If they have require_serial: true, it means they have most likely tested this issue and that it should be added. As a conservative approach, for all local hooks, if you don't know if it should be added (or if you encounter problems without it), it's always a good idea to specify require_serial: true.

The Fix

To fix this, I would change the .pre-commit-config.yaml to:

  - repo: local
    hooks:
      - id: pylint
        name: (pylint) Check for code smells
        entry: pylint
        language: system
        types: [python]
        require_serial: true
        args: [
          "-rn", # Only display messages
          "-sn", # Don't display the score
        ]

      - id: pytest
        name: (pytest, coverage) Run tests and coverage
        stages: [push]
        entry: coverage run -m pytest && coverage html
        types: [python]
        language: system
        pass_filenames: false
        always_run: true
        require_serial: true

      - id: mypy
        name: (mypy) Check static type hinting
        entry: mypy
        language: system
        pass_filenames: false
        types: [python]
        require_serial: true

NOTE: By default, these packages indicate they automatically search for and find configuration information in expected files: i.e. .pylintrc, mypy.ini, pytest.ini, etc. If for some reason these are not picked up, we can add args: [] to each and pass the appropriate command line option that specifies where to look for the configuration file for the package.

We also need to add a section to our CI to pip install pylint, pytest, and mypy

Addendum

I keep this at the top of my .pre-commit-config.yaml because it helps me remember important concepts before I make changes (which is only every once in a while). It would be nice to add the same to pyvista, either in the .pre-commit-config.yaml or somewhere in the docs:

Precommit Instructions

# By default, `pre-commit` only runs against changed files (i.e. files that have been
# modified and added to your `git` staging area with `git add`). Unstaged files will not
# be checked and `pre-commit` will not run if you have unstaged changes to your
# `.pre-commit-config.yaml`.
#
# So, when you first add a new hook to this repo, it is a good idea to save a local backup
# of your files on your branch
#
# git stash push -m 'WIP on your-branch'
#
# and then run the hook against all files in the repo
#
# pre-commit run your-new-hook --all-files
#
# This will show you how it works by default so you can add arguments to the hook or
# configure it to work exactly how you want. To restore your backup, run
#
# git reset --hard HEAD
# git stash apply
#
# Once you're sure you back to where you started, you can clear the stash
#
# git stash clear
#
# Individual hooks can be run by specifying their `id` to `pre-commit run`. All hooks in
# your `.pre-commit-config.yaml` can be run with `pre-commit run --all`.
#
# We often want to run against the latest version of the linter/formatters available. We
# also want to make sure all our hook configuration options are respected so we know it
# runs the way we expect across all systems (virtual environments, CI, different OS's,
# etc.)
#
# In general, you pass the hook repo url to `repo` and `pre-commit` creates an isolated
# (virtual) environment where it clones (downloads) necessary files and installs them.
# This is nice so that

# - you don't have to require developers to install the dependency (`pip install x` for
#   Python)
# - the testing is independent of your system, which reduces gotcha's like "it works on
#   my machine, but not yours" (forces you to be deliberate about passing configuration
#   arguments to the hook)
#
# Because `pre-commit` runs in an isolated environment, it won't have access to your
# locally-installed dependencies. Hence, if the hook requires additional dependencies
# (like `toml`), you must specify them as additional dependencies to the hook:
#
# repo: https://yourhook.com
# rev: x.y.z
# hooks:
# - id: do-the-thing
#   additiona_dependencies: [
#     "toml"
#   ]
#
# However, some hooks have special environment requirements that don't allow them to be
# run in isolation (e.g. `pylint` and `pytest`). For these, you pass the argument
# `'local'` to `repo` without a `rev` to specify that it will be run on the current
# machine (local or CI). `pre-commit` will not run these in an isolated virtual
# environment, so you must setup the virtual environment and install dependencies
# manually (typically from a maintained `requirements.txt` or `pyproject.toml` file).
#
# In addition, sometimes there are conflicts between the files `pre-commit` passes to
# your hook (the changed files in your staging area) and the hook that can't be well-
# managed with include or exclude configuration options. Other times, you may simply
# want to always to run a hook against the same set of files before commit (not just
# changed files in your staging area). For this, you can use
#
# `pass_filenames: false`
#
# Note that, in general, you want to avoid this because the hook will take longer to
# run (since you're always running against a fixed set of files in this case rather than
# just changed files). Alternatively, it is better to specify files to include and
# exclude:
#
# `files: ...`
# `exclude: ...`
#
# To ensure hooks stay up to date:
#
# 1. For non-local hooks (using the repo url), run
#
#    pre-commit autoupdate --repo hook-repo-url
#
#    To update all hooks in your `.pre-commit-config.yaml` automatically, run
#
#    pre-commit autoupdate
#
#    (NOTE: You usually ALWAYS want `rev`` hard-coded to a specific release tag and let
#    `autoupdate` handle updating the hook version. This way, you know exactly which
#     version of the hook is running. Also, `pre-commit` typically warns about "floating"
#     or imprecise revs) To update to the latest rev, use.
#
# 2. For local hooks, dependencies must be updated manually (e.g. `pip install --upgrade`)
#
# Note that for local hooks, all the following properties must be specified for the hook
# to run:
#
#   - id
#   - name
#   - language
#   - entry
#   - files/types
#
# Finally, By default, `pre-commit` runs all hooks sequentially in the order they are
# listed in the `.pre-commit-config.yaml`. However, since version `v1.13.0`, `pre-commit`
# runs each _individual hook operation_ on all the files passed to it in parallel. The is
# beneficial for the majority of cases since it means each hook will execute faster, but
# there are some scenarios where this may present a problem:
#
#   - if a hook reads from other files not passed to it (e.g. a configuration file), or
#   - if a hook writes to other files not passed to it (e.g. a log file)
#
# In the above cases, this may create a fork bomb. See
#
#   - https://github.com/pre-commit/pre-commit/issues/510
#
# In the event your hook needs to access the same file across multiple processes, you can
# specify the hook be run on files sequentially by passing
#
#   - `require_serial: true`
#
# In general, when using a third-party package locally, it is recommended to go to the
# package repo, look at their `.pre-commit-config.yaml` and/or `.pre-commit-hooks.yaml`
# and see if they have specified `require_serial: true`. If yes, it should be added as
# a parameter. However, if not specified, it is good to add it anyway as a precaution
# (or in the event you encounter problems when using the package).
#

[tkoyama010]

@adam-grant-hendry Thank you very much. I don't quite understand, but there is no CI installation of mypy and pylint in the pyvista/pyvista repository. Is there any reason why we cannot do it that way here?

[adam-grant-hendry]

Is there any reason why we cannot do it that way here?

From the pylint docs:

Since pylint needs to import modules and dependencies to work correctly, the hook only works with a local installation of pylint (in your environment).

Similarly, for pytest, since you have to add repository-specific dependencies (specified in our requirements_test.txt), it's recommended as a local install

The mypy issue also requires we run this locally.

I don't quite understand, but there is no CI installation of mypy and pylint in the pyvista/pyvista repository.

If you look at pyvista's .pre-commit-config.yaml, you'll notice pylint and pytest are not in there. mypy is there, but I made it local in pyvistaqt due to this reported issue.

If you look at our testing-and-deployment.yml, you'll notice the github action sets up a virtual environment and we install our requirements_test.txt into it.

[adam-grant-hendry]

@tkoyama010 I could be wrong, but I think we may need to do some work so that the pyvistaqt github actions workflows better match what we are doing in the pyvista github actions workflows.

We could probably leave pytest out of our .pre-commit-config.yaml since it is part of our github actions. However, I recommend keeping pylint and mypy in there. pylint finds a lot of extra hard-to-find code smells that flake8 won't catch.

[adam-grant-hendry]

The mypy issue also requires we run this locally.

@tkoyama010 Actually, I take that back:

The mypy[misc]: duplicate module error can happen either because:

1. You are using namespace packages (no __init__.py) and have two .py files with the same name in different folders, in which mypy thinks you have duplicate files because it doesn't understand namespace packages well (this is rare, but happened here), or

2. There is an overlap between the options specified in a mypy config file for what files to parse (typically a glob pattern) vs. what is used in the .pre-commit-config.yaml file include/exclude options (typically a regex).

By default, pre-commit only runs against changed files (i.e. files that have been modified and added to your git staging area with git add). Unstaged files will not be checked. Specifying pass_filenames: false tells pre-commit to not pass staged files as input to mypy. As a result, mypy will run against all files in your repo (except those excluded in a glob in your .ini/toml config settings) every time pre-commit is run.

Hence everytime pre-commit is run, if pass_filenames: false is set, it will run against all files (not just changed ones) in your project (as if you simply entered mypy . --config-file=pyproject.toml), which the pre-commit author has stated will make your mypy hook run slower.

We don't use namespace packages or monorepos, so I think instead using the files: and exclude: options are better suited for this.

Sometimes specifying pass_filenames: false can be useful if you want to always run a hook against the same set of files (not just changed files in your staging area), for example if you have to use pytest as a hook, but it's probably better if we just run pytest off the commandline in our CI.

pylint might be the only one that would have to be local.

[tkoyama010]

Close this as we now use https://pre-commit.ci/ .