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

Remove dependency on six. #9495

Merged
merged 3 commits into from
Nov 1, 2021
Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Oct 21, 2021

I noticed while working on #9448 that we import six but it doesn't appear to be a dependency of cuDF. Since that package is meant for Python 2 compatibility and we do not support Python 2, I think it can be removed.

six is probably being installed as a transitive dependency of some dependency of cuDF, if I were to guess, otherwise I'd think tests would fail.

Note: the queryutils.py module containing these lines may be somewhat outdated. The module is not covered by tests from 21.08 through 21.12, but was covered by tests in 21.06. It is currently only used in a couple places, which may deserve some cleanup: https://github.com/rapidsai/cudf/search?q=queryutils

@bdice bdice added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. tech debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 21, 2021
@bdice bdice self-assigned this Oct 21, 2021
@bdice bdice requested a review from a team as a code owner October 21, 2021 22:26
@vyasr
Copy link
Contributor

vyasr commented Oct 21, 2021

queryutils is definitely outdated. However, it is used in the implementation of df.query. I considered removing and replacing it via AST evaluation, but that effort was put on hold for a while due to the substantial refactorings of the AST internals undertaken during 21.10. Once I can assess that #8022 offers a suitable replacement for queryutils (no regressions with respect to the various features pandas expects df.query to have) queryutils should be removable.

@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #9495 (709762e) into branch-21.12 (ab4bfaa) will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9495      +/-   ##
================================================
- Coverage         10.79%   10.66%   -0.13%     
================================================
  Files               116      117       +1     
  Lines             18869    19727     +858     
================================================
+ Hits               2036     2104      +68     
- Misses            16833    17623     +790     
Impacted Files Coverage Δ
python/dask_cudf/dask_cudf/sorting.py 92.90% <0.00%> (-1.21%) ⬇️
python/cudf/cudf/io/csv.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/hdf.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
... and 66 more

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 77c6f1d...709762e. Read the comment docs.

@bdice
Copy link
Contributor Author

bdice commented Oct 25, 2021

The test failure will be resolved once #9508 is merged and CI is re-triggered.

@bdice bdice added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Oct 27, 2021
@bdice
Copy link
Contributor Author

bdice commented Oct 27, 2021

@gpucibot rerun tests

1 similar comment
@bdice
Copy link
Contributor Author

bdice commented Oct 27, 2021

@gpucibot rerun tests

@bdice
Copy link
Contributor Author

bdice commented Oct 27, 2021

Tests have failed on random download errors twice. I am re-running again. This is ready to merge once CI gets it together. 🙃

@galipremsagar
Copy link
Contributor

rerun tests

@vyasr
Copy link
Contributor

vyasr commented Oct 28, 2021

@bdice for future reference, the request to rerun test does not go through gpucibot.

@galipremsagar
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 59f2bdd into rapidsai:branch-21.12 Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants