Skip to content

Use CString::new instead of CString::from_str to init chat template #683

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

Merged

Conversation

AsbjornOlling
Copy link
Contributor

Not sure what happened here but:

cargo build fails for me on the current main branch. It seems like CString doesn't implement from_str()

error[E0599]: no function or associated item named `from_str` found for struct `CString` in the current scope
   --> llama-cpp-2/src/model.rs:50:26
    |
50  |         Ok(Self(CString::from_str(template)?))
    |                          ^^^^^^^^ function or associated item not found in `CString`
    |

This PR just changes it to CString::new() which seems to have the same type signature and behavior.

I guess it could be a thing about rust std versions?

My rustup show says the following, so it doesn't seem like I'm running anything unusual.

Default host: x86_64-unknown-linux-gnu
rustup home:  /home/asbjorn/.rustup

installed toolchains
--------------------

stable-x86_64-unknown-linux-gnu (default)
nightly-x86_64-unknown-linux-gnu

installed targets for active toolchain
--------------------------------------

aarch64-linux-android
x86_64-pc-windows-gnu
x86_64-unknown-linux-gnu

active toolchain
----------------

stable-x86_64-unknown-linux-gnu (default)
rustc 1.85.0 (4d91de4e4 2025-02-17)

I can't really explain why this works in your CI, but not on my machine- but it seems like a pretty uncontroversial change.

@AsbjornOlling AsbjornOlling changed the title Use CString::new instead of Cstring::from_str to init chat template Use CString::new instead of CString::from_str to init chat template Feb 25, 2025
@vlovich
Copy link
Contributor

vlovich commented Feb 25, 2025

That seems really really strange. You might want to see if you can write a reduced test case as it seem like it would be some kind of bug with Rust stdlib. The change looks fine to me - those two functions are aliases for each other.

@MarcusDunn MarcusDunn merged commit 045c5f2 into utilityai:main Feb 25, 2025
@AsbjornOlling
Copy link
Contributor Author

Something must have been wrong with my environment. I can't have been running rustc 1.85.0.

I've tested more closely with specific versions, and found that CString::from_str works in 1.84.0 and later.

It's pretty clearly visible in the changelog as well: https://releases.rs/docs/1.84.0/

Anyway.. this PR just makes llama-cpp-rs work with rustc versions older than January 9th of this year. A small improvement, but an improvement regardless 😅

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