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

llama : return nullptr from llama_grammar_init #8093

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

danbev
Copy link
Collaborator

@danbev danbev commented Jun 24, 2024

This commit updates llama_grammar_init to return nullptr instead of throwing an exception.

The motivation for this is that this function is declared inside an extern "C" block and is intended/may be used from C code which will not be able to handle exceptions thrown, and results in undefined behavior.

On Windows and using MSVC the following warning is currently generated:

C:\llama.cpp\llama.cpp(13998,1): warning C4297: 'llama_grammar_init': function assumed not to throw an exception but does
C:\llama.cpp\llama.cpp(13998,1): message :__declspec(nothrow), throw(), noexcept(true), or noexcept was specified on the function

This commit updates llama_grammar_init to return nullptr instead of
throwing an exception.

The motivation for this is that this function is declared inside an
extern "C" block and is intended/may be used from C code which will not
be able to handle exceptions thrown, and results in undefined behavior.

On Windows and using MSVC the following warning is currently generated:
```console
C:\llama.cpp\llama.cpp(13998,1): warning C4297: 'llama_grammar_init':
function assumed not to throw an exception but does
C:\llama.cpp\llama.cpp(13998,1): message :
__declspec(nothrow), throw(), noexcept(true), or noexcept was specified
on the function
```

Signed-off-by: Daniel Bevenius <[email protected]>
@mofosyne mofosyne added the Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix label Jun 24, 2024
@github-actions github-actions bot added the testing Everything test related label Jun 24, 2024
@HanClinto
Copy link
Collaborator

Overall this is a good change, but I'm curious how this changes the stack trace returned with left-recursive grammars. I think we should do an audit for everything that calls llama_grammar_init() to ensure that we are checking for null ptrs. Examples include sampling.cpp:31 and (less-importantly) gbnf-validator.cpp:101.

Copy link
Collaborator

@HanClinto HanClinto left a comment

Choose a reason for hiding this comment

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

Overall I think this is a good change, though there may be some cleanup spots to do later.

HanClinto and others added 2 commits June 24, 2024 22:58
Add checks for nullptr when calling llama_grammar_init.

Signed-off-by: Daniel Bevenius <[email protected]>
@danbev
Copy link
Collaborator Author

danbev commented Jun 25, 2024

I think we should do an audit for everything that calls llama_grammar_init() to ensure that we are checking for null ptrs.

Good point, I'll take a look at this! Thanks

@HanClinto
Copy link
Collaborator

I think we should do an audit for everything that calls llama_grammar_init() to ensure that we are checking for null ptrs.

Good point, I'll take a look at this! Thanks

Did you resolve everything you wanted to in 35d9680 or should we wait for more? If you're happy, I think you're good to click "Squash and Merge".

@danbev
Copy link
Collaborator Author

danbev commented Jun 25, 2024

Did you resolve everything you wanted to in 35d9680 or should we wait for more? If you're happy, I think you're good to click "Squash and Merge".

All good from my side 👍 I can rebase and squash the commits to make it easier to merge, but I don't I have write access so I don't think I can merge.

@HanClinto
Copy link
Collaborator

All good from my side 👍 I can rebase and squash the commits to make it easier to merge, but I don't I have write access so I don't think I can merge.

Aah, right. Thanks for the PR!

@HanClinto HanClinto merged commit e6bf007 into ggml-org:master Jun 25, 2024
63 checks passed
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Jun 30, 2024
* llama : return nullptr from llama_grammar_init

This commit updates llama_grammar_init to return nullptr instead of
throwing an exception.

The motivation for this is that this function is declared inside an
extern "C" block and is intended/may be used from C code which will not
be able to handle exceptions thrown, and results in undefined behavior.

On Windows and using MSVC the following warning is currently generated:
```console
C:\llama.cpp\llama.cpp(13998,1): warning C4297: 'llama_grammar_init':
function assumed not to throw an exception but does
C:\llama.cpp\llama.cpp(13998,1): message :
__declspec(nothrow), throw(), noexcept(true), or noexcept was specified
on the function
```

Signed-off-by: Daniel Bevenius <[email protected]>

* squash! llama : return nullptr from llama_grammar_init

Add checks for nullptr when calling llama_grammar_init.

Signed-off-by: Daniel Bevenius <[email protected]>

---------

Signed-off-by: Daniel Bevenius <[email protected]>
Co-authored-by: Clint Herron <[email protected]>
MagnusS0 pushed a commit to MagnusS0/llama.cpp-normistral-tokenizer that referenced this pull request Jul 1, 2024
* llama : return nullptr from llama_grammar_init

This commit updates llama_grammar_init to return nullptr instead of
throwing an exception.

The motivation for this is that this function is declared inside an
extern "C" block and is intended/may be used from C code which will not
be able to handle exceptions thrown, and results in undefined behavior.

On Windows and using MSVC the following warning is currently generated:
```console
C:\llama.cpp\llama.cpp(13998,1): warning C4297: 'llama_grammar_init':
function assumed not to throw an exception but does
C:\llama.cpp\llama.cpp(13998,1): message :
__declspec(nothrow), throw(), noexcept(true), or noexcept was specified
on the function
```

Signed-off-by: Daniel Bevenius <[email protected]>

* squash! llama : return nullptr from llama_grammar_init

Add checks for nullptr when calling llama_grammar_init.

Signed-off-by: Daniel Bevenius <[email protected]>

---------

Signed-off-by: Daniel Bevenius <[email protected]>
Co-authored-by: Clint Herron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants