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

Switched linters, black -> ruff #334

Merged

Conversation

ishandeva
Copy link
Contributor

Removed black configs, added black configs under ruff, ran ruff check and applied safe fixes

What does this PR do?

Fixes Issue #186

Before submitting

  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case. -> Issue Switch to ruff native formatter #186
  • Did you run all tests locally and make sure they pass.
  • Did you write any new necessary tests? -> Not Applicable

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

Removed black configs, added black configs under ruff, ran ruff check and applied safe fixes
Copy link
Collaborator

@dacorvo dacorvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull-request: a few changes are required before I can merge it.

Makefile Outdated
@@ -3,11 +3,9 @@
check_dirs := optimum test bench examples

check:
black --check ${check_dirs}
ruff check ${check_dirs}
Copy link
Collaborator

@dacorvo dacorvo Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, the ruff linter only takes care of very specific formatting rules (like sorting imports).
This is why you also need to call ruff format.
See an example in another project where I only use ruff:

check:
	ruff check --show-fixes ${check_dirs}
	ruff format ${check_dirs} --diff

style:
	ruff check ${check_dirs} --fix
	ruff format ${check_dirs}

Also, as you can see, when calling check I added some verbosity options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dacorvo for the inputs.
Will call format as well.

Also, I think the reference repo that you mentioned (for check options) might be private? As I'm getting a code 404 on hitting the link.

Copy link
Collaborator

@dacorvo dacorvo Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think the reference repo that you mentioned (for check options) might be private?

Ah yes, sorry. I edited my message to copy the relevant portion of the Makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Also, as you can see, when calling check I added some verbosity options.

Yeah, makes sense to list out the changes before applying them.

@@ -12,6 +12,6 @@ else
pip install --upgrade --pre torch torchvision torchaudio --index-url https://download.pytorch.org/whl/nightly/cu118
fi
# Build tools
pip install black ruff pytest build
pip install ruff pytest build
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a legacy file I should have removed a long time ago since it is obsoleted by optional targets in pyproject.toml.

Anyway, you also need to edit the python-quality CI workflows under .github.

Copy link
Contributor Author

@ishandeva ishandeva Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, you also need to edit the python-quality CI workflows under .github.

Do we want similar verbosity flags on this one as well?
I see that currently it simply calls check :
- run: ruff check bench examples optimum test

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that would be useful thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alrighty then, done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason it might still be relevant is because ppl submitting pull-request might not be familiar with the concept of linting/formatting, but would understand immediately what this is about when looking at the failing CI logs for their pull-request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... yes, agreed.

This will enable enumeration of changes before ruff applies them.
Added listing flag on ruff check
@ishandeva ishandeva requested a review from dacorvo October 7, 2024 13:06
Copy link
Collaborator

@dacorvo dacorvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there:

  • you need to check also the format in the CI workflow.
  • it seems ruff formatting is a bit different, and if I run make check locally using your branch, I have 21 files that would need to be reformatted (most of them are about removing one line break in class declarations).

.github/workflows/python-quality.yml Show resolved Hide resolved
Added format check to the CI workflow

Co-authored-by: David Corvoysier <[email protected]>
@ishandeva
Copy link
Contributor Author

ishandeva commented Oct 7, 2024

Right. Even I saw those, but I assumed that was a valid correction based on ruff/new formatting standard

Sure, but you need to apply the changes into a commit, because now the CI will fail. In retrospect I wasn't clear at all in my review: please forgive me.

Ran ruff formatter on the project (`check_dirs` in makefile) to fix format issues.
@ishandeva
Copy link
Contributor Author

Right. Even I saw those, but I assumed that was a valid correction based on ruff/new formatting standard

Sure, but you need to apply the changes into a commit, because now the CI will fail. In retrospect I wasn't clear at all in my review: please forgive me.

No worries! Formatting changes have been applied. 👍

@ishandeva ishandeva requested a review from dacorvo October 8, 2024 11:43
@dacorvo dacorvo merged commit b0cce24 into huggingface:main Oct 8, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants