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

Update pre-commit to run black 22.3.0 #10523

Merged
merged 5 commits into from
Mar 28, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Mar 28, 2022

This PR updates us to use black 22.3.0, which is now necessary because older versions of black are not compatible with current versions of Click (see psf/black#2964 is resolved).

I've opened this for 22.06 since I don't see any open PRs attempting to merge into 22.04 anymore, but this issue will block CI (which runs style checks using pre-commit) so if necessary I can backport.

@vyasr vyasr added bug Something isn't working 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. non-breaking Non-breaking change labels Mar 28, 2022
@vyasr vyasr self-assigned this Mar 28, 2022
@github-actions github-actions bot removed the Python Affects Python cuDF API. label Mar 28, 2022
@github-actions github-actions bot added the conda label Mar 28, 2022
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 am seeing CI failures. This should fix the problem until we can identify a longer-term fix (which may require upgrading black).

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.06@0d78007). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 0555dbc differs from pull request most recent head 631d056. Consider uploading reports for the commit 631d056 to get more accurate results

@@               Coverage Diff               @@
##             branch-22.06   #10523   +/-   ##
===============================================
  Coverage                ?   86.33%           
===============================================
  Files                   ?      140           
  Lines                   ?    22294           
  Branches                ?        0           
===============================================
  Hits                    ?    19248           
  Misses                  ?     3046           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d78007...631d056. Read the comment docs.

@vyasr vyasr requested a review from a team as a code owner March 28, 2022 19:51
@github-actions github-actions bot added the Python Affects Python cuDF API. label Mar 28, 2022
@vyasr vyasr requested review from a team as code owners March 28, 2022 19:52
@github-actions github-actions bot removed the conda label Mar 28, 2022
@vyasr vyasr changed the title Pin click version for black compatibility Update pre-commit to run black 22.3.0 Mar 28, 2022
@vyasr
Copy link
Contributor Author

vyasr commented Mar 28, 2022

Just FYI I'm following up on what to do regarding the outstanding copyright changes here,

@ajschmidt8 ajschmidt8 merged commit 90882d3 into rapidsai:branch-22.06 Mar 28, 2022
def from_scalar(
val: ScalarLike,
size: int
) -> ColumnBase: # TODO: This should be Scalar, not ScalarLike
Copy link
Contributor

Choose a reason for hiding this comment

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

@vyasr FYI this reformat drops comments in .pyi files. I noticed this in a previous attempt to update black but I wasn't sure how to address it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the note. I'll see what we can do about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the issue is specifically the inline comments on lines that get compressed. I guess we'll just have to remember to put comments on separate lines from now on in pyi files.

Copy link
Member

Choose a reason for hiding this comment

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

With TODOs, wonder if we should just file these as issues. That would make them easier to track, cross-reference, triage, resolve, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair question. I try to use my best judgment. TODOs have a lower energy barrier and issues feel excessive to track one-line changes, but there's definitely a visibility cost. Especially given the current state of cudf internals (heavily in flux) I'm inclined to be a bit lax about this for now. Maybe we get stricter in the future once we have a clearer design and code isn't constantly being deleted and rewritten, but at the moment it's not uncommon for TODOs like this to become moot when the relevant code paths are removed wholesale.

Copy link
Member

Choose a reason for hiding this comment

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

...at the moment it's not uncommon for TODOs like this to become moot when the relevant code paths are removed wholesale

Honestly this is my biggest worry with TODOs and have seen this happen to other code bases. Comments left that no longer pertain to the code they are near with no one that has a clue what they mean. It is clearer to see what issues intended and when they are resolved.

rapids-bot bot pushed a commit that referenced this pull request Mar 28, 2022
Adds back a comment that was accidentally removed in #10523.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #10526
rapids-bot bot pushed a commit that referenced this pull request Mar 28, 2022
This is a follow-up to #10523 that updates the conda environment now that the [conda-forge package should be available](conda-forge/black-feedstock#42).

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - https://github.com/jakirkham
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: #10525
jjacobelli added a commit that referenced this pull request Mar 30, 2022
This PR pins Click to a version supported by the version of black that we use. #10523 is too disruptive to backport to 22.04 in the middle of a release, so this is a minimum change to unblock linter runs.

Authors:
   - Vyas Ramasubramani (https://github.com/vyasr)
   - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
   - GALI PREM SAGAR (https://github.com/galipremsagar)
rapids-bot bot pushed a commit that referenced this pull request Mar 30, 2022
…0541)

This PR undoes #10535 (which was just a patch for cudf 22.04) on cudf 22.06 since we have implemented the longer term solution in #10523.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Ashwin Srinath (https://github.com/shwina)

URL: #10541
@vyasr vyasr deleted the fix/black_pre_commit branch March 31, 2022 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants