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

[FEA] Move cudf::test::print out of test into a public API #13450

Closed
ttnghia opened this issue May 25, 2023 · 2 comments · Fixed by #13720
Closed

[FEA] Move cudf::test::print out of test into a public API #13450

ttnghia opened this issue May 25, 2023 · 2 comments · Fixed by #13720
Labels
0 - Backlog In queue waiting for assignment feature request New feature or request improvement Improvement / enhancement to an existing function

Comments

@ttnghia
Copy link
Contributor

ttnghia commented May 25, 2023

With the growth of libcudf development, there is also an increasing number of debugging tasks. Printing a column content is one of the useful debugging approaches. In cudf, we can easily call cudf::test::print for that purpose, but the downstream applications don't have access to such functionality in the C++ test module.

I propose to change the function cudf::test::print into a public API cudf::print(column_view const&) or cudf::to_string(column_view const&).

@ttnghia ttnghia added feature request New feature or request Needs Triage Need team to review and classify labels May 25, 2023
@davidwendt
Copy link
Contributor

I would rather consider a cmake compile option that links the libcudftestutil.a with libcudf.so such that you can use cudf::test::print within libcudf functions temporarily for development.
There is a bunch of code in cudf::test::print and I fear moving it to cudf::print would generate too many distracting requirements including scalability and performance. I feel it is a perfectly suited for debugging small test cases and would prefer it be maintained in cudf::test.

Another option is to use the CSV writer with a data_sink that takes a std::vector<char>. The CSV writer contains alot of specialized code to convert columns to strings and I would not want to duplicate it unnecessarily.

@GregoryKimball GregoryKimball added 0 - Backlog In queue waiting for assignment Spark Functionality that helps Spark RAPIDS and removed Needs Triage Need team to review and classify labels Jul 10, 2023
@ttnghia ttnghia added improvement Improvement / enhancement to an existing function and removed Spark Functionality that helps Spark RAPIDS labels Jul 14, 2023
@ttnghia
Copy link
Contributor Author

ttnghia commented Jul 18, 2023

I have another idea: continue to keep cudf::test::print in the test framework but make that utility function independently compilable in a separate TU. When somebody needs to use it, they can manually change the cmake file to include such TU directly into libcudf.

See #13720.

rapids-bot bot pushed a commit that referenced this issue Oct 23, 2023
This PR extracts the implementation of the debug utility function `cudf::test::print()` from `column_utilities.hpp/cu` into its separate header/source files (`debug_utilities.hpp/cu`) for better organizing the relevant code. The new header name is also more expressive and more relevant to its purpose.

The changes in this PR are only moving code around. Not any new functionality or implementation was added.
Closes #13450 (although this is not to address that issue).

Authors:
  - Nghia Truong (https://github.com/ttnghia)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Robert Maynard (https://github.com/robertmaynard)
  - David Wendt (https://github.com/davidwendt)

URL: #13720
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Backlog In queue waiting for assignment feature request New feature or request improvement Improvement / enhancement to an existing function
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants