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 for quantize fail on init due to undefined static initialization of complex objects #927

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

arikpoz
Copy link
Contributor

@arikpoz arikpoz commented Apr 12, 2023

Addresses issue #920

Replaced static initialization of complex objects with a initialization on first use. This prevents an undefined behavior on program run, for example, crash in Release build, works in Debug build

…on on first use. This prevents an undefined behavior on program run, for example, crash in Release build, works in Debug build
@jayliang701
Copy link

@arikpoz this MR still can not fix the issue.

image

@ggerganov
Copy link
Owner

Hmm, how is this UB - on which compiler / platform?

@arikpoz
Copy link
Contributor Author

arikpoz commented Apr 13, 2023

I was able to reproduce it and fix it on Windows 10 with MSVC compiler, where the exception came from the static initialization of the maps. It could be the original reported issue was a different exception

@arikpoz
Copy link
Contributor Author

arikpoz commented Apr 13, 2023

@jayliang701 , do you get the same behavior if you compile in Debug vs. Release?
If it works in Debug and fails on Release, try to compile in Release with Debug symbols, and get a more accurate location of the exception

@jayliang701
Copy link

jayliang701 commented Apr 13, 2023

@jayliang701 , do you get the same behavior if you compile in Debug vs. Release? If it works in Debug and fails on Release, try to compile in Release with Debug symbols, and get a more accurate location of the exception

still using Macbook Air M1 (2020), Memory 16G

yes. I have tried Debug (cmake --build . --config Debug, is it correct?) & Release using this MR, both have same issue like before. And main command also returns zsh: killed error (I download a ggml q4 model from HuggingFace)

@arikpoz
Copy link
Contributor Author

arikpoz commented Apr 14, 2023

@ggerganov , after fixed AVX flag mentioned in PR #809 , the original crash on static initialization I had with quantize disappeared. That said, I believe this is random behavior of the compiler and we should keep the changes suggested in this PR, since when you have static initialization of complex objects that uses other modules (std::) there is no guaranteed order for running the static initializers across the different modules, so the behavior is undefined. See https://en.cppreference.com/w/cpp/language/siof for reference

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

I really don't think these static maps can ever cause initialization order problems (or any other problems) as they are used only in the current translation unit.

But anyway, it does not hurt to change it as suggested.
Just make the functions static and return const references

llama.cpp Outdated Show resolved Hide resolved
@arikpoz
Copy link
Contributor Author

arikpoz commented Apr 15, 2023

Updated code as requested

@arikpoz arikpoz requested a review from ggerganov April 16, 2023 22:41
@arikpoz
Copy link
Contributor Author

arikpoz commented Apr 17, 2023

@ggerganov , let me know if something is missing to merge this

@ggerganov ggerganov merged commit efd0564 into ggerganov:master Apr 17, 2023
@sw sw mentioned this pull request Apr 22, 2023
Deadsg pushed a commit to Deadsg/llama.cpp that referenced this pull request Dec 19, 2023
* Add MistralLite format

* Update llama_chat_format.py

* Update llama_chat_format.py
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.

None yet

3 participants