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

Fix isort and black line length #350

Closed

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Jul 24, 2020

Appears that isort and black (via flake8) had different line lengths, which clashed with each other resulting in the lint always failing. This makes sure isort uses the same line length that black (and flake8) use. Also reruns isort, black, and flake8 to make sure everything is copacetic.

@jakirkham jakirkham requested a review from a team as a code owner July 24, 2020 23:58
@jakirkham jakirkham force-pushed the fix_isort_black_line_length branch from a981294 to a5baefe Compare July 25, 2020 00:04
@jakirkham jakirkham requested a review from a team as a code owner July 25, 2020 00:04
@jakirkham
Copy link
Member Author

This really weird. I'm not seeing this issue locally...


>>>> Detected in repo script, using 'ci/checks/style.sh' for build...
All done! ✨ 🍰 ✨
32 files would be left unchanged.


>>>> PASSED: isort style check




>>>> PASSED: black style check




>>>> FAILED: flake8 style check; begin output





>>>> FAILED: flake8 style check; end output

https://gpuci.gpuopenanalytics.com/blue/organizations/jenkins/rapidsai%2Fgpuci%2Fdask-cuda%2Fprb%2Fdask-cuda-style/detail/dask-cuda-style/458/pipeline

@jakirkham
Copy link
Member Author

Would suggest running isort and black locally. What I've seen is they both have a different way of handling this line. Depending on which one is run, this either becomes one line or is broken by a \. Not sure why they handle this differently.

from ucp._libs.topological_distance import \
TopologicalDistance # NOQA

@jakirkham
Copy link
Member Author

rerun tests

@jakirkham
Copy link
Member Author

Just to be clear, I tried both options (setting to 79 and 88). The problem I run into is black seems to ignore the configuration and still unwraps that line regardless of the line length specified. It may be a bug in how black works. Choosing 88 seems to at least get isort and black to cooperate.

@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2020

Codecov Report

Merging #350 into branch-0.15 will decrease coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.15     #350      +/-   ##
===============================================
- Coverage        60.30%   60.13%   -0.17%     
===============================================
  Files               17       17              
  Lines             1320     1342      +22     
===============================================
+ Hits               796      807      +11     
- Misses             524      535      +11     
Impacted Files Coverage Δ
dask_cuda/explicit_comms/comms.py 99.00% <100.00%> (ø)
dask_cuda/benchmarks/utils.py 0.00% <0.00%> (ø)
dask_cuda/benchmarks/local_cudf_merge.py 0.00% <0.00%> (ø)
dask_cuda/device_host_file.py 98.64% <0.00%> (+0.07%) ⬆️
dask_cuda/explicit_comms/dataframe_merge.py 90.82% <0.00%> (+0.08%) ⬆️
dask_cuda/utils.py 87.40% <0.00%> (+0.09%) ⬆️
dask_cuda/local_cuda_cluster.py 83.52% <0.00%> (+0.19%) ⬆️
dask_cuda/cuda_worker.py 72.50% <0.00%> (+0.34%) ⬆️
dask_cuda/initialize.py 93.33% <0.00%> (+0.74%) ⬆️

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 d9d666b...a5baefe. Read the comment docs.

@pentschev
Copy link
Member

TBH, it's still unclear to me what should be done here or why that's necessary. At least in its current state, this PR seems to be following a diverging path to that of other libraries, as per #350 (comment) .

@pentschev
Copy link
Member

Do we still need this @jakirkham ? It seems like we rely on specific versions of formatting packages, it's not really clear to me what's the intent of this PR.

@pentschev
Copy link
Member

It seems to me that formatting works properly provided we locally use the same versions as CI does (see https://github.com/rapidsai/integration/blob/branch-0.17/conda/recipes/versions.yaml for reference). Therefore, I'm tentatively closing this, please feel free to reopen if there's still something that needs to be done here @jakirkham .

@pentschev pentschev closed this Nov 30, 2020
@jakirkham jakirkham deleted the fix_isort_black_line_length branch November 30, 2020 22:49
@jakirkham
Copy link
Member Author

Mads fixed this issue in PR ( #477 ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants