[Bugfix] Update expected shape for per token strategy

[kylesayrs]

Background

• While implementing Accelerate Utilities #193, a bug was discovered where the shape of per-token scales is being initialized with the incorrect shape

Changes

• Change shape of initialized quantization parameters in per token case
• Unrelated, compare tuples rather than lists in tests/test_quantization/test_configs/test_strategies.py

Testing

• Added new tests in tests/test_quantization/lifecycle/test_initialize.py

[dsikka]

Where are you seeing the failures?

The only relevant case is dynamic per token, which shouldn't be initializing anything as the scales are determined on the fly - so I dont think this change is required.

[kylesayrs]

@dsikka There is a per-token non-dynamic test case in the tests.

I discovered this bug while implementing #193, which uses copy_ rather than out-of-place assignment when updating quantization parameters. Because copy_ requires the original shape (which used to be (1, )) to be the same as the newly updating shape (which is (1, 1)), an error was thrown.

In general, the initialized shape should be the same as the shape computed by q_params(). While I agree this bug only appears in special cases and that this change isn't necessary outside of the context of #193, I believe that it is correct.

[dsikka]

@dsikka There is a per-token non-dynamic test case in the tests.

I discovered this bug while implementing #193, which uses copy_ rather than out-of-place assignment when updating quantization parameters. Because copy_ requires the original shape (which used to be (1, )) to be the same as the newly updating shape (which is (1, 1)), an error was thrown.

In general, the initialized shape should be the same as the shape computed by q_params(). While I agree this bug only appears in special cases and that this change isn't necessary outside of the context of #193, I believe that it is correct.

Yeah I agree that the shape is correct.
What I'm thinking about now is when is the shape ever just 1. I guess in the static per tensor case but we actually update this in vLLM to be (1, 1) anyway (have to confirm if this is still the case) so we could make this the default potentially

[kylesayrs]

@dsikka From my reading, itlooks like it's just (1, ) https://github.com/vllm-project/vllm/blob/main/vllm/model_executor/layers/quantization/compressed_tensors/schemes/compressed_tensors_w8a8_int8.py#L121-L124

This is my understanding of the different expected shapes

Strategy	Scale Shape	ZP Shape
Tensor	(1, )	(1, )
Channel	(out_dims, 1)	(out_dims, 1)
Group	(in_dims, out_dims // group_size)	(in_dims, out_dims // group_size)
Block	(1, )	(1, )
Token	(1, 1)	(1, 1)

I haven't explored block and token quantization much, but the (1, ) shape for block quantization seems suspicious to me. Maybe @rahul-tuli you might be familiar?

[horheynm]

@dsikka From my reading, itlooks like it's just (1, ) https://github.com/vllm-project/vllm/blob/main/vllm/model_executor/layers/quantization/compressed_tensors/schemes/compressed_tensors_w8a8_int8.py#L121-L124

This is my understanding of the different expected shapes

Strategy Scale Shape ZP Shape
Tensor (1, ) (1, )
Channel (out_dims, 1) (out_dims, 1)
Group (in_dims, out_dims // group_size) (in_dims, out_dims // group_size)
Block (1, ) (1, )
Token (1, 1) (1, 1)
I haven't explored block and token quantization much, but the (1, ) shape for block quantization seems suspicious to me. Maybe @rahul-tuli you might be familiar?

This is helpful, maybe should include in docs somewhere. Hard to navigate to here

[horheynm]

general question - for output activation we dont need?

[kylesayrs]

We could test output activation, but I decided not to for test simplicity

[dsikka]

I would also make sure we're mocking correctly since the test case you pointed out applies the config to initialize scales/zp (which would be impacted by your change) but the mock doesn't seem to care about the shape
https://github.com/neuralmagic/compressed-tensors/blob/main/tests/conftest.py