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

Bump isort version to 5.6 #286

Merged
merged 9 commits into from
Jul 16, 2021
Merged

Bump isort version to 5.6 #286

merged 9 commits into from
Jul 16, 2021

Conversation

charlesbluca
Copy link
Member

This change is intended for 21.08

With isort 5.6.0 came the ability to resort .pxd files, in addition to .pyx. By bumping the version here, we could begin enforcing .pxd package sorting on RAPIDS projects with Cython code.

Also relevant to rapidsai/cudf#8215

@charlesbluca charlesbluca requested a review from a team as a code owner May 28, 2021 14:45
@charlesbluca charlesbluca mentioned this pull request May 28, 2021
Merged
@jakirkham
Copy link
Member

JFYI this is still targeting branch-21.06 as branch-21.08 wasn't available when this was opened. We are happy to move over to branch-21.08 once it is ready 🙂

@ajschmidt8 ajschmidt8 marked this pull request as draft June 7, 2021 17:32
@ajschmidt8
Copy link
Member

Converted this PR to a draft so that it doesn't accidentally get merged to 21.06.

@ajschmidt8 ajschmidt8 changed the base branch from branch-21.06 to branch-21.08 June 9, 2021 16:10
@charlesbluca
Copy link
Member Author

Is this safe to mark as ready for review?

@jakirkham
Copy link
Member

Yes it's been retargeted to the new branch

@charlesbluca charlesbluca marked this pull request as ready for review June 10, 2021 18:04
@charlesbluca
Copy link
Member Author

Trying to assert if this will be a breaking PR, i.e. if code resorted by isort=5.0 will pass checks with isort=5.6; depending on this, we might want to open follow up PRs with relevant Python repos to make sure the version in their pre-commit hooks is also bumped

@ajschmidt8
Copy link
Member

Trying to assert if this will be a breaking PR, i.e. if code resorted by isort=5.0 will pass checks with isort=5.6; depending on this, we might want to open follow up PRs with relevant Python repos to make sure the version in their pre-commit hooks is also bumped

@charlesbluca, sounds like we might want to figure that out before merging this. Please post your findings here. Until then, I'll switch this to a draft so we don't break anything with an accidental merge.

@ajschmidt8 ajschmidt8 marked this pull request as draft June 14, 2021 18:07
@charlesbluca
Copy link
Member Author

charlesbluca commented Jun 14, 2021 •
edited
Loading

It looks like the repos this could potentially break are those running isort that have Cython code:

Essentially, we would either want to make sure these repos' gpuCI isort checks are ignoring Cython code, or that their isort pre-commit hooks are resorting it and this is being enforced by gpuCI.

This was referenced Jun 14, 2021
Merged
Merged
Merged
@charlesbluca
Copy link
Member Author

charlesbluca commented Jun 14, 2021 •
edited
Loading

Opened up PRs for the impacted repos:

I skipped over rmm-testbed as it seems like that repo would probably pull the changes from rmm, but happy to open up a PR there too if that isn't the case

@ajschmidt8 ajschmidt8 marked this pull request as ready for review June 21, 2021 18:10
@ajschmidt8
Copy link
Member

Opened up PRs for the impacted repos:

I skipped over rmm-testbed as it seems like that repo would probably pull the changes from rmm, but happy to open up a PR there too if that isn't the case

@charlesbluca, rmm-testbed is fine to skip 👍. can you review the status of these PRs? I see a few failures. It seems some may be related to isort. I'm not sure if they'll be fixed after this dependency is updated. If everything is good to go here, I can start approving/merging all these PRs.

@charlesbluca
Copy link
Member Author

charlesbluca commented Jun 21, 2021 •
edited
Loading

Sure! I'd imagine the failures are because isort is now checking the PYX files and failing because they are not sorted properly - ideally, what we would want to do is run the isort=5.6 hook on all the codebases in these PRs as well, which should resolve any failures provided that there aren't any sorting quirks within the Cython files.

I can do this now, which should request review from the Python codeowners of the respective repos.

rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Jun 24, 2021
@charlesbluca
Bump isort, enable Cython package resorting (#806)
f7884b3 
With rapidsai/integration#286, the version of isort running on gpuCI will be bumped to 5.6.4, allowing us to enforce the sorting of packages in Cython (pyx, pxd) files. This PR intends to:

- Enable these checks in the gpuCI style script
- Enable Cython package resorting in the pre-commit hook
- Resort all the Cython files in this repo so they pass the newly enabled checks

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

Approvers:
  - Dillon Cullinan (https://github.com/dillon-cullinan)

URL: #806
@charlesbluca charlesbluca mentioned this pull request Jun 24, 2021
Merged
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Jun 24, 2021
@charlesbluca
Run updated isort hook on pxd files (#812)
3c86211 
I neglected to run the updated isort hooks from #806 before it was merged - this should ensure that rapidsai/integration#286 doesn't cause any CI failures here when it is merged.

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

Approvers:
  - https://github.com/jakirkham

URL: #812
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Jul 6, 2021
@charlesbluca
Bump isort, enable Cython package resorting (#419)
a470e17 
With rapidsai/integration#286, the version of `isort` running on gpuCI will be bumped to 5.6.4, allowing us to enforce the sorting of packages in Cython (pyx, pxd) files. This PR intends to:

- Enable these checks in the gpuCI style script
- Enable Cython package resorting in the pre-commit hook
- Resort all the Cython files in this repo so they pass the newly enabled checks

These checks are optional, meaning that even without this being merged, gpuCI should still pass on style checks even when rapidsai/integration#286 is merged.

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

Approvers:
  - H. Thomson Comer (https://github.com/thomcom)
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: #419
@charlesbluca
Copy link
Member Author

charlesbluca commented Jul 15, 2021 •
edited
Loading

With all the isort PRs merged, this should be safe to merge now

@galipremsagar
Copy link
Contributor

rerun tests

@jjacobelli jjacobelli merged commit 4a418b1 into rapidsai:branch-21.08 Jul 16, 2021
@charlesbluca charlesbluca mentioned this pull request Jul 16, 2021
Merged
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Jul 16, 2021
@charlesbluca
Bump isort to 5.6.4 and remove isort overrides made for 5.0.7 (#…
4540728 
…8755)

As a follow up to rapidsai/integration#286 being merged and bumping gpuCI's `isort` version to 5.6.4, this removes the temporary overrides made to avoid failures with the old conflicting version.

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Richard (Rick) Zamora (https://github.com/rjzamora)

URL: #8755
This was referenced Jul 21, 2021
Closed
Merged
rapids-bot bot pushed a commit to rapidsai/cucim that referenced this pull request Jul 21, 2021
@charlesbluca
Pin isort hook to 5.6.4 (#73)
1aa5802 
Pins the `isort` pre-commit hook to 5.6.4 to correspond with the version bump in gpuCI (see rapidsai/integration#286). Also moves the pre-commit configuration to the root directory so it can be detected when running `pre-commit`.

Authors:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

Approvers:
  - https://github.com/jakirkham

URL: #73
@charlesbluca charlesbluca mentioned this pull request Sep 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@jjacobelli jjacobelli jjacobelli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@charlesbluca @jakirkham @ajschmidt8 @galipremsagar @jjacobelli