FEAT support any causal LM in GCG embedding helpers - #2501
Open
devangpratap wants to merge 1 commit into
Open
Conversation
get_embedding_layer, get_embedding_matrix and get_embeddings dispatched through an isinstance chain over six hard-coded architectures and raised ValueError for anything else. GCG loads models with AutoModelForCausalLM, so a model could load successfully and then fail at the gradient step. All three now go through PreTrainedModel.get_input_embeddings, which every causal LM implements. Verified on tiny random configs that the previous hard-coded paths return the identical object for gpt2, gptj, gpt_neox, llama, mistral, mixtral and phi3, so this is not a behaviour change for already-supported models, while qwen3, gemma3, olmo2, starcoder2 and mpt now work instead of raising. get_embeddings keeps returning float16 for GPT-2, GPT-J and GPT-NeoX, matching the previous .half() calls, so the dtype asymmetry between those and the other architectures is preserved rather than silently changed. Replaces the three unknown-model ValueError tests with parametrized coverage over nine architectures asserting the resolved layer and matrix identity plus the dtype contract on both sides.
Author
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2489 (embedding adapters portion).
Problem
GCG loads models with
AutoModelForCausalLM, butget_embedding_layer,get_embedding_matrixandget_embeddingseach dispatched through anisinstancechain over six hard-coded architectures and ended inraise ValueError(f"Unknown model type: {type(model)}"). A newly supported Hugging Face causal model therefore loads successfully and then fails later, at the gradient step.Reproduced on
mainwith tiny random configs, no weights or GPU:Every one of them implements
PreTrainedModel.get_input_embeddings(), so what GCG needs was already reachable through the public interface.Change
All three helpers now resolve through
model.get_input_embeddings().Before collapsing the branches I checked that they were actually equivalent rather than assuming it. For each previously hard-coded architecture, the old expression returns the identical object to the universal accessor:
So this is not a behaviour change for models that already worked.
On the
.half()asymmetry.get_embeddingscalled.half()for GPT-2, GPT-J and GPT-NeoX but not for Llama, Mistral, Mixtral or Phi-3. I raised this on the issue and have deliberately preserved it exactly rather than folding a dtype change into a compatibility fix. If that asymmetry is incidental rather than intended, I am happy to remove it in a separate PR where it can be reviewed on its own merits.The now-unused imports for
LlamaForCausalLM,MistralForCausalLM,MixtralForCausalLMandPhi3ForCausalLMare dropped. The GPT-2, GPT-J and GPT-NeoX imports stay, since the dtype branch still needs them.Tests
The three
test_*_raises_for_unknown_modeltests asserted the behaviour this PR intentionally removes, so they are replaced rather than left to fail. New parametrized coverage over nine architectures builds tiny random models on CPU and asserts:get_embedding_layer(model) is model.get_input_embeddings()get_embedding_matrix(model) is model.get_input_embeddings().weightget_embeddingsreturns the model's hidden sizePre-commit clean on both changed files, including
ruff format,ruff checkandty.Not in this PR
The issue also asks for a tokenizer preflight check and a maintained compatibility matrix. I have left those out so this stays reviewable on its own, and I am happy to follow up with them once the direction here is agreed.