Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Update flake8 version in pre-commit #11684

Closed
wence- opened this issue Sep 12, 2022 · 6 comments · Fixed by #11736
Closed

[ENH] Update flake8 version in pre-commit #11684

wence- opened this issue Sep 12, 2022 · 6 comments · Fixed by #11736
Assignees
Labels
improvement Improvement / enhancement to an existing function proposal Change current process or code

Comments

@wence-
Copy link
Contributor

wence- commented Sep 12, 2022

At present, the pre-commit environment uses flake8 version 3.8.3. As part of moves to support Python 3.10, we will need to go to at least version 4 (to include PyCQA/flake8#1374). The current release version is 5.0.4.

I suspect that this will want to be synchronised across a bunch of the rapids repos.

@wence- wence- added proposal Change current process or code Needs Triage Need team to review and classify improvement Improvement / enhancement to an existing function labels Sep 12, 2022
@vyasr
Copy link
Contributor

vyasr commented Sep 12, 2022

The issue that we're going to run into here is that flake8 4.0.0 dropped support for Cython. If we want to update flake8, we need to make a plan for how we handle Cython files in the future. Our options are

  1. Do nothing, so Cython files no longer get linted
  2. Add the flake8-force plugin, which allows us to continue forcing flake8 to run on Cython files.
  3. Find a new Cython linter such as cython-lint

Option 2 is the easiest way to proceed here without losing linting while we wait for solutions like cython-lint to mature, but if people are willing we could be more proactive about evaluating Cython linter solutions.

As far as synchronization, our switch to using pre-commit means that cudf can update its linters largely independently of the rest of RAPIDS. While I do think that we should aim to synchronize best practices across RAPIDS, we remain quite a ways away from that. We still have multiple open issues around modernizing and improving cudf's linting, so my recommendation would be for us to get our house in order and then try to propagate all changes across RAPIDS afterwards rather than trying to keep everything in sync as we go. Having cudf trailblaze is more helpful than trying to seek full consensus a priori. Worst case we'll need to modify a few rule inclusions/exclusions afterwards I suspect.

@wence- wence- self-assigned this Sep 13, 2022
@vyasr vyasr mentioned this issue Sep 20, 2022
4 tasks
@bdice
Copy link
Contributor

bdice commented Sep 20, 2022

At present, options 2 and 3 are orthogonal, and we should adopt both in tandem. The current state of cython-lint performs checks that are distinct from those of flake8 (plus flake8-force). Unused cimports won’t be caught by flake8(-force).

That said, I think there may be bugs in cython-lint because I tried applying it to cudf and found it marked some imports as unused when they were in fact used. Might need to file some bugs for that if we use it.

@vyasr
Copy link
Contributor

vyasr commented Sep 20, 2022

We should definitely adopt flake8-force. I don't know if cython-lint is quite ready for production use yet, which is why I'm not sure about adopting it right now vs. waiting a bit. Someone is going to have to put in the time and effort to try it and find bugs if we're going to move forward there.

In any case, I think adopting flake8-force is a clear win for us and we should do that whenever someone has the time to make a PR moving us to a newer flake8.

@bdice
Copy link
Contributor

bdice commented Sep 21, 2022

I opened #11736 to update our pre-commit hook but haven't touched the developer conda environment version yet because that has to match the rest of RAPIDS. I suppose I need to make similar PRs to other packages and the integration environment before #11736 can be merged -- OR we could accept pre-commit as a single source of truth and remove flake8 from our cudf developer conda environment...

@vyasr
Copy link
Contributor

vyasr commented Sep 26, 2022

I would be fine with just accepting cudf as our source of truth. If anybody outside the cudf team is running flake8 on our source I have to imagine its via pre-commit hooks.

@MarcoGorelli
Copy link
Contributor

That said, I think there may be bugs in cython-lint because I tried applying it to cudf and found it marked some imports as unused when they were in fact used. Might need to file some bugs for that if we use it.

Hey @bdice , thanks for taking a look!

Sorry to jump in, just wanted to say - I've fixed some things since last week, and have tried this on the cudf repo and didn't notice any false negatives, could you upgrade and try again please?
If there is a false negative I missed, please do report it and I'll try to fix it - I'm pretty keen on getting this working well as there's a tonne of Cython files in pandas too
Thanks 🙌

rapids-bot bot pushed a commit that referenced this issue Oct 18, 2022
Resolves #11684, required for eventually supporting Python 3.10 (which requires flake8 >= 4.0.0). flake8 >= 4.0.0, however, does not support parsing Cython code, even with rule exclusions. This necessitates the flake8-force plugin, which was designed (by a cupy developer) for forcing flake8 to check Cython code with a limited set of rules.

Per this comment (#11684 (comment)), this PR removes duplicate pinnings between pre-commit configuration and the developer conda environment. Developers should use pre-commit for style checks consistent with the CI environment.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)
  - AJ Schmidt (https://github.com/ajschmidt8)
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #11736
@bdice bdice removed the Needs Triage Need team to review and classify label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function proposal Change current process or code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants