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

llguidance build fixes for Windows #11664

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mmoskal
Copy link
Contributor

@mmoskal mmoskal commented Feb 4, 2025

The actual changes is by @phil-scott-78 , see comment

I tested it on fresh Windows install, did minor naming fixes, and updated docs.

Also fixed link to VS download to be English not German.

cc @HanClinto

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 4, 2025
@phil-scott-78
Copy link

i was hoping to dig a bit more into the code this afternoon, but I've ran out of time before my kids come home from school. But it looks like llama_sampler_init_llg isn't exported either. Not sure if that's on purpose until the support isn't behind a flag. I'm using LlamaSharp and they build the samplers up by hand rather than calling common_sampler_init.

@mmoskal
Copy link
Contributor Author

mmoskal commented Feb 4, 2025

I suspect LlamaSharp is only linking against llama.dll ? The llguidance is currently in common.lib

@phil-scott-78
Copy link

Correct, it only takes a dependency on llama.dll that's shipped with the releases and calls that via .NET's DllImports to do the unmanaged calls to exported functions. So it would need to be exported.

I could see an argument that if the build was done with LLGuidance flag set then the existing llama_sampler_init_grammar and llama_sampler_init_grammar_lazy should be able to handle routing the grammar without the need for additional calls exported. much like the set up with common_sampler_init is handling it now, so maybe that's the design decision instead.

@mmoskal
Copy link
Contributor Author

mmoskal commented Feb 4, 2025

It was ggerganov decision to include the llguidance only in common.lib, which is not AFAIK included in llama.dll. So the code is not there, it's not only about exporting. I understand he's open to including it in llama.dll at a later date after it proves itself.

@phil-scott-78
Copy link

Ok, that makes sense. Then no worries, I think I can come up with some creative solutions until it's properly baked now that it's building on Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants