Skip to content

lora: make sure model keep track of associated adapters #18490

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.

Sign up for GitHub

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

Jump to bottom
Merged
ngxson merged 4 commits into from
Jan 15, 2026
Merged

lora: make sure model keep track of associated adapters #18490

ngxson merged 4 commits into from
Jan 15, 2026

Conversation

@ngxson
Copy link
Collaborator

@ngxson ngxson commented Dec 30, 2025

Ref: #18469 (comment)

I'm extracting this change to a dedicated PR for visibility

ggerganov
ggerganov reviewed Dec 30, 2025
View reviewed changes
Copy link
Member

@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.

If we delegate it to the llama_model to free the adapters, then we should remove the llama_adapter_lora_free() from the public API.

For example, with the current change I believe we will be double freeing the adapters - once on model destruction and once more here for all llama_adapter_lora_ptr instances:

llama.cpp/include/llama-cpp.h

Lines 23 to 26 in cd78e57

struct llama_adapter_lora_deleter {
void operator()(llama_adapter_lora * adapter) { llama_adapter_lora_free(adapter); }
};

@loci-dev loci-dev mentioned this pull request Dec 30, 2025
Open
@ngxson
Copy link
Collaborator Author

ngxson commented Dec 31, 2025

Yes that makes sense. llama_adapter_lora_free exists because we wanted to allow hot-loading / hot-removing of lora adapter. But I don't think it's actually used that way in reality.

By removing llama_adapter_lora_free, we will now assume that lora adapters are an "extension" to a model, and must be loaded before the model can be used (i.e. context creation from a model)

@ngxson ngxson requested a review from ggerganov December 31, 2025 11:07
@ggerganov
Copy link
Member

ggerganov commented Jan 15, 2026

@ngxson This is probably good to merge?

@ngxson
Copy link
Collaborator Author

ngxson commented Jan 15, 2026

Ah yes, thanks. Merging it now

@ngxson ngxson merged commit a7e6ddb into master Jan 15, 2026
79 of 80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggerganov ggerganov ggerganov approved these changes

@CISC CISC Awaiting requested review from CISC CISC is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ngxson @ggerganov