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

Python/Cython bindings for multibyte_split #8998

Merged
merged 88 commits into from
Sep 17, 2021
Merged

Python/Cython bindings for multibyte_split #8998

merged 88 commits into from
Sep 17, 2021

Conversation

jdye64
Copy link
Contributor

@jdye64 jdye64 commented Aug 9, 2021 •
edited by vuule
Loading

Provides the Python/Cython bindings for #8702 multibyte_split. This PR depends on #8702 being merged first.

Closes #8557

cwharris added 30 commits June 26, 2021 21:59
@cwharris
multibyte-split scaffolding
e1b71e6
@cwharris
cudf::io::text::input_stream
836773a
@cwharris
trie test scaffolding
3e06c18
@cwharris
superstate + tests
ac14dbd
@cwharris
added device trie
ea8cee2
@cwharris
add superstate to multibyte_split
a4a8dd0
@cwharris
cub block scan superstates
094d2d2
@cwharris
block-wide superstate matching
1117ab8
@cwharris
fix superstate constructor bug where only the first 8 states were ini…
51b1444 
…tialized
@cwharris
multibyte_split multiple delimeter support
d1f7eb3
@cwharris
scan output-offsets in multibyte_split
a628d73
@cwharris
printf offsets in multibyte_split
e1cc84d
@cwharris
add match-length to trie to adjust for output offset in multibyte_split
c7177bc
@cwharris
adjust multibyte_split test case to expect delimiters to be retained …
42dc014 
…in output
@cwharris
printf match_begin and match_end for multibyte_split
5171711
@cwharris
multibyte_split test passing
6b62ceb
@cwharris
add multibyte_split comments, break test intentionally to work on mul…
a2c9756 
…ti-block scaling
@cwharris
multibyte_split add multi-block support
21b8b25
@cwharris
rename BYTES_PER_TILE to ITEMS_PER_TILE
f59a93e
@cwharris
add bounds check to multibyte_split load and flag
5fa112a
@cwharris
multibyte_split benchmark scaffolding
cf42fd0
@cwharris
multibyte_split increase threads per block and adjust test case.
e6e9741
@cwharris
use circular buffer in multibyte_split to allow for stream inputs
b5c2e05
@cwharris
update multibyte_split to work with streaming inputs
738af48
@cwharris
consolidate two passes of stream-scanning to a single function
0121b22
@cwharris
add tile_state partial to multibyte_split but dont use yet
a233ca2
@cwharris
add reusable tilestate callback to multibyte_split
4946058
@cwharris
begin working on warp-reduce window aggregation of tile state in mult…
d69aeca 
…ibyte_split
@cwharris
fix multibyte_split bug where non-streaming approach would hang
079d1ea
@cwharris
interleaved streaming io for multibyte_split
970aac2
@codecov
Copy link

codecov bot commented Sep 3, 2021 •
edited
Loading

Codecov Report

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

❗ Current head 799b7fb differs from pull request most recent head 6a06a5d. Consider uploading reports for the commit 6a06a5d to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #8998   +/-   ##
===============================================
  Coverage                ?   10.85%           
===============================================
  Files                   ?      116           
  Lines                   ?    19168           
  Branches                ?        0           
===============================================
  Hits                    ?     2080           
  Misses                  ?    17088           
  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 eb09d14...6a06a5d. Read the comment docs.

vyasr
vyasr requested changes Sep 7, 2021
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.

Can we add a couple of tests? Some of my changes are necessary for the code to work, and the API question I raised would be easier to discuss in the context of how this API would be called.

python/cudf/cudf/_lib/cpp/io/text.pxd Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/cpp/io/text.pxd Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/cpp/io/text.pxd Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/cpp/io/text.pxd Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/cpp/io/text.pxd Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/text.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/text.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/io/text.py Outdated Show resolved Hide resolved
python/cudf/cudf/utils/ioutils.py Show resolved Hide resolved
@vuule vuule requested a review from vyasr September 15, 2021 20:23
vyasr
vyasr requested changes Sep 15, 2021
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.

Looks good, just some minor nits to improve Python style.

python/cudf/cudf/_lib/text.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/io/text.py Outdated Show resolved Hide resolved
python/cudf/cudf/io/text.py Outdated Show resolved Hide resolved
python/cudf/cudf/tests/test_text.py Outdated Show resolved Hide resolved
python/cudf/cudf/tests/test_text.py Outdated Show resolved Hide resolved
python/cudf/cudf/utils/ioutils.py Show resolved Hide resolved
python/cudf/cudf/tests/test_text.py Outdated Show resolved Hide resolved
@vuule vuule requested a review from vyasr September 15, 2021 23:40
@jdye64
Copy link
Contributor Author

jdye64 commented Sep 16, 2021

rerun tests

@jdye64
Copy link
Contributor Author

jdye64 commented Sep 16, 2021

Keep getting git timeouts .... trying again. rerun tests

@jdye64
Copy link
Contributor Author

jdye64 commented Sep 16, 2021

rerun tests

@jdye64
Copy link
Contributor Author

jdye64 commented Sep 17, 2021

Github checkout of dask returned 429 code. Trying again

@jdye64
Copy link
Contributor Author

jdye64 commented Sep 17, 2021

rerun tests

vyasr
vyasr approved these changes Sep 17, 2021
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.

LGTM

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Needs cuDF (Python) Reviewer labels Sep 17, 2021
@vuule
Copy link
Contributor

vuule commented Sep 17, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 11781e8 into rapidsai:branch-21.10 Sep 17, 2021
@jdye64 jdye64 deleted the read_text_pyx_only branch September 18, 2021 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@galipremsagar galipremsagar galipremsagar approved these changes

@vyasr vyasr vyasr approved these changes

@skirui-source skirui-source Awaiting requested review from skirui-source

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Support multi-character newline delimiters with a read_text API
8 participants
@jdye64 @harrism @vuule @vyasr @galipremsagar @nvdbaranec @randerzander @cwharris