8000 llama : default pooling last for qwen3 by ngxson · Pull Request #14028 · ggml-org/llama.cpp · GitHub
[go: up one dir, main page]

Skip to content

llama : default pooling last for qwen3 #14028

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

Closed
wants to merge 1 commit into from

Conversation

ngxson
Copy link
Collaborator
@ngxson ngxson commented Jun 5, 2025

Maybe too late to fix upstream model (one of the GGUF already set to public), so I think we can do a quick patch in llama.cpp

CC @yuhao318 do you have any visibility about why only one GGUF is public?

@ngxson ngxson requested a review from ggerganov June 5, 2025 11:48
Comment on lines 822 to +823
ml.get_key(LLM_KV_ATTENTION_LAYERNORM_RMS_EPS, hparams.f_norm_rms_eps);
hparams.pooling_type = LLAMA_POOLING_TYPE_LAST; // for embeddings model
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ml.get_key(LLM_KV_ATTENTION_LAYERNORM_RMS_EPS, hparams.f_norm_rms_eps);
hparams.pooling_type = LLAMA_POOLING_TYPE_LAST; // for embeddings model
hparams.pooling_type = LLAMA_POOLING_TYPE_LAST; // for embeddings model
ml.get_key(LLM_KV_POOLING_TYPE, hparams.pooling_type, false);
ml.get_key(LLM_KV_ATTENTION_LAYERNORM_RMS_EPS, hparams.f_norm_rms_eps);

Copy link
Collaborator
@CISC CISC Jun 5, 2025

Choose a reason for hiding this comment

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

Perhaps use metadata if it exists, and add the pooling check to conversion (do they use 1_Pooling)? Ah, the check is already there, guess they don't.

Copy link
Collaborator Author
@ngxson ngxson Jun 5, 2025

Choose a reason for hiding this comment

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

For rerank model, there is be a KV metadata to switch the pooling to "rank"

So yes, it's better to do as you suggested

@ngxson
Copy link
Collaborator Author
ngxson commented Jun 5, 2025

superseded by the other PR

@ngxson ngxson closed this Jun 5, 2025
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.

2 participants
0