Add ruff linter #201

ksimpson-work · 2024-10-30T18:53:54Z

Add Ruff linter and format to pre-commit-hooks + make the appropriate changes in the code base so that we start with a passing commit.

some per-file ignores have been added, and some globals. I think these can be a work in progress, and I used my best judgement, but I would love to hear feedback.

Depends on #203

The only real bug this exposes is #203 which I am addressing in a separate PR and issue for cleanliness

Closes #195

copy-pr-bot · 2024-10-30T18:53:58Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

ksimpson-work · 2024-10-30T18:56:59Z

/ok to test

ksimpson-work · 2024-10-30T20:45:40Z

The actions seem to be broken as of a long time ago. The culprit being a faulty build command which looks for setup.py in cuda-python top level directory. I'm wondering if that will be addressed by QA, or if I/someone should fix it.

vzhurba01

I couple of comments:

Once you're ready, can you try testing the builds and tests across our supported Python versions (3.9 to 3.12) and 3.13? For the bindings just doing 3.9 and 3.13 is probably sufficient(?), just because the build there takes a while. In the past some of my own code formats had issues for certain versions, so some paranoia is creeping in. Surely since this is a linter all of that ought to be ok, but still 🙂.
~~Would need a follow-up PR into the 11.8.x branch. Just an FYI for when the time comes.~~ Actually no, this shouldn't be necessary since the only change expected on that branch are the bindings which aren't covered by the linter anyways. The only items remaining would be the tests/examples, but that is expected to stay the same.

cuda_bindings/pyproject.toml

cuda_bindings/cuda/bindings/_version.py

cuda_bindings/examples/4_CUDA_Libraries/conjugateGradientMultiBlockCG_test.py

cuda_core/cuda/core/experimental/_device.py

cuda_core/cuda/core/experimental/_event.py

cuda_core/cuda/core/experimental/_launcher.py

cuda_core/cuda/core/experimental/_stream.py

cuda_core/tests/example_tests/test_basic_examples.py

rwgk

Looks good to me, but what do you think about splitting this PR into two parts:

Add tox testing only.
Add ruff only with the tox testing in place already.

That would make it easier** to see what changes (if any?) are required to establish tox testing, and what changes are a consequence of introducing ruff.

easier** in the future, in case we have to backtrack for whatever reason.

.pre-commit-config.yaml

ksimpson-work · 2024-11-01T21:09:33Z

@vzhurba01 The changes have been verified against python 3.9 and 3.13 successfully.

rwgk

Another idea borrowed from pybind11:

https://github.com/pybind/pybind11/blob/75e48c5f959b4f0a49d8c664e059b6fb4b497102/.github/workflows/format.yml#L24-L37

This ensures that all PRs pass the pre-commit checks.

I didn't see that here. I'm not sure if you have another way of achieving the same?

ksimpson-work · 2024-11-19T18:45:53Z

Dependencies have been addressed and submitted.

rwgk

Looks good to me, but question: Do you want to wait merging this PR until after #249 is merged?

Minor: I think there are a couple files missing a newline at the end of the file, which can be annoying. VSCode has these settings: "Files Insert Final Newline", "Files: Trim Final Newlines", "Notebook: Insert Final Newline" that work well for me. I also have "Editor: Render Final Newline: off".

I think in another PR we should add a pre-commit checks for whitespace issues like that one.

ksimpson-work · 2024-11-20T20:08:53Z

yeah, I am not opposed to waiting for #249. I'm wokring on a follow up to 249 that will run the testsuites against different versions of python and cuda as well. I'll look into the newline, I would expect ruff formatter to handle that.

ksimpson-work · 2024-11-25T20:15:22Z

/ok to test

rwgk

Looks good to me. Keenan confirmed on chat that local testing passes for him.

leofang

LGTM overall. AI:

@ksimpson-work could you merge with the latest main and rerun the linter? There are merge conflicts
@vzhurba01 could you take one more look at cuda.bindings changes?

cuda_bindings/benchmarks/test_cupy.py

cuda_core/cuda/core/experimental/_memory.py

leofang · 2024-11-26T19:16:37Z

/ok to test

cuda_bindings/cuda/bindings/_version.py

vzhurba01

I noticed that fixes for some older comments got reverted. Perhaps happened due to a rebase? I unresolved those that I'd like to see re-applied.

cuda_bindings/pyproject.toml

cuda_core/pyproject.toml

ksimpson-work · 2024-11-26T23:54:46Z

Yes that is my bad. Rather than merge, I checked-out main and reran the linter but that reverted soem of the manual changes I forgot about. I believe everything has been addressed now.

vzhurba01

LGTM thank you @ksimpson-work!

leofang · 2024-11-27T02:37:48Z

Thanks @ksimpson-work and all! Glad this is set up. Let's get this in and follow up in new PRs if needed.

leofang · 2024-11-28T03:27:27Z

FYI, I added some basic docs mentioning about pre-commit and code linting in commit 30d65ed.

leofang · 2024-11-30T03:45:44Z

cuda_core/cuda/core/experimental/utils.py

@@ -2,4 +2,3 @@
 #
 # SPDX-License-Identifier: LicenseRef-NVIDIA-SOFTWARE-LICENSE

-from cuda.core.experimental._memoryview import StridedMemoryView, viewable


This technically is a breaking change. It slipped the code review as it was removed right before the refresh before merging, and was not caught by the tests because we're still adding them (#247). Fixing it in b11e1ae.

ksimpson-work requested review from vzhurba01, leofang and rwgk and removed request for vzhurba01 October 30, 2024 20:43

ksimpson-work marked this pull request as ready for review October 30, 2024 20:43

ksimpson-work marked this pull request as draft October 30, 2024 20:48

vzhurba01 reviewed Oct 31, 2024

View reviewed changes

rwgk reviewed Nov 1, 2024

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

rwgk previously approved these changes Nov 1, 2024

View reviewed changes

leofang added this to the cuda.core beta 2 milestone Nov 2, 2024

ksimpson-work marked this pull request as ready for review November 4, 2024 17:20

ksimpson-work changed the title ~~Ksimpson/ruff linter~~ add ruff linter Nov 4, 2024

ksimpson-work changed the title ~~add ruff linter~~ Add ruff linter Nov 4, 2024

ksimpson-work dismissed rwgk’s stale review via 431334e November 4, 2024 21:08

leofang assigned ksimpson-work Nov 16, 2024

leofang added support All things related to the project that can't be categorized P0 High priority - Must do! cuda.core Everything related to the cuda.core module labels Nov 16, 2024

ksimpson-work force-pushed the ksimpson/ruff_linter branch from 431334e to 28537dd Compare November 19, 2024 18:43

ksimpson-work requested review from rwgk and vzhurba01 November 19, 2024 18:43

re-run ruff linter and formatter on updated main

5e950f7

ksimpson-work force-pushed the ksimpson/ruff_linter branch from 28537dd to 5e950f7 Compare November 19, 2024 18:59

rwgk previously approved these changes Nov 20, 2024

View reviewed changes

merge main

8dc3eaf

ksimpson-work dismissed rwgk’s stale review via 8dc3eaf November 25, 2024 20:12

ksimpson-work added 2 commits November 25, 2024 12:13

merge main

c836125

Merge branch 'main' into ksimpson/ruff_linter

b2f5638

ksimpson-work requested review from rwgk November 25, 2024 20:15

rwgk previously approved these changes Nov 25, 2024

View reviewed changes

leofang reviewed Nov 26, 2024

View reviewed changes

cuda_bindings/benchmarks/test_cupy.py Show resolved Hide resolved

cuda_core/cuda/core/experimental/_memory.py Outdated Show resolved Hide resolved

merge main

1fed1e7

ksimpson-work dismissed rwgk’s stale review via 1fed1e7 November 26, 2024 18:38

ksimpson-work added 3 commits November 26, 2024 10:48

address multi line tuple changes

5433e53

address multi line tuple changes

6dcb611

fix the tuple commas which triggered inconsistent formatting

4c81da7

vzhurba01 reviewed Nov 26, 2024

View reviewed changes

cuda_bindings/cuda/bindings/_version.py Outdated Show resolved Hide resolved

exclude the _version files from formatting as well

6868ee2

vzhurba01 reviewed Nov 26, 2024

View reviewed changes

cuda_bindings/pyproject.toml Show resolved Hide resolved

cuda_core/pyproject.toml Outdated Show resolved Hide resolved

address comments

0b34c6d

fix newline oddity

9b86782

vzhurba01 approved these changes Nov 27, 2024

View reviewed changes

leofang merged commit fdc76e8 into main Nov 27, 2024

leofang deleted the ksimpson/ruff_linter branch November 27, 2024 02:37

leofang reviewed Nov 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ruff linter #201

Add ruff linter #201

ksimpson-work commented Oct 30, 2024 •

edited

Loading

copy-pr-bot bot commented Oct 30, 2024

ksimpson-work commented Oct 30, 2024

ksimpson-work commented Oct 30, 2024

vzhurba01 left a comment •

edited

Loading

rwgk left a comment

ksimpson-work commented Nov 1, 2024

rwgk left a comment

ksimpson-work commented Nov 19, 2024

rwgk left a comment

ksimpson-work commented Nov 20, 2024

ksimpson-work commented Nov 25, 2024

rwgk left a comment

leofang left a comment •

edited

Loading

leofang commented Nov 26, 2024

vzhurba01 left a comment •

edited

Loading

ksimpson-work commented Nov 26, 2024

vzhurba01 left a comment

leofang commented Nov 27, 2024

leofang commented Nov 28, 2024

leofang Nov 30, 2024

Add ruff linter #201

Add ruff linter #201

Conversation

ksimpson-work commented Oct 30, 2024 • edited Loading

copy-pr-bot bot commented Oct 30, 2024

ksimpson-work commented Oct 30, 2024

ksimpson-work commented Oct 30, 2024

vzhurba01 left a comment • edited Loading

Choose a reason for hiding this comment

rwgk left a comment

Choose a reason for hiding this comment

ksimpson-work commented Nov 1, 2024

rwgk left a comment

Choose a reason for hiding this comment

ksimpson-work commented Nov 19, 2024

rwgk left a comment

Choose a reason for hiding this comment

ksimpson-work commented Nov 20, 2024

ksimpson-work commented Nov 25, 2024

rwgk left a comment

Choose a reason for hiding this comment

leofang left a comment • edited Loading

Choose a reason for hiding this comment

leofang commented Nov 26, 2024

vzhurba01 left a comment • edited Loading

Choose a reason for hiding this comment

ksimpson-work commented Nov 26, 2024

vzhurba01 left a comment

Choose a reason for hiding this comment

leofang commented Nov 27, 2024

leofang commented Nov 28, 2024

leofang Nov 30, 2024

Choose a reason for hiding this comment

ksimpson-work commented Oct 30, 2024 •

edited

Loading

vzhurba01 left a comment •

edited

Loading

leofang left a comment •

edited

Loading

vzhurba01 left a comment •

edited

Loading