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

[Styling] stylify using ruff #27144

Merged
merged 54 commits into from
Nov 16, 2023
Merged

[Styling] stylify using ruff #27144

merged 54 commits into from
Nov 16, 2023

Conversation

ArthurZucker
Copy link
Collaborator

@ArthurZucker ArthurZucker commented Oct 30, 2023

What does this PR do?

Removes our dependency on black in favor of rust:

  • faster make style I'll try to get numbers but that one is pretty obvious
  • not need to use doc-builder's black styling as the doc is styled in ruff
  • faster overall CI to check the repo quality

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 30, 2023

The documentation is not available anymore as the PR was closed or merged.

@@ -156,7 +155,7 @@
"rhoknp>=1.1.0,<1.3.1",
"rjieba",
"rouge-score!=0.0.7,!=0.0.8,!=0.1,!=0.1.1",
"ruff>=0.0.241,<=0.0.259",
"ruff>=0.1.5,<=0.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you elaborate a bit on this choice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Latest version for better performance and unified api I'd say. While <=2 to have enough rolling in case they change things

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I misread the numbers. Got it!

utils/check_copies.py Outdated Show resolved Hide resolved
Comment on lines 228 to 234
async def run_ruff(code):
command = ["ruff", "format", "-", "--config", "pyproject.toml", "--silent"]
process = await asyncio.create_subprocess_exec(
*command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE
)
stdout, _ = await process.communicate(input=code.encode())
return stdout.decode()
Copy link
Collaborator

Choose a reason for hiding this comment

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

would love to hear the motivation to make is async 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Followed what was done in huggingface hub, nor real reasons apart trying to make it faster. Though it won't be run in parallel. Did not benchmark both

Copy link
Collaborator

@ydshieh ydshieh Nov 16, 2023

Choose a reason for hiding this comment

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

Well, as here we don't really use the concurrency (which is the advantage of using coroutine), it looks somehow overwhelming to use async stuff here.

But my real concern is: here we spawn a new process whenever there is a # Copied from statement, which is an expensive op. Previously, blackify uses black.format_str which is inside the same python process, despite black itself is slow.

I checked the run (check_copies) with this PR and on main: 44 sec. on this PR and 32 sec. on main .

See run with this PR and run with main.

I am afraid, at least for check_copies.py, we lose the advantage of using ruff format due to the many new processes being created each time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well I can do not async, no worries. Black is just too complicated, and with ruff you don't need to use doc-builder on top of it.
Now differences can also come from modified files as well but let's remove black because if you take the make style time into account it's already worth it

Copy link
Collaborator

@ydshieh ydshieh 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 this awesome work! Having a few question, but sure LGTM!

@ArthurZucker ArthurZucker merged commit 651408a into main Nov 16, 2023
22 of 23 checks passed
@ArthurZucker ArthurZucker deleted the remove-black branch November 16, 2023 16:43
@amyeroberts amyeroberts mentioned this pull request Nov 17, 2023
4 tasks
wgifford pushed a commit to namctin/transformers that referenced this pull request Nov 17, 2023
* try to stylify using ruff

* might need to remove these changes?

* use ruf format andruff check

* use isinstance instead of type comparision

* use # fmt: skip

* use # fmt: skip

* nits

* soem styling changes

* update ci job

* nits isinstance

* more files update

* nits

* more nits

* small nits

* check and format

* revert wrong changes

* actually use formatter instead of checker

* nits

* well docbuilder is overwriting this commit

* revert notebook changes

* try to nuke docbuilder

* style

* fix feature exrtaction test

* remve `indent-width = 4`

* fixup

* more nits

* update the ruff version that we use

* style

* nuke docbuilder styling

* leve the print for detected changes

* nits

* Remove file I/O

Co-authored-by: charliermarsh
 <charlie.r.marsh@gmail.com>

* style

* nits

* revert notebook changes

* Add # fmt skip when possible

* Add # fmt skip when possible

* Fix

* More `  # fmt: skip` usage

* More `  # fmt: skip` usage

* More `  # fmt: skip` usage

* NIts

* more fixes

* fix tapas

* Another way to skip

* Recommended way

* Fix two more fiels

* Remove asynch
Remove asynch

---------

Co-authored-by: charliermarsh <charlie.r.marsh@gmail.com>
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
* try to stylify using ruff

* might need to remove these changes?

* use ruf format andruff check

* use isinstance instead of type comparision

* use # fmt: skip

* use # fmt: skip

* nits

* soem styling changes

* update ci job

* nits isinstance

* more files update

* nits

* more nits

* small nits

* check and format

* revert wrong changes

* actually use formatter instead of checker

* nits

* well docbuilder is overwriting this commit

* revert notebook changes

* try to nuke docbuilder

* style

* fix feature exrtaction test

* remve `indent-width = 4`

* fixup

* more nits

* update the ruff version that we use

* style

* nuke docbuilder styling

* leve the print for detected changes

* nits

* Remove file I/O

Co-authored-by: charliermarsh
 <charlie.r.marsh@gmail.com>

* style

* nits

* revert notebook changes

* Add # fmt skip when possible

* Add # fmt skip when possible

* Fix

* More `  # fmt: skip` usage

* More `  # fmt: skip` usage

* More `  # fmt: skip` usage

* NIts

* more fixes

* fix tapas

* Another way to skip

* Recommended way

* Fix two more fiels

* Remove asynch
Remove asynch

---------

Co-authored-by: charliermarsh <charlie.r.marsh@gmail.com>
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.

4 participants