dask-ml icon indicating copy to clipboard operation
dask-ml copied to clipboard

Pin static-checking versions and add pre-commit Action

Open hristog opened this issue 3 years ago • 7 comments

Updates, associated with this PR:

  • The original intention was to only pin code checking-related versions to their dask and distributed equivalents (as per https://github.com/dask/dask/pull/7256 and https://github.com/dask/distributed/pull/4533).
  • However, isort and black can apparently be stuck in a never-ending loop of proposing updates to the same file back and forth.
  • Given the latter, a quick search has revealed that there have been recent efforts to facilitate the easier interoperability of black and isort, namely a --profile=black option, which is available in recent isort versions (but not in 4.3.21, which is why this PR proposes bumping the isort version up to 5.8.0, released on March 20th 2021). I appreciate that some previous efforts to enable isort and black to play nicely together are present in the isort section of setup.cfg, but it somehow feels more logical to aim to employ recent versions in addition to that, unless there exist some associated constraints (CI environment-related or otherwise).

hristog avatar Mar 28 '21 14:03 hristog

@jsignell, I've just discovered your https://github.com/dask/dask/pull/7370 (I can see that the isort version there is 5.7.0, and also that the PR is in a draft state). Could you, please, have a look at this PR when you get a chance? I'll be happy to update this PR to isort==5.7.0 (instead of the proposed 5.8.0), if that's your preference.

hristog avatar Mar 28 '21 20:03 hristog

I imagine I just picked the latest version of isort when I opened that PR on dask, so there is no special magic to that version :)

jsignell avatar Mar 29 '21 14:03 jsignell

My main suggestion is that in dask/dask, there is a github action that runs pre-commit.

Thanks for checking this out, @jsignell!

Apologies - I'm not very familiar with GitHub Actions, and wanted to double-check if implementing your suggestion could be done as part of this PR, or if it requires maintainer access to the repository?

hristog avatar Mar 29 '21 20:03 hristog

GitHub Actions... could be done as part of this PR, or if it requires maintainer access to the repository?

It can be done as part of this PR or as a follow-on if you prefer. Shouldn't require any special access. This is how the dask pre-commit action is set up https://github.com/dask/dask/blob/main/.github/workflows/pre-commit.yml

jsignell avatar Mar 30 '21 17:03 jsignell

It can be done as part of this PR or as a follow-on if you prefer. Shouldn't require any special access. This is how the dask pre-commit action is set up https://github.com/dask/dask/blob/main/.github/workflows/pre-commit.yml

Understood - thank you very much, @jsignell! I'll update this PR a bit later today, to incorporate your suggestion.

hristog avatar Mar 30 '21 18:03 hristog

@jameslamb has brought up a very good point in https://github.com/dask/dask-ml/pull/822#discussion_r610737352 about considering pinning the static checker versions in https://github.com/dask/dask-ml/blob/f5e5bb4d4d21782b0e93f0f6541e6f2501b0c06c/setup.py#L28 as well (if my understanding has been correct). Also, I've just noticed that mypy is missing from setup.py.

@jrbourbeau, when you get a chance, could you confirm if you've got any objections, with respect to pinning optional-dependency versions and adding mypy to setup.py?

hristog avatar Apr 09 '21 16:04 hristog

pinning the static checker versions

One potential disadvantage of that (i.e., pinning minimum versions for isort, flake8 and friends) might be that the local dev environment would have to be modified which, in turn, might cause inconvenience when the same underlying tools would need to be utilized in the context of other projects.

On the other hand, that's already what happens with respect to the non-testing package requirements: https://github.com/dask/dask-ml/blob/f5e5bb4d4d21782b0e93f0f6541e6f2501b0c06c/setup.py#L13

hristog avatar Apr 09 '21 18:04 hristog