Conversation
507349c to
51f347e
Compare
25c7339 to
f473276
Compare
Merging this PR will not alter performance
|
49e8eec to
c098583
Compare
c098583 to
37fd9e4
Compare
|
codex has some findings that I will have to investigate tomorrow: Details[P1] Lazy TQPack still cannot be used as the write-time path. initialize() only registers the TQUnpack array plugin, and TQPack’s array serialization returns None / deserialization bails, so writing TQPack::try_new_array(...).into_array() will fail before pack_vector runs. The file test currently writes an already-executed TurboQuant array, so it does not cover the actual lazy write path. See lib.rs (line 81), pack.rs (line 158), and file.rs (line 30). [P2] vortex_turboquant::initialize() is not self-contained for lazy TQUnpack file reads. TQUnpack deserialization requires the parent dtype to downcast as AnyVector, but initialize() no longer registers vortex_tensor::vector::Vector. The current file tests mask this by separately calling vortex_tensor::initialize(&session). A session that only calls the TurboQuant initializer can register TQUnpack but still fail to deserialize its Vector parent dtype. See lib.rs (line 76) and unpack.rs (line 167). [P2] The public SORF dimension padding path uses unchecked next_power_of_two(). SorfMatrix::try_new and SorfTransform::return_dtype can panic in debug builds on oversized dimensions instead of returning VortexResult, and validate_sorf_options does not reject the bad dimension before serialized metadata reaches this path. Use checked_next_power_of_two() and reject zero/overflow in validation. See rotation.rs (line 79) and vtable.rs (line 96). |
37fd9e4 to
12b4aa0
Compare
|
Please can we think a little about the reviewer this is a 4k+ PR with a large move and a new feature |
joseph-isaacs
left a comment
There was a problem hiding this comment.
please can this be split up
| } | ||
| } | ||
|
|
||
| pub(super) fn serialize_config(config: &TurboQuantConfig) -> Vec<u8> { |
There was a problem hiding this comment.
Seems a bit weird these aren't on the TQScalarFnMetadata impl? Don't mind that much 🤷 - you could also just inline them to the call site they're so small
There was a problem hiding this comment.
I mean we only use the struct as an intermediate type, these are not really methods
| } | ||
|
|
||
| impl ScalarFnVTable for TQUnpack { | ||
| type Options = TurboQuantConfig; |
There was a problem hiding this comment.
Do you need this? Isn't this entirely encapsulated by the child's ext dtype metadata?
You should model this as e.g. options the user must provide in their SQL query SELECT tq.unpack(..., <options>)
There was a problem hiding this comment.
We do actually need this in order to (lossily) reconstruct the original vectors:
/// Configuration for lossy TurboQuant packing.
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct TurboQuantConfig {
bit_width: u8,
seed: u64,
num_rounds: u8,
}None of this information can be taken from the dtype
robert3005
left a comment
There was a problem hiding this comment.
Can we use prs as atomic units instead of commits, please. Github isn't made for reviewing individual commits
|
@joseph-isaacs how do you suggest I split this PR up? The best alternative here that I can think of is that I split this into a) copy and paste a bunch of code from vortex-tensor, b) clean up, and c) add the scalar functions, which is frankly a waste of time if we know most of the code is going to change. This is all due to the fact that we decided to change the design quite significantly but the implementation is still mostly the same. |
|
I will clean up my git history and we can figure out if it makes sense to split changes out of this or if everything together is required for context for reviewing |
edcebd9 to
2cf49f7
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
2cf49f7 to
3c3836f
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
0ae8703 to
11ac1c0
Compare
its done now
Summary
Tracking issue: #7830
Moves TurboQuant out of
vortex-tensorinto a newvortex-turboquantcrate.The first commit was mostly copying and pasting a bunch of code, as well as adding the
unpackmethod to replace canonicalization. The second commit was cleaning up everything holistically.A lot of the code in
vortex-tensorwas reviewed pretty lightly because we knew that it was unstable, but now that we are more certain about the implementation (not necessarily about the exact design, but the actual implementation of the TQ algorithms), I think it is worth reviewing everything as a whole.Testing
These tests were mostly there before, but now there are more!