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.

Sign up for GitHub

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

Jump to bottom

lint: replace isort with Ruff's rule I #16685

Merged
merged 23 commits into from
Oct 25, 2024
Merged

lint: replace isort with Ruff's rule I #16685

merged 23 commits into from
Oct 25, 2024

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Aug 29, 2024

Description

since #15312 moved formatting from Black to Rufft, it would make sense also unify import formatting under the same ruff so use build-in I rule instead of additional isort

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@Borda Borda requested review from a team as code owners August 29, 2024 00:03
@Borda Borda requested a review from bdice August 29, 2024 00:03
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 29, 2024

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.

@bdice
Copy link
Contributor

bdice commented Aug 29, 2024

Hi @Borda, thanks for this. However, we have been keeping isort because ruff does not support Cython code but isort does. See this discussion for more info: #14882 (comment)

@vyasr
Copy link
Contributor

vyasr commented Aug 29, 2024

Hi @Borda, thanks for this. However, we have been keeping isort because ruff does not support Cython code but isort does. See this discussion for more info: #14882 (comment)

@bdice the rest of the discussion in that thread does suggest that we'd be OK with making the switch and losing the limited Cython support. Maybe should go ahead with this now.

@bdice
Copy link
Contributor

bdice commented Aug 29, 2024

You’re right. I forgot the rest of that discussion. @vyasr If you’re in support, I am fine with it.

vyasr
vyasr reviewed Aug 29, 2024
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
@vyasr vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 3, 2024
@vyasr
Copy link
Contributor

vyasr commented Sep 3, 2024

/ok to test

@Borda Borda requested review from a team as code owners September 3, 2024 22:03
@github-actions github-actions bot added Python Affects Python cuDF API. cudf.pandas Issues specific to cudf.pandas labels Sep 3, 2024
@Borda Borda requested a review from vyasr September 3, 2024 22:07
@vyasr
Copy link
Contributor

vyasr commented Sep 4, 2024

You'll notice that we have isort configuration in each of our Python package's pyproject.toml files, like cudf's here. We definitely want to preserve those rules, so you'll need to add those back as well. #14882 (comment) discusses a bit how we could accomplish this in a way that shares most of the logic between the different packages in the repo. ruff's docs also have some discussion of how to ensure near-equivalent results, and if you scroll down you'll see discussion of the first/third-party configuration and more. Hopefully that provides you a good enough starting point, but please follow up if you have more questions!

@vyasr
Copy link
Contributor

vyasr commented Sep 25, 2024

@Borda let us know if you need any help here. Please also retarget this PR at branch-24.12, we are on the verge of freezing 24.10.

@Borda
Copy link
Contributor Author

Borda commented Sep 27, 2024 •
edited
Loading

Please also retarget this PR at branch-24.12, we are on the verge of freezing 24.10.

sure will do it over weekend 🦩

@Borda Borda requested review from a team as code owners October 16, 2024 10:02
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue Java Affects Java cuDF API. cudf.polars Issues specific to cudf.polars pylibcudf Issues specific to the pylibcudf package labels Oct 16, 2024
@KyleFromNVIDIA
Copy link
Contributor

/ok to test

wence-
wence- reviewed Oct 21, 2024
View reviewed changes
python/dask_cudf/dask_cudf/backends.py Outdated Show resolved Hide resolved
python/pylibcudf/pyproject.toml Show resolved Hide resolved
@KyleFromNVIDIA
Copy link
Contributor

/ok to test

@Borda Borda requested a review from wence- October 23, 2024 11:39
@bdice
Copy link
Contributor

bdice commented Oct 24, 2024

@Borda I'm going to trigger tests to run -- we can hold off on merging the upstream for now, unless there are conflicts.

@bdice
Copy link
Contributor

bdice commented Oct 24, 2024

/ok to test

bdice
bdice reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I have a few local changes to push, then will re-trigger CI. I think this is nearly ready?

docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Oct 24, 2024

/ok to test

bdice
bdice approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I'm happy with this, assuming that CI passes. @vyasr @wence- Can you give a final check, so we don't hold this PR in limbo? (it has been requiring a lot of upstream-merges and conflict-resolution)

vyasr
vyasr reviewed Oct 25, 2024
View reviewed changes
python/cudf/pyproject.toml Outdated Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Oct 25, 2024

/ok to test

@Borda Borda requested a review from vyasr October 25, 2024 18:34
vyasr
vyasr approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Cool this LGTM now.

@bdice
Copy link
Contributor

bdice commented Oct 25, 2024

/merge

@rapids-bot rapids-bot bot merged commit 5cba4fb into rapidsai:branch-24.12 Oct 25, 2024
104 checks passed
@wence-
Copy link
Contributor

wence- commented Oct 28, 2024

Thanks @Borda!

@vyasr
Copy link
Contributor

vyasr commented Oct 28, 2024

Thanks for this @Borda!

@bdice bdice mentioned this pull request Dec 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@jakirkham jakirkham jakirkham approved these changes

@vyasr vyasr vyasr approved these changes

@brandon-b-miller brandon-b-miller Awaiting requested review from brandon-b-miller brandon-b-miller is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@KyleFromNVIDIA KyleFromNVIDIA Awaiting requested review from KyleFromNVIDIA

@wence- wence- Awaiting requested review from wence-

Assignees
No one assigned
Labels
cudf.pandas Issues specific to cudf.pandas improvement Improvement / enhancement to an existing function non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
cuDF Python
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Borda @bdice @vyasr @wence- @KyleFromNVIDIA @jakirkham