Skip to content
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

Fix uninitialized variable in quantized compressors #205

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

markmc
Copy link
Contributor

@markmc markmc commented Nov 13, 2024

Both compressors have a can_quantize() check, which if ever doesn't succeed would trigger:

UnboundLocalError: cannot access local variable 'quantized_weight' where it is not associated with a value

Add the obvious fix for this and highly artificial test cases that would trigger it.

Looks like this error was introduced by #109

Definitely begs the question whether this code path has any real world use!

@kylesayrs kylesayrs self-assigned this Nov 19, 2024
Copy link
Contributor

@kylesayrs kylesayrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution @markmc! Indeed you made a good catch, these variables are uninitialized in some cases.

Thank you for including test cases as well. The tests are useful to illustrate when this error occurs, but I agree with you in that they are highly artificial. Since they are so artificial, I think we should avoid merging the test cases into main, but keep your bug fixes under src.

Once those changes have been made, I'd be happy to help get this merged in. Thanks again! 🙂

Both compressors have a can_quantize() check, which if ever doesn't
succeed would trigger:

> UnboundLocalError: cannot access local variable 'quantized_weight' where it is not associated with a value
@markmc
Copy link
Contributor Author

markmc commented Nov 19, 2024

Thank you for including test cases as well. The tests are useful to illustrate when this error occurs, but I agree with you in that they are highly artificial. Since they are so artificial, I think we should avoid merging the test cases into main, but keep your bug fixes under src.

No problem, I've dropped the tests 👍

@kylesayrs kylesayrs merged commit 525ef3a into neuralmagic:main Nov 26, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants