[REVIEW] Switch `engine=cudf` to the new `JSON` reader #12509

galipremsagar · 2023-01-09T23:58:17Z

Description

Fixes: #12470
This PR:

Switches the cudf engine in json reader to the map to the newest JSON reader. Introduces the cudf_legacy engine to map to the old JSON reader.
Fixes an issue with _fsspec_data_transfer & compression that is required for the switch, these failures are already caught by tests.

Note: When engine='auto', and line=False, the pandas json reader will be used. To override the selection, we pass engine='cudf'.

Dependent on :

Adding support for an array of arrays in new json reader: Support "values" orient (array of arrays) in Nested JSON reader #12498

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

codecov · 2023-01-10T01:09:05Z

Codecov Report

Base: 86.58% // Head: 85.70% // Decreases project coverage by -0.88% ⚠️

Coverage data is based on head (c01e0ff) compared to base (b6dccb3).
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-23.02   #12509      +/-   ##
================================================
- Coverage         86.58%   85.70%   -0.88%     
================================================
  Files               155      155              
  Lines             24368    24870     +502     
================================================
+ Hits              21098    21316     +218     
- Misses             3270     3554     +284

Impacted Files	Coverage Δ
python/cudf/cudf/_version.py	`1.41% <0.00%> (-98.59%)`	⬇️
python/cudf/cudf/core/buffer/spill_manager.py	`72.50% <0.00%> (-7.50%)`	⬇️
python/cudf/cudf/core/buffer/spillable_buffer.py	`91.07% <0.00%> (-1.78%)`	⬇️
python/cudf/cudf/utils/dtypes.py	`77.85% <0.00%> (-1.61%)`	⬇️
python/cudf/cudf/options.py	`86.11% <0.00%> (-1.59%)`	⬇️
python/cudf/cudf/core/single_column_frame.py	`94.30% <0.00%> (-1.27%)`	⬇️
python/cudf/cudf/io/json.py	`91.04% <0.00%> (-1.02%)`	⬇️
...ython/custreamz/custreamz/tests/test_dataframes.py	`98.38% <0.00%> (-1.01%)`	⬇️
python/dask_cudf/dask_cudf/io/csv.py	`96.34% <0.00%> (-1.00%)`	⬇️
python/dask_cudf/dask_cudf/io/parquet.py	`91.81% <0.00%> (-0.59%)`	⬇️
... and 46 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…witch

python/cudf/cudf/tests/test_json.py

…witch

vuule

Looks good.
The only suggestion is to remove all engine='cudf' and engine="cudf" :)

docs/cudf/source/user_guide/io/read-json.md

python/cudf/cudf/tests/test_json.py

bdice · 2023-01-20T22:33:28Z

python/cudf/cudf/io/json.py

+        # TODO: Deprecated in 23.02, please
+        # give some time until `cudf_legacy`
+        # support can be removed completely.


"give some time until cudf_legacy support can be removed completely"

Will our one-release deprecation policy be enough for this, or do you think a special exception is warranted? If more time is needed, let's try to indicate the release in which support should be removed in the error message or perhaps a code comment.

Yeah, we definitely want to give this a special exception. I don't think we have a specific release decided yet to remove it completely. I updated the comment here.

cc: @GregoryKimball @vuule too.

I think we'll pick the removal release based on 23.02 feedback. In general, we'll remove the old reader as soon as there are no user issues specific to the new one.

That sounds reasonable! I just wanted to make sure we discussed this topic before merging. 👍

bdice · 2023-01-20T22:36:36Z

python/cudf/cudf/io/json.py

+        raise ValueError(
+            "engine='cudf_experimental' support has been removed, "
+            "use `engine='cudf'`"
+        )


Should this be a hard error or a deprecation warning that replaces the value with "cudf"?

Since it was experimental, I feel we have the flexibility to make this an error, what do you think?

I think that's justifiable -- but we'll want to delete this error at some point later. Therefore, it's the same amount of work for us as developers to deprecate it as to force an error. "Add warning, delete warning later" vs. "add error, delete error later."

No strong feelings here - resolve as you see fit.

python/cudf/cudf/io/json.py

python/dask_cudf/dask_cudf/io/tests/test_json.py

into read_json_switch

docs/cudf/source/user_guide/io/read-json.md

bdice

Looks good to me, once conversations are resolved.

python/dask_cudf/dask_cudf/io/json.py

vuule

Looks good. Looking forward to future removal of engine='cudf's :)

galipremsagar · 2023-01-20T23:43:17Z

Looks good. Looking forward to future removal of engine='cudf's :)

Which also means the removal of engine='pandas', actually the engine parameter entirely 😉

python/dask_cudf/dask_cudf/io/json.py

python/dask_cudf/dask_cudf/io/tests/test_json.py

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

…witch

karthikeyann

LGTM 👍
Looking forward to add more orients, and remove engine parameter entirely.

python/dask_cudf/dask_cudf/io/tests/test_json.py

rjzamora

Thanks @galipremsagar - Looks good on the dask side!

galipremsagar · 2023-01-23T23:32:16Z

/merge

karthikeyann · 2023-01-24T12:27:28Z

🚀

switch to new json reader

129e692

github-actions bot added the Python Affects Python cuDF API. label Jan 9, 2023

galipremsagar added improvement Improvement / enhancement to an existing function breaking Breaking change labels Jan 10, 2023

galipremsagar added 4 commits January 17, 2023 08:26

Merge remote-tracking branch 'upstream/branch-23.02' into read_json_s…

fff3c25

…witch

switch legacy

84705bf

Merge remote-tracking branch 'upstream/branch-23.02' into read_json_s…

d9266d8

…witch

drop experimental in more places

16b29f7

GregoryKimball assigned galipremsagar Jan 18, 2023

Merge remote-tracking branch 'upstream/branch-23.02' into read_json_s…

cfbda0b

…witch

karthikeyann reviewed Jan 20, 2023

View reviewed changes

python/cudf/cudf/tests/test_json.py Show resolved Hide resolved

galipremsagar added 3 commits January 20, 2023 12:45

update tests

3016f03

improve test

d4d6dd9

zip

8cf3b9a

galipremsagar changed the title ~~[WIP] Switch default to new json reader~~ [REVIEW] Switch default to new json reader Jan 20, 2023

galipremsagar changed the title ~~[REVIEW] Switch default to new json reader~~ [REVIEW] Switch default engine to the new JSON reader Jan 20, 2023

Merge remote-tracking branch 'upstream/branch-23.02' into read_json_s…

d880e35

…witch

galipremsagar added 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Jan 20, 2023

galipremsagar marked this pull request as ready for review January 20, 2023 22:04

galipremsagar requested review from a team as code owners January 20, 2023 22:04

galipremsagar requested review from bdice, brandon-b-miller and vuule January 20, 2023 22:04

vuule requested changes Jan 20, 2023

View reviewed changes

docs/cudf/source/user_guide/io/read-json.md Show resolved Hide resolved

python/cudf/cudf/tests/test_json.py Show resolved Hide resolved

bdice reviewed Jan 20, 2023

View reviewed changes

drop engine

fb32906

galipremsagar added 2 commits January 20, 2023 14:50

Merge branch 'read_json_switch' of https://github.com/galipremsagar/cudf

e80112f

into read_json_switch

drop engine when not needed

b6e8569

galipremsagar commented Jan 20, 2023

View reviewed changes

docs/cudf/source/user_guide/io/read-json.md Outdated Show resolved Hide resolved

docs/cudf/source/user_guide/io/read-json.md Outdated Show resolved Hide resolved

Apply suggestions from code review

78b55c0

bdice approved these changes Jan 20, 2023

View reviewed changes

python/dask_cudf/dask_cudf/io/json.py Outdated Show resolved Hide resolved

vuule approved these changes Jan 20, 2023

View reviewed changes

galipremsagar added 2 commits January 20, 2023 15:40

docs

76b7e10

docs

2641e49

galipremsagar changed the title ~~[REVIEW] Switch default engine to the new JSON reader~~ [REVIEW] Switch engine=cudf to the new JSON reader Jan 20, 2023

galipremsagar added 4 - Needs Dask Reviewer and removed 4 - Needs cuDF (Python) Reviewer labels Jan 21, 2023

rjzamora reviewed Jan 23, 2023

View reviewed changes

python/dask_cudf/dask_cudf/io/json.py Outdated Show resolved Hide resolved

python/dask_cudf/dask_cudf/io/tests/test_json.py Show resolved Hide resolved

galipremsagar and others added 3 commits January 23, 2023 09:55

Apply suggestions from code review

1df85fa

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

improve tests

5e0bde4

Merge remote-tracking branch 'upstream/branch-23.02' into read_json_s…

6c90095

…witch

galipremsagar requested a review from rjzamora January 23, 2023 15:57

karthikeyann approved these changes Jan 23, 2023

View reviewed changes

rjzamora reviewed Jan 23, 2023

View reviewed changes

python/dask_cudf/dask_cudf/io/tests/test_json.py Show resolved Hide resolved

address reviews

c01e0ff

galipremsagar requested a review from rjzamora January 23, 2023 18:19

rjzamora approved these changes Jan 23, 2023

View reviewed changes

galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Dask Reviewer labels Jan 23, 2023

rapids-bot bot merged commit ecef4e2 into rapidsai:branch-23.02 Jan 23, 2023

firestarman mentioned this pull request Jan 30, 2023

[BUG] GPU JSON reader fails to read the JSON string of an empty body NVIDIA/spark-rapids#7616

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Switch `engine=cudf` to the new `JSON` reader #12509

[REVIEW] Switch `engine=cudf` to the new `JSON` reader #12509

galipremsagar commented Jan 9, 2023 •

edited

Loading

codecov bot commented Jan 10, 2023 •

edited

Loading

vuule left a comment

bdice Jan 20, 2023 •

edited

Loading

galipremsagar Jan 20, 2023

vuule Jan 20, 2023

bdice Jan 20, 2023

bdice Jan 20, 2023

galipremsagar Jan 20, 2023

bdice Jan 20, 2023 •

edited

Loading

bdice left a comment

vuule left a comment

galipremsagar commented Jan 20, 2023

karthikeyann left a comment

rjzamora left a comment

galipremsagar commented Jan 23, 2023

karthikeyann commented Jan 24, 2023

[REVIEW] Switch engine=cudf to the new JSON reader #12509

[REVIEW] Switch engine=cudf to the new JSON reader #12509

Conversation

galipremsagar commented Jan 9, 2023 • edited Loading

Description

Checklist

codecov bot commented Jan 10, 2023 • edited Loading

Codecov Report

vuule left a comment

Choose a reason for hiding this comment

bdice Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

galipremsagar Jan 20, 2023

Choose a reason for hiding this comment

vuule Jan 20, 2023

Choose a reason for hiding this comment

bdice Jan 20, 2023

Choose a reason for hiding this comment

bdice Jan 20, 2023

Choose a reason for hiding this comment

galipremsagar Jan 20, 2023

Choose a reason for hiding this comment

bdice Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

vuule left a comment

Choose a reason for hiding this comment

galipremsagar commented Jan 20, 2023

karthikeyann left a comment

Choose a reason for hiding this comment

rjzamora left a comment

Choose a reason for hiding this comment

galipremsagar commented Jan 23, 2023

karthikeyann commented Jan 24, 2023

[REVIEW] Switch `engine=cudf` to the new `JSON` reader #12509

[REVIEW] Switch `engine=cudf` to the new `JSON` reader #12509

galipremsagar commented Jan 9, 2023 •

edited

Loading

codecov bot commented Jan 10, 2023 •

edited

Loading

bdice Jan 20, 2023 •

edited

Loading

bdice Jan 20, 2023 •

edited

Loading