-
Notifications
You must be signed in to change notification settings - Fork 199
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
[Internal Cleanup] pre-commit ruff (excluding docs/tools, libcudacxx/test) #3110
Conversation
dcfc5d1
to
b0f5b41
Compare
/ok to test |
I think it's great to have some autoformatting (and linting) for python code! What's most important to me is that pre-commit and the CI can handle and check that for me. I assume this is the case? Regarding code style, I have no experience what's available and what's best practice. Looking at the diff, I prefer code to be dense if possible (less line breaks and less empty lines), but otherwise I have no opinions. Thank you for engaging in this! |
- case_df = remove_matching_distributions(alpha, case_df)
+ remove_matching_distributions(alpha, case_dfs) # TODO(rwgk): CORRECT? Using the function definition: def remove_matching_distributions(alpha, df):
closure = functools.partial(distributions_are_different, alpha)
return df[df.apply(closure, axis=1)] I don't think the diff is correct, because |
Yes. This simply hooks into the existing pre-commit. You can run locally (usually I use
I'm striving to keeping the defaults as much as possible. — ruff more-or-less supersedes black. black deliberately made it hard for users to be creative with style options. |
804e898
to
85e18f4
Compare
🟨 CI finished in 1h 46m: Pass: 94%/174 | Total: 3d 06h | Avg: 27m 09s | Max: 1h 09m | Hits: 61%/20169
|
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
CUDA Experimental | |
+/- | python |
CCCL C Parallel Library | |
Catch2Helper |
Modifications in project or dependencies?
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | python |
+/- | CCCL C Parallel Library |
+/- | Catch2Helper |
🏃 Runner counts (total jobs: 174)
# | Runner |
---|---|
124 | linux-amd64-cpu16 |
25 | linux-amd64-gpu-v100-latest-1 |
15 | windows-amd64-cpu16 |
10 | linux-arm64-cpu16 |
85e18f4
to
c16710f
Compare
@@ -86,7 +86,7 @@ def __exit__(self, type, value, traceback): | |||
def rename_folder(staging_dir: StagingDirectory, folder_name: str): | |||
try: | |||
staging_dir.promote_and_rename(folder_name) | |||
except OSError as exc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be formatting anything in packman
because this is vendored code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is helpful, this is how we exclude files in RAPIDS: pyproject.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will exclude (currently testing here: 8ac2254)
docs/tools/repoman/repoman.py
Outdated
@@ -1,7 +1,5 @@ | |||
import os | |||
import sys | |||
import io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with repoman.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will exclude (currently testing here: 8ac2254)
assert sys.platform.startswith('linux') or sys.platform.startswith('darwin') \ | ||
or sys.platform.startswith('cygwin') or sys.platform.startswith('freebsd') \ | ||
or sys.platform.startswith('netbsd') | ||
assert ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also unsure if we should be mucking with libcu++'s testing scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is helpful in deciding, I have extremely high confidence in the correctness of the rewrites that ruff
uses. The number of invalid changes it makes is vanishingly small. I looked at the changes in this file and a few others in libcudacxx/
, they seem valid to me, and they improve the quality of the code imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll exclude this, too. We can work on this in a follow-on PR, as needed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do! The formatting in those scripts is abysmal. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks! I'll work on that as soon as this PR is merged. It should be easy because I have the fixes already, although there was a weird "old" MSVC error that I'd need to debug if it still pops up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the two commits requested:
Ruff configs are mostly good, I have two suggestions.
Manual changes look good to me.
The one concerning pattern here is that there are many instances of from X import Y as Y
. The as Y
is redundant and should be removed.
.pre-commit-config.yaml
Outdated
# TODO/REMINDER: add the Ruff vscode extension to the devcontainers | ||
# Ruff, the Python auto-correcting linter/formatter written in Rust | ||
- repo: https://github.com/astral-sh/ruff-pre-commit | ||
rev: v0.8.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest is v0.8.2.
rev: v0.8.1 | |
rev: v0.8.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 8ac2254
(Currently in a scratch PR; once it's settled there I'll force push here.)
pyproject.toml
Outdated
@@ -0,0 +1,2 @@ | |||
[tool.ruff] | |||
target-version = "py312" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the minimum version that CCCL aims to support? That is what should be used here.
I would encourage 3.10 (minimum for RAPIDS, minimum for NEP29).
Other possibilities:
- Python 3.9 is still receiving security updates until October 2025
- Python 3.11 is the minimum for SPEC 0 but I feel that is too aggressive
target-version = "py312" | |
target-version = "py310" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 8ac2254
It didn't change anything. TBH I'm still not sure if the pyproject.toml file actually has any effect for ruff running via pre-commit. E.g. putting the exclusions here definitely didn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should be in the
[tool.ruff.lint]
key to take effect.
Yes, that's what I tried, a few times actually while working on this project (including the "prior PRs" listed in the PR description). Each time it didn't work.
But I believe it's not worth spending time trying harder, at least not right now: I think it's best to not run ruff
directly, but to always simply run pre-commit
.
@bdice wrote:
The ruff error is e.g.:
I asked ChatGPT about it: https://docs.google.com/document/d/1VoBEoLOLuKLdi2zSuXm94_EEqkIyNzSrKLVLmX_4p8w/edit?usp=sharing
|
I recommend these solutions for F401, in order of preference:
|
🟨 CI finished in 2h 28m: Pass: 96%/174 | Total: 3d 09h | Avg: 28m 04s | Max: 1h 05m | Hits: 51%/20178
|
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
CUDA Experimental | |
+/- | python |
CCCL C Parallel Library | |
Catch2Helper |
Modifications in project or dependencies?
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | python |
+/- | CCCL C Parallel Library |
+/- | Catch2Helper |
🏃 Runner counts (total jobs: 174)
# | Runner |
---|---|
124 | linux-amd64-cpu16 |
25 | linux-amd64-gpu-v100-latest-1 |
15 | windows-amd64-cpu16 |
10 | linux-arm64-cpu16 |
@bdice what do you think about this? — a4eab4b (That commit is in a scratch PR, because I'm force-pushing to this PR, to keep the organization into 3 commits as explained in the PR description.) |
c16710f
to
15693d4
Compare
Based on testing under #3130 this PR should be good for merging now. Compared to what you reviewed before:
I expect that the only test failures here will be resolved by @pciolkosz 's PR #3129. — I'm actually testing that under #3130, it'll probably be done in ~1 hour; I'd be very surprised if the testing there doesn't finish cleanly. |
🟨 CI finished in 1h 32m: Pass: 97%/174 | Total: 1d 03h | Avg: 9m 31s | Max: 33m 34s | Hits: 90%/22406
|
Project | |
---|---|
+/- | CCCL Infrastructure |
libcu++ | |
+/- | CUB |
+/- | Thrust |
CUDA Experimental | |
+/- | python |
CCCL C Parallel Library | |
Catch2Helper |
Modifications in project or dependencies?
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | python |
+/- | CCCL C Parallel Library |
+/- | Catch2Helper |
🏃 Runner counts (total jobs: 174)
# | Runner |
---|---|
124 | linux-amd64-cpu16 |
25 | linux-amd64-gpu-v100-latest-1 |
15 | windows-amd64-cpu16 |
10 | linux-arm64-cpu16 |
Testing under PR #3130, which is this PR + PR #3129, passed: #3130 (comment) |
15693d4
to
c716ce3
Compare
🟩 CI finished in 2h 33m: Pass: 100%/174 | Total: 3d 01h | Avg: 25m 19s | Max: 1h 25m | Hits: 77%/22406
|
Project | |
---|---|
+/- | CCCL Infrastructure |
libcu++ | |
+/- | CUB |
+/- | Thrust |
CUDA Experimental | |
+/- | python |
CCCL C Parallel Library | |
Catch2Helper |
Modifications in project or dependencies?
Project | |
---|---|
+/- | CCCL Infrastructure |
+/- | libcu++ |
+/- | CUB |
+/- | Thrust |
+/- | CUDA Experimental |
+/- | python |
+/- | CCCL C Parallel Library |
+/- | Catch2Helper |
🏃 Runner counts (total jobs: 174)
# | Runner |
---|---|
124 | linux-amd64-cpu16 |
25 | linux-amd64-gpu-v100-latest-1 |
15 | windows-amd64-cpu16 |
10 | linux-arm64-cpu16 |
Thanks everybody! :-) :-) |
Consolidation of results obtained in prior incremental PRs into 3 commits:
The cleanup uncovered these oversights:
For easy future reference, the list of prior incremental PRs (all obsolete): #3104, #3101, #3099, #3086