[REVIEW] Fix an issue with reading raw string in cudf.read_json
#10924
Conversation
galipremsagar
added
bug
galipremsagar
removed
the
improvement
|
rerun tests |
| if _is_local_filesystem(fs): | ||
| # Doing this as `read_json` accepts a json string | ||
| # path_or_data need not be a filepath like string | ||
| if os.path.exists(paths[0]): | ||
| path_or_data = paths if len(paths) > 1 else paths[0] | ||
| if len(paths): |
There was a problem hiding this comment.
Did we lose the check for len(paths) == 0 above this intentionally? Even if _is_local_filesystem returns false, I think we still need paths to be nonempty.
There was a problem hiding this comment.
Yea, this was intentional because fsspec tends to not return any paths now. However, this comment lead me to think about the check when _is_local_filesystem is False i.e., in else block I was missing this check. Added it now.
There was a problem hiding this comment.
Right, we should be erroring based on len(paths) == 0 only in the case when _is_local_filesystem returns False. In the True case we should check for paths being nonempty but only error if paths is nonempty and those paths don't exist. Empty paths does not constitute an error, which is the change in fsspec that is causing our tests to fail.
Co-authored-by: Bradley Dice <[email protected]>
Co-authored-by: Bradley Dice <[email protected]>
There was a problem hiding this comment.
Couple questions, but nothing blocking (although I am curious about the os.path.exists->fs.exists change). Let's prioritize getting this merged ASAP to unblock CI. Thanks @galipremsagar!
| if _is_local_filesystem(fs): | ||
| # Doing this as `read_json` accepts a json string | ||
| # path_or_data need not be a filepath like string | ||
| if os.path.exists(paths[0]): | ||
| path_or_data = paths if len(paths) > 1 else paths[0] | ||
| if len(paths): |
There was a problem hiding this comment.
Right, we should be erroring based on len(paths) == 0 only in the case when _is_local_filesystem returns False. In the True case we should check for paths being nonempty but only error if paths is nonempty and those paths don't exist. Empty paths does not constitute an error, which is the change in fsspec that is causing our tests to fail.
galipremsagar
added
5 - Ready to Merge
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10924 +/- ##
================================================
+ Coverage 86.30% 86.32% +0.02%
================================================
Files 144 144
Lines 22665 22668 +3
================================================
+ Hits 19560 19569 +9
+ Misses 3105 3099 -6
Continue to review full report at Codecov.
|
|
@gpucibot merge |
Fixes issue described here: #10275 (comment)
This PR removes a false error that says path couldn't be resolved. But that isn't true incase of a json reader where the input can be a json string itself, hence to resolve this issue
is_raw_text_like_inputthat indicates an IO reader(like read_json) is calling the utility function and the path need not be a valid one. In case of a true invalid path, either fsspec or libcudf throws a file not found error.