dask-ml
dask-ml copied to clipboard
Pin static-checking versions and add pre-commit Action
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
andblack
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
andisort
, namely a--profile=black
option, which is available in recentisort
versions (but not in4.3.21
, which is why this PR proposes bumping theisort
version up to5.8.0
, released on March 20th 2021). I appreciate that some previous efforts to enableisort
andblack
to play nicely together are present in theisort
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).
@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.
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 :)
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?
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
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.
@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
?
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