Add json APIs to pylibcudf #17025
Add json APIs to pylibcudf #17025
Conversation
mroeschke
added
improvement
| @@ -12,6 +12,7 @@ strings | |||
| find_multiple | |||
| findall | |||
| padding | |||
| json | |||
There was a problem hiding this comment.
The get_json_object and related APIs are not part of the cudf::strings namespace in libcudf.
https://github.com/rapidsai/cudf/blob/branch-24.12/cpp/include/cudf/json/json.hpp
Should pylibcudf match libcudf more closely than cudf?
There was a problem hiding this comment.
Yes pylibcudf should match libcudf more closely than cudf python. So, I think we should move json.pxd and json.pyx out of strings.
But a broader point: I think pylibcudf is first and foremost "python bindings for libcudf", so we should match libcudf as closely as possible. But pylibcudf is a python library so I think it makes sense over time to make it more "pythonic" (eg. easier to use with other python libraries).
cc. @vyasr
There was a problem hiding this comment.
Thanks for the catch. I moved this out of the string namespace.
There was a problem hiding this comment.
I agree with not putting this in the strings namespace.
@Matt711 I don't understand the relevance of your second point. I agree that we'll be making some cosmetic changes to make it more Pythonic than a raw translation of libcudf API calls, but does that have any particular bearing on what's being discussed here? Or were you just responding to David's question with a statement that while pylibcudf APIs should closely match libcudf APIs, they may not match exactly if there is a more Pythonic choice for the pylibcudf API that still faithfully reflects the semantics of the C++ functions?
There was a problem hiding this comment.
Or were you just responding to David's question with a statement that while pylibcudf APIs should closely match libcudf APIs, they may not match exactly if there is a more Pythonic choice for the pylibcudf API that still faithfully reflects the semantics of the C++ functions?
That's right. I realized I didn't add "Is that right?" before I @'d you because I wanted to get your thoughts.
mroeschke
changed the title
| @@ -97,7 +97,8 @@ skip = [ | |||
| ] | |||
|
|
|||
| [tool.pytest.ini_options] | |||
| addopts = "--tb=native --strict-config --strict-markers" | |||
| # --import-mode=importlib because two test_json.py exists and tests directory is not a structured module | |||
| addopts = "--tb=native --strict-config --strict-markers --import-mode=importlib" | |||
There was a problem hiding this comment.
According to https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html, --import-mode=importlib was added in pytest 6.0.
So we probably need a floor here:
Line 749 in 7173b52
like:
- pytest>=6.0,<8
There was a problem hiding this comment.
This is technically true, but I think we have other reasons why we wouldn't be compatible with lower pytest versions either and we're just generally underspecified. I don't think that we need to address this here and should instead look into this as part of rapidsai/build-planning#105.
There was a problem hiding this comment.
Sure, definitely not worth another run of CI for.
|
/merge |
Description
Contributes to #15162
Checklist