Make quantize_ respect the usages of the quantizer #1836
Conversation
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
for more information, see https://pre-commit.ci
|
/te-ci pytorch |
| const std::vector<size_t>& shape, DType dtype, const py::object& output = py::none(), | ||
| std::optional<at::Tensor> rowwise_data = std::nullopt) const = 0; |
There was a problem hiding this comment.
Somewhat orthogonal, but since we're touching Quantizer::create_tensor, we should consider removing the rowwise_data arg. It was a UB-specific option that doesn't really make sense anymore. I believe all usages have been refactored away.
There was a problem hiding this comment.
ok, cool. I will do that - it will make the code nicer.
There was a problem hiding this comment.
Actually can't do that just yet. Attention also uses this unfortunately.
Signed-off-by: Przemek Tredak <[email protected]>
for more information, see https://pre-commit.ci
|
/te-ci pytorch |
Signed-off-by: Przemek Tredak <[email protected]>
|
/te-ci pytorch |
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
for more information, see https://pre-commit.ci
|
/te-ci pytorch |
Signed-off-by: Przemek Tredak <[email protected]>
|
/te-ci pytorch |
| if (output_tensor_type == PythonTensorType::TENSOR_BASE) { | ||
| output.attr("_quantizer") = this->quantizer; | ||
| } else { | ||
| output.attr("_quantizer") = python_copy(this->quantizer); |
There was a problem hiding this comment.
Why do we use python_copy(this->quantizer) for:
- output is not None (reused)
- construction of quantized tensor class (non-base)
But for the base class, we choose to use this->quantizer instead of python_copy(this->quantizer).
Have we analyzed CPU overhead of this copy?
It looks okay to me since at least the Base class doesn't require python-copy (since in my #1793, I enforced the creation of base class and I even avoided using _a in the constructor to reduce overhead by 2x), but still curious to know why python-copy is needed.
Signed-off-by: Przemek Tredak <[email protected]>
for more information, see https://pre-commit.ci
|
/te-ci pytorch |
Signed-off-by: Przemek Tredak <[email protected]>
|
/te-ci pytorch |
Signed-off-by: Przemek Tredak <[email protected]>
Signed-off-by: Przemek Tredak <[email protected]>
for more information, see https://pre-commit.ci
Description
This PR changes the create_tensor function to accept an optional
outparameter - a tensor that can be reused. It also changes the tex.quantize function to always try to recreate the tensor (potentially reusing the providedout) rather than blindly believing thatoutis already prepared properly. This ensures that the quantizer settings are going to be preserved and enables us to drop the workaround inget_weight_workspacesto ignore the cached MXFP8 weight tensor when the usages are incompatible. See #1593 (comment) for more details.Adding @guyueh1
Type of change
Changes
Please list the changes introduced in this PR:
Checklist: