Add support for nvCOMP batch API #249
Conversation
|
Pull requests from external contributors require approval from a |
jakirkham
added
improvement
|
/ok to test |
There was a problem hiding this comment.
Nice work @Alexey-Kamenev !
Have some minor suggestion for my first review pass.
python/kvikio/nvcomp_codec.py
Outdated
| """ | ||
| return self.encode_batch([buf])[0] | ||
|
|
||
| def encode_batch(self, bufs): |
There was a problem hiding this comment.
| def encode_batch(self, bufs): | |
| def encode_batch(self, bufs : List[Any]) -> List[Any]: |
There was a problem hiding this comment.
Done. I did not add type hints since numcodecs Codec does not use them, so I decided to do the same (but I still prefer to use type hints).
There was a problem hiding this comment.
numcodecs Codec does not use them
Could you please raise an upstream issue?
| max_chunk_size, | ||
| num_chunks, | ||
| temp_buf, | ||
| comp_chunks, |
There was a problem hiding this comment.
I don't think comp_chunks is used afterwards, I guess it should be part of the returned result?
There was a problem hiding this comment.
That's correct - comp_chunks is used only as a container that stores pointers to actual chunks. nvCOMP requires this container to be on GPU as well i.e. it's a pointer to pointers and it has to be in GPU memory, same as actual chunk pointers. Once compress returns, this container is not needed/used anymore.
There was a problem hiding this comment.
Ahh got it, could you add some comments describing the nature of comp_chunks and comp_chunks_header in more detail?
There was a problem hiding this comment.
Done - also added similar comments to decode_batch.
* Addressed review feedback. * Added sample Jupyter notebook.
|
It looks like there are 3 pipeline failures for this PR but I don't think they are related to the PR itself, since the errors look like this: |
|
/ok to test |
| max_chunk_size, | ||
| num_chunks, | ||
| temp_buf, | ||
| comp_chunks, |
There was a problem hiding this comment.
Ahh got it, could you add some comments describing the nature of comp_chunks and comp_chunks_header in more detail?
|
/ok to test |
|
/merge |
See #248 for more details.