-
Notifications
You must be signed in to change notification settings - Fork 58
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
Use nvcomp defaults for algo options. #450
Conversation
@madsbk @Alexey-Kamenev @thomcom could you please look over these changes when you have a moment? 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semantically the change looks good to me, not qualified to evaluate the (Python) code quality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After Alexey observed the f-string
typo. Saw a few more. Flagged them below
As it appears we do tests constructing compressors with default arguments, this should already be covered in the existing test suite kvikio/python/kvikio/tests/test_nvcomp.py Lines 195 to 217 in 3ce8bcc
Though please let me know if I've missed anything here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I only have some type hint suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking nits
|
||
def _get_comp_temp_size( | ||
self, | ||
size_t batch_size, | ||
size_t max_uncompressed_chunk_bytes, | ||
) -> (nvcompStatus_t, size_t): | ||
) -> tuple[nvcompStatus_t, size_t]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, how did this ever work? I guess cython is less strict with parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are valid, the type format we had before is called a ctuple
and has been around in Cython longer
Support for typing.Tuple
is relatively new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cython already checks whether the value is an int
or None
based on the function signature. So the exceptions can be dropped. If we wanted to be extra careful, we could add an assert
. Though likely that is unneeded
Example of this behavior demonstrated below:
In [1]: %load_ext cython
In [2]: %%cython
...:
...: from typing import Optional
...:
...: class Class:
...: def __init__(self, v: Optional[int] = None):
...: print(f"v = {v}")
...: self.v = v
...:
In [3]: Class()
v = None
Out[3]: <_cython_magic_96fdfa8e6c04d6dbb7c0c7398f039f6decfcbe5b.Class at 0x10524ad10>
In [4]: Class(1)
v = 1
Out[4]: <_cython_magic_96fdfa8e6c04d6dbb7c0c7398f039f6decfcbe5b.Class at 0x104f4c610>
In [5]: Class(3.14)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[5], line 1
----> 1 Class(3.14)
TypeError: Argument 'v' has incorrect type (expected int, got float)
Co-authored-by: jakirkham <jakirkham@gmail.com>
Thanks everyone for the reviews! I'll wait another work day to make sure folks are happy with the way the comments like #450 (comment) have been resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reviews and fixes. I am happy with this.
/merge |
Update the nvCOMP version used for compression/decompression to 4.0.1. See also: rapidsai/rapids-cmake#633 rapidsai/cudf#16076 Depends on #450. Authors: - Vukasin Milovanovic (https://github.com/vuule) - Bradley Dice (https://github.com/bdice) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - Bradley Dice (https://github.com/bdice) URL: #449
This PR updates the nvcomp bindings to use nvcomp's defaults rather than hardcode the default options. The defaults changed in nvcomp 4.0.1, so this is needed for #449.