cudf icon indicating copy to clipboard operation
cudf copied to clipboard

Add `gdb` pretty-printers for simple types

Open upsj opened this issue 1 year ago • 0 comments

Description

This adds gdb pretty printers for rmm::device_uvector, thrust::*_vector, thrust::device_reference and cudf::*_span. The implementation is based on https://github.com/NVIDIA/thrust/pull/1631. I will probably backport the thrust-specific changes to there as well, but since the location of the thrust source is not fixed, I'd prefer having all types in a self-contained file.

Checklist

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

upsj avatar Aug 09 '22 17:08 upsj

This seems to be working for all the cases I tested. The csv reader is a good place to try them out, since it uses host_span, device_span and device_uvector.

upsj avatar Aug 11 '22 06:08 upsj

Is this the right home for all of this? You already added pretty printers for thrust in thrust. Pretty printers for RMM should probably live in RMM so more of RAPIDS can benefit?

harrism avatar Aug 11 '22 21:08 harrism

I believe it would be hard to import dependencies from other components, since they may be placed in an arbitrary location (build/$config/_deps/.../scripts/gdb-pretty-printers.py in rapids-compose if pulling it via CPM, somewhere else if using an existing library). If I wanted to use pretty-printers from the individual files, I'd have to run

source path/to/rmm/scripts/gdb-pretty-printer.py
source path/to/thrust/scripts/gdb-pretty-printer.py
source path/to/cudf/scripts/gdb-pretty-printer.py

manually instead of just sourcing a single file. All pretty-printers here rely on DeviceIterator and HostIterator, so they need to either pull in the file containing them or we duplicate the code across all of them.

OTOH, you mentioned thrust::*_vector no longer being used inside cudf, so maybe this isn't too much of a roadblock after all?

upsj avatar Aug 12 '22 05:08 upsj

Selfishly, I want to be able to find RMM pretty printers in RMM so that I can use them there and in other libraries. So I guess it's OK to duplicate them in cuDF, but I think they should be added to RMM as well.

harrism avatar Aug 15 '22 21:08 harrism

Codecov Report

:exclamation: No coverage uploaded for pull request base (branch-22.10@65a7821). Click here to learn what that means. Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.10   #11499   +/-   ##
===============================================
  Coverage                ?   86.41%           
===============================================
  Files                   ?      145           
  Lines                   ?    22993           
  Branches                ?        0           
===============================================
  Hits                    ?    19870           
  Misses                  ?     3123           
  Partials                ?        0           

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

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

codecov[bot] avatar Aug 15 '22 23:08 codecov[bot]

I'm also in favor of putting these in their respective repos. Perhaps a script can be used to wrap the

I believe it would be hard to import dependencies from other components, since they may be placed in an arbitrary location (build/$config/_deps/.../scripts/gdb-pretty-printers.py in rapids-compose if pulling it via CPM, somewhere else if using an existing library). If I wanted to use pretty-printers from the individual files, I'd have to run

source path/to/rmm/scripts/gdb-pretty-printer.py
source path/to/thrust/scripts/gdb-pretty-printer.py
source path/to/cudf/scripts/gdb-pretty-printer.py

manually instead of just sourcing a single file.

Could we not just have a script to do these calls? I'd rather not have duplicates to maintain.

davidwendt avatar Aug 16 '22 11:08 davidwendt

I totally see the issue - we could configure the rmm and thrust path via CMake and create the pretty-printer/load script inside build. Would that be an acceptable solution?

upsj avatar Aug 16 '22 11:08 upsj

I totally see the issue - we could configure the rmm and thrust path via CMake and create the pretty-printer/load script inside build. Would that be an acceptable solution?

I think that makes sense. These are only useful if you do a debug build after all.

davidwendt avatar Aug 16 '22 11:08 davidwendt

Considering the horrible things I have to do to try and make the deduplication work (concatenating files in CMake! exec(open(...).read()) in Python), I'm very open to hearing alternative approaches. gdb has its own command file syntax that could be used to load the individual files one after the other.

upsj avatar Aug 17 '22 18:08 upsj

Considering the horrible things I have to do to try and make the deduplication work (concatenating files in CMake! exec(open(...).read()) in Python), I'm very open to hearing alternative approaches. gdb has its own command file syntax that could be used to load the individual files one after the other.

The gdb approach seems reasonable. Perhaps we could create some documentation on debugging and include a gdb helper script to illustrate how to load/use these.

davidwendt avatar Aug 17 '22 18:08 davidwendt

I think this is in a good state for a review now. Sadly, since rmm is being installed by default inside rapids-compose, CPM picks up the installed rmm and thus the load-pretty-printers file doesn't get created there.

upsj avatar Aug 17 '22 20:08 upsj

style check cmake-format.............................................................Failed

karthikeyann avatar Aug 26 '22 20:08 karthikeyann

yes, this depends on https://github.com/rapidsai/rmm/pull/1088 being merged, so I removed the incorrect label. I'll update it soon™️

upsj avatar Aug 26 '22 20:08 upsj

@gpucibot merge

upsj avatar Sep 09 '22 07:09 upsj