Feature: pre commit improvement by ZzEeKkAa · Pull Request #170 · NVIDIA/numba-cuda

ZzEeKkAa · 2025-03-21T14:56:18Z

Description

Replace flake8 with ruff code style to automatically format code. ruff has same set of features as flake8, so having both linters is redundant. ruff also adds auto formatting features like black. Config was ported from .flake8 to .pre-commit-config.yaml with these differences:
- remove ignores for W503 and W504 (line break after binary operator).
Add pre-commit hooks that removes trailing spaces, ensures line brakes in the end of the file, formats yaml, toml, json, prevents big files to be added to history etc
Run pre-commit in separate commit and add it to git blame ignore.

This PR must be merged without squash, to separate config changes (that we want in history) from formatter changes (that we do not want). If rebased, please update .git-blame-ignore-revs commit hash.

Old description

Add black code style to automatically format code.
Run pre-commit in separate commit and add it to git blame ignore.

This PR must be merged without squash!

ZzEeKkAa · 2025-03-21T14:59:35Z

I realized the pre-commit is already run, so I'll force push to this branch to remove github action that I added.

This PR must be merged without squash!

isVoid · 2025-03-21T17:36:49Z

@ZzEeKkAa what do you think of ruff v.s. black? I'm using ruff in Numbast and it's faster and usually run into fewer non-auto fixable errors than black IMO

ZzEeKkAa · 2025-03-21T20:34:12Z

I don't have anything against ruff) Just did not use it before

isVoid · 2025-03-24T17:44:36Z

I don't have anything against ruff) Just did not use it before

I suggest using ruff - Numbast will deliver static bindings formatted with ruff and I think it should run into fewer issue if Numba also uses a ruff formatter upon re-formatting to Numba's code standard.

gmarkall · 2025-03-25T10:13:53Z

This PR must be merged without squash!

Is this because of the addition of .git-blame-ignore-revs? I think even if I rebase and merge, commit hashes end up different due to something Github does. So I think to make this work correctly, I might need to "rebase and merge" then just add the .git-blame-ignore-revs in a follow-up PR afterwards.

I don't have anything against ruff) Just did not use it before

I suggest using ruff

I'm in favour of this PR (I just never had the experience of setting something up like this before or the time to figure it out) and I'm happy with either of black or ruff, whatever you (@isVoid and @ZzEeKkAa) agree on.

leofang · 2025-03-25T12:35:23Z

Let's use Ruff as it's used by all other CUDA Python projects.

ZzEeKkAa · 2025-03-25T23:26:57Z

This PR must be merged without squash!

Is this because of the addition of .git-blame-ignore-revs?

Yes

I think even if I rebase and merge, commit hashes end up different due to something Github does. So I think to make this work correctly, I might need to "rebase and merge" then just add the .git-blame-ignore-revs in a follow-up PR afterwards.

Idea is to keep history for the changes to pyproject.toml/.pre-commit-config.yaml and do not show commit where I run pre-commit run -a because it does not have logical sense.

So yeah, if commit will be different after rebase (not sure why it is needed for branch merge, we need to update commit hash there.

ZzEeKkAa · 2025-03-25T23:28:01Z

Just rebased the branch to work with ruff. Should be good to go)

ZzEeKkAa · 2025-03-26T12:51:23Z

Numbast will deliver static bindings formatted with ruff and I think it should run into fewer issue if Numba also uses a ruff formatter upon re-formatting to Numba's code standard.

I guess it is generally a bad idea to run any linters/formatters on the generated code. But I'm all in for ruff.

isVoid

I managed to give a rough pass to the code which mostly looks good to me. I went over the new ruff config v.s. the flake8 config and discovered a nice small discussion around W503 and W504 - apparently it was controversial and ruff didn't implement them:
astral-sh/ruff#4125

As for the f-string dicussion below - I realized they are unsafe fixes. They might be a nice code style improvement in a future PR if we can scrutinize into each of the fstring use case deeper. But they shouldn't block this PR.

@ZzEeKkAa What do you think the current developer should do to merge this PR into current working branches?

isVoid · 2025-03-26T21:11:46Z

numba_cuda/numba/cuda/cudadrv/dummyarray.py

+                newdims = (
+                    newdims[0:unknownidx]
+                    + (self.size // knownsize,)
+                    + newdims[unknownidx + 1 :]
+                )


We even have backslash line breaks! 🤣

isVoid · 2025-03-26T21:14:52Z

numba_cuda/numba/cuda/dispatcher.py

+                    mem, sz = cufunc.module.get_global_symbol(
+                        "%s__%s__" % (cufunc.name, name)
+                    )


This looks so much nicer, but it would be even better if ruff can auto format into f-strings.

isVoid · 2025-03-26T21:20:39Z

numba_cuda/numba/cuda/simulator/kernel.py

+            tid = "tid=%s" % list(self.threadIdx)
+            ctaid = "ctaid=%s" % list(self.blockIdx)
+            if str(e) == "":
+                msg = "%s %s" % (tid, ctaid)
            else:
-                msg = '%s %s: %s' % (tid, ctaid, e)
+                msg = "%s %s: %s" % (tid, ctaid, e)


Same, f-strings would be nice.

ZzEeKkAa · 2025-03-31T12:57:25Z

I managed to give a rough pass to the code which mostly looks good to me. I went over the new ruff config v.s. the flake8 config and discovered a nice small discussion around W503 and W504 - apparently it was controversial and ruff didn't implement them: astral-sh/ruff#4125

Yes, and in flake8 those where in the ignore list, so no changes for numba-cuda by simply removing them.

As for the f-string dicussion below - I realized they are unsafe fixes. They might be a nice code style improvement in a future PR if we can scrutinize into each of the fstring use case deeper. But they shouldn't block this PR.

Yes, lets target it in another PR.

@ZzEeKkAa What do you think the current developer should do to merge this PR into current working branches?

Simple merge + resolve conflicts generally should work, otherwise it will be nice to get .pre-commit-config.yaml and pyproject.toml from this MR (or git checkout first commit from this PR), run pre-commit on changed files and merge after this.

ZzEeKkAa · 2025-04-07T13:28:50Z

@gmarkall could we merge this PR?

gmarkall · 2025-04-08T08:57:19Z

I just merged this, but I had to do it outside the normal PR flow because even "rebase and merge" changes the commit hashes, which would mess up the .git-blame-ignore-revs file. As such, this isn't detected as merged, and I'll close this PR.

- Locate nvvm, libdevice, nvrtc, and cudart from nvidia-*-cu12 wheels (NVIDIA#155) - reinstate test (NVIDIA#226) - Restore PR NVIDIA#185 (Stop Certain Driver API Discovery for "v2") (NVIDIA#223) - Report NVRTC builtin operation failures to the user (NVIDIA#196) - Add Module Setup and Teardown Callback to Linkable Code Interface (NVIDIA#145) - Test CUDA 12.8. (NVIDIA#187) - Ensure RTC Bindings Clamp to the Maximum Supported CC (NVIDIA#189) - Migrate code style to ruff (NVIDIA#170) - Use less GPU memory in test_managed_alloc_driver_undersubscribe. (NVIDIA#188) - Update workflows to always use proxy cache. (NVIDIA#191)

- Locate nvvm, libdevice, nvrtc, and cudart from nvidia-*-cu12 wheels (#155) - reinstate test (#226) - Restore PR #185 (Stop Certain Driver API Discovery for "v2") (#223) - Report NVRTC builtin operation failures to the user (#196) - Add Module Setup and Teardown Callback to Linkable Code Interface (#145) - Test CUDA 12.8. (#187) - Ensure RTC Bindings Clamp to the Maximum Supported CC (#189) - Migrate code style to ruff (#170) - Use less GPU memory in test_managed_alloc_driver_undersubscribe. (#188) - Update workflows to always use proxy cache. (#191)

ZzEeKkAa force-pushed the feature/pre-commit-improvement branch from b863570 to 82b2ac5 Compare March 21, 2025 15:00

gmarkall added the 3 - Ready for Review Ready for review by team label Mar 24, 2025

ZzEeKkAa force-pushed the feature/pre-commit-improvement branch from 82b2ac5 to 15c31b6 Compare March 25, 2025 23:20

ZzEeKkAa added 3 commits March 25, 2025 16:23

Replace flake8 with ruff and pre-commit-hooks

e4bebf9

Migrate code style to ruff (pre-commit run -a)

06b6202

Add .git-blame-ignore-revs

6d7de70

ZzEeKkAa force-pushed the feature/pre-commit-improvement branch from 15c31b6 to 6d7de70 Compare March 25, 2025 23:23

isVoid approved these changes Mar 26, 2025

View reviewed changes

gmarkall closed this Apr 8, 2025

gmarkall mentioned this pull request Apr 8, 2025

Fix cuda.jit decorator inline #181

Merged

gmarkall mentioned this pull request Apr 22, 2025

Bump version to 0.9.0 #229

Merged

Conversation

ZzEeKkAa commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Old description

Uh oh!

ZzEeKkAa commented Mar 21, 2025

Uh oh!

isVoid commented Mar 21, 2025

Uh oh!

ZzEeKkAa commented Mar 21, 2025

Uh oh!

isVoid commented Mar 24, 2025

Uh oh!

gmarkall commented Mar 25, 2025

Uh oh!

leofang commented Mar 25, 2025

Uh oh!

ZzEeKkAa commented Mar 25, 2025

Uh oh!

ZzEeKkAa commented Mar 25, 2025

Uh oh!

ZzEeKkAa commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isVoid left a comment

Choose a reason for hiding this comment

Uh oh!

isVoid Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

isVoid Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

isVoid Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

ZzEeKkAa commented Mar 31, 2025

Uh oh!

ZzEeKkAa commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmarkall commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ZzEeKkAa commented Mar 21, 2025 •

edited

Loading

ZzEeKkAa commented Mar 26, 2025 •

edited

Loading

ZzEeKkAa commented Apr 7, 2025 •

edited

Loading