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

Lint overhaul (pylint to ruff) #3004

Closed
wants to merge 19 commits into from
Closed

Lint overhaul (pylint to ruff) #3004

wants to merge 19 commits into from

Conversation

akx
Copy link
Contributor

@akx akx commented Sep 26, 2023

This PR looks a bit big, but it's pretty straightforward if you look at the commits in isolation.

Basically, it replaces pylint and isort with the ruff linter, and re-enables linting in CI (now that it's not screaming an awful lot – I hope @erogol has found some peace of mind (ref. 8c1d8df)). The pylint configuration was converted with ttps://github.com/akx/pylint-to-ruff.

  • pre-commit is also updated, but currently running pre-commit run --all-files yields a ton of changes.
  • Some ruff rules are not enabled at this point at all because they're kind of noisy, but they could be enabled in future PRs one by one.
  • This PR supersedes Update pre-commit tools #2992.
  • I found [Bug] compute_attention_masks.py doesn't work #3002 while working on this, so hey, there's clearly a point to this.

@erogol
Copy link
Member

erogol commented Sep 27, 2023

Hey @akx. Can you explain why ruff over pylint a little?

@akx
Copy link
Contributor Author

akx commented Sep 27, 2023

@erogol

@akx
Copy link
Contributor Author

akx commented Oct 23, 2023

@erogol Rebased on current dev.

@akx akx force-pushed the lint-overhaul branch 4 times, most recently from 2dd3a40 to 2afda54 Compare October 31, 2023 14:31
@akx akx force-pushed the lint-overhaul branch 2 times, most recently from ab26129 to 1f8a62b Compare October 31, 2023 15:39
@akx akx force-pushed the lint-overhaul branch 4 times, most recently from da837ba to 1e6abb3 Compare November 6, 2023 19:41
@akx
Copy link
Contributor Author

akx commented Nov 6, 2023

@erogol Rebased once again... 🙂

@akx
Copy link
Contributor Author

akx commented Nov 7, 2023

... and again, following #3149...

@akx
Copy link
Contributor Author

akx commented Nov 8, 2023

... and again. This also includes #3170 because it'd otherwise cause a lint failure.

@akx
Copy link
Contributor Author

akx commented Nov 9, 2023

@erogol Could this get merged now? You can always disable some lints if they end up being too noisy...

@akx
Copy link
Contributor Author

akx commented Dec 4, 2023

Rebased for the new month. The finetuning example that was merged in #3296 was pretty messy...

Copy link

stale bot commented Jan 12, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. You might also look our discussion channels.

@stale stale bot added the wontfix This will not be worked on but feel free to help. label Jan 12, 2024
@stale stale bot closed this Jan 28, 2024
@eginhard
Copy link
Contributor

eginhard commented Apr 8, 2024

I've merged this and the check_skip PR into our fork, thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on but feel free to help.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants