Repository navigation
ci: relax DyT tol, skip MTA test for xpu, unify logit logprobs tol - #778
Conversation
|
@Tcc0403 thanks for fixing, although I see it says do not merge for now. Are there more changes needed? |
|
Trying to square with #778, although I'd prefer not commenting out the tests if possible |
|
@shimizust I was trying to make convergence tests passed as well but didn't have time. I'll try making it ready today. |
8f06b89 to
21fa8ca
Compare
Oops you mean #775 right? My bad, I didn't see that before creating this PR.
+1. Since we didn't change tolerance for logits when replacing them with logrpobs, I think some of them become too strict for logrpobs. |
|
@shimizust Instead, I propose a multi-stage tolerance testing:
I've made a simplest implementation of the proposal and applied it to For the second usage I mentioned, we can redirect output in the future. |
|
Hey @Tcc0403 thanks for the proposal on making the test tolerances more systematic. The problem I see is that the tolerance values I don't think would be expected to have a universal standard and should be expected to be a function of model architecture (e.g. MoE, multimodal, activation function), random initialized weights, the mini model config used for testing, etc.. We can set some initial buckets, but it's still arbitrary, and the addition of a new model type may necessitate creating another bucket or redefining the low/med/high tolerances so tests pass. Another potential issue is that it may obscure real regressions, like if a model was initially in the high tolerance category, but a bug caused it to move to low tolerance, it wouldn't be immediately obvious. Thus, I think each model requires tracking its own tolerance, with some reasons added for why a model needs higher tolerance. Another approach is to empirically find an acceptable tolerance for each model by running multiple experiments with different random seeds, and measuring the natural range of variation (atol/rtol) across N runs of the original model. For now, I think it would be reasonable to proceed by tuning tolerances incrementally to unblock the CI, while figuring out a more reasoned approach in the meantime. What do you think? @lancerts did you have any thoughts? |
|
Thanks for the feedback.
I see, model architecture, initial weights, the config may have a huge impact on numerical errors as well, so universal tolerances are only viable if we can find the settings that can generate similar results, i.e. in the same floating range, for each models. I can see that the model architecture would affect the most. If we are going to set a unviersal standard, we need to find the tolerance for different model architecture at the very least.
My idea is to show the category for each models on our future regression tracking site one day, so moving between categories can be captured (maybe make a dedicated benchmarking for this purpose). Since tolerances are also represented as the confidence level of our patching, if a small bug causes the model dropping into the lower bracket, it is still acceptable. We can always keep track of it and decide whether we should start investigating or put it off. Plus, if we set the tolerance too high, such small bugs are more difficult to be detected.
It might work but it also increases the burden of supporting new models for contributors.
Gotcha. I'll go with the original method and unblock the ci first. |
02a1b00 to
c566406
Compare
|
Some |
|
@shimizust I found that the casting in GEGLUMLP computation can result in huge discrepancy in bf16 scenario. See #783. (Update: SwiGLUMLP as well) We can merge that PR first. The change of gelu kernel is included in this PR just to help fixing ci. Before the fixWe can see there's a huge gap between two models. Some statistics:max values at the mismatch row differ, resulting a large discrepancy after logsoftmax computation # original model
(expected_output['logits'][0, 1015, :]).max()=tensor(10.2500, device='cuda:0', grad_fn=<MaxBackward1>)
(expected_output['topk_logprobs'][0, 1015, :])=tensor([-1.2276, -6.9463, -6.9463, -6.9776, -7.0713, -7.0713, -7.1338, -7.1338,
-7.1651, -7.1963, -7.2588, -7.3213, -7.3838, -7.4151, -7.4151, -7.4151,
-7.4463, -7.4463, -7.4463, -7.4463], device='cuda:0')
# patched model
(actual_output['logits'][0, 1015, :]).max()=tensor(9.8125, device='cuda:0', grad_fn=<MaxBackward1>)
actual_output['topk_logprobs'][0, 1015, :]=tensor([-1.5070, -6.7570, -6.8820, -6.8820, -6.9445, -6.9445, -7.0070, -7.0070,
-7.1008, -7.1320, -7.1320, -7.1633, -7.1945, -7.2258, -7.2570, -7.2883,
-7.3508, -7.3508, -7.3508, -7.3508], device='cuda:0')After the fixSucessfully passed the test Some statisticsmax values are now the same, resulting in closer outputs after logsoftmax computation # original model
(expected_output['logits'][0, 1015, :]).max()=tensor(10.2500, device='cuda:0', grad_fn=<MaxBackward1>)
(expected_output['topk_logprobs'][0, 1015, :])=tensor([-1.2276, -6.9463, -6.9463, -6.9776, -7.0713, -7.0713, -7.1338, -7.1338,
-7.1651, -7.1963, -7.2588, -7.3213, -7.3838, -7.4151, -7.4151, -7.4151,
-7.4463, -7.4463, -7.4463, -7.4463], device='cuda:0')
# patched model
(actual_output['logits'][0, 1015, :]).max()=tensor(10.2500, device='cuda:0', grad_fn=<MaxBackward1>)
actual_output['topk_logprobs'][0, 1015, :]=tensor([-1.2278, -6.8840, -6.9778, -7.0090, -7.1028, -7.1028, -7.1028, -7.1340,
-7.1653, -7.1965, -7.2278, -7.3215, -7.3840, -7.3840, -7.4153, -7.4465,
-7.4465, -7.4465, -7.4465, -7.4778], device='cuda:0') |
checkstyle Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com> fix typo Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com> Use `extra_info` in testings Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com> Disable llama4 fp32 multimodal test since dtype mismatch Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com> Remove redundant llama4 config Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com> Remove mistral comment Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com> Add silu fix Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
415580a to
cafc4a4
Compare
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
759f3c5 to
9b35fd4
Compare
Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com> Add reason Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com> format Signed-off-by: Tcc0403 <76503978+Tcc0403@users.noreply.github.com>
9b35fd4 to
de6b358
Compare
|
@shimizust Ready for review! |
shimizust
left a comment
There was a problem hiding this comment.
Thanks so much for getting the ci issues resolved!
Summary
Fix #761, #742.
This PR found and addressed couple of issues
skip_logits, causing failures for bwd-test ci.skip_logitstolce_forward_deprecated()s.bfloat16, couldn't work withfloat32model.Minor change:
assert_verbose_allclose()to show addional information at the start of the error message.Testing Done
make testto ensure correctnessmake checkstyleto ensure code stylemake test-convergenceto ensure convergence