Refactor contiguous_split API into contiguous_split.hpp #13186
Conversation
github-actions
bot
added
Java
abellina
added
improvement
|
Java tests passed, so I took this out of draft. |
|
Python builds fail because of unrelated (already known) reason. |
…udf into refactor_contig_split_headers
…_contig_split_headers
|
@ttnghia Thanks for the reviews so far. I have addressed the things I could address, and commented on those I couldn't and am looking for any follow on steps here. @shwina FYI, I had to change more of the python code and will need another check but I would wait until we settle on the c++ types first, since those are all at the interface level. |
| : metadata_(std::move(md)), gpu_data(std::move(gd)) | ||
| { | ||
| } | ||
|
|
||
| std::unique_ptr<metadata> metadata_; ///< Host-side metadata buffer | ||
| std::unique_ptr<rmm::device_buffer> gpu_data; ///< Device-side data buffer | ||
| std::unique_ptr<std::vector<uint8_t>> metadata_; ///< Host-side metadata buffer |
There was a problem hiding this comment.
| std::unique_ptr<std::vector<uint8_t>> metadata_; ///< Host-side metadata buffer | |
| std::unique_ptr<std::vector<uint8_t>> metadata; ///< Host-side metadata buffer |
There was a problem hiding this comment.
good eye.. will fix
| packed_columns() | ||
| : metadata_(std::make_unique<std::vector<uint8_t>>()), | ||
| gpu_data(std::make_unique<rmm::device_buffer>()) | ||
| { | ||
| } |
There was a problem hiding this comment.
| packed_columns() | |
| : metadata_(std::make_unique<std::vector<uint8_t>>()), | |
| gpu_data(std::make_unique<rmm::device_buffer>()) | |
| { | |
| } | |
| packed_columns() = default; |
There was a problem hiding this comment.
There is code depending on the default constructor to instantiate the unique_ptrs (since we need to keep these for now). If these were members directly of packed_columns I am with you, but as it is I can't remove.
|
@shwina I think this is ready for another python pass if you have a chance. I had to change the cython code again, and I don't trust myself. |
|
I am noticing a JNI gpu leak with this patch applied if I run with OOM injection logic we have added. I am working on identifying what is going on here. |
|
I triaged the JNI leak to: #13225 Essentially the cuDF version I tested without leaks vs. the new one in this PR missed some code that was added that causes creation of column vectors to be unsafe due to exceptions. That is unrelated to this change. |
|
/merge |
|
Thanks all for the reviews! |
This PR moves contiguous_split specific APIs (pack/unpack/metadata/contiguous_split) out of
copying.hppand creates a new headercontiguous_split.hpp.I have built the cpp side, created doxygen docs, and built the python side and ran
pack_test, but I could use more eyes there.I've marked this as breaking because APIs are moving from
copying.hpptocontiguous_split.hpp@nvdbaranec fyi