-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
transformers.fx.symbolic_trace supports inputs_embeds #31574
transformers.fx.symbolic_trace supports inputs_embeds #31574
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working in this fix!
I think we should be a bit smarter about the input preparation here
tests/test_modeling_common.py
Outdated
if "inputs_embeds" in inspect.signature(model.forward).parameters: | ||
inputs_to_test.append( | ||
{ | ||
"inputs_embeds": torch.rand( | ||
3, 5, model.config.hidden_size, dtype=torch.float, device=torch_device | ||
) | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be handled by _prepare_for_class
? Especially as it would avoid this hard-coded 3, 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to, but inputs_embeds
does not seem to be tested (AFAIK it is not in general in prepare_config_and_inputs_for_common
). Do you suggest to edit all the tests/models/**/test_modeling_*.py
to get inputs_embeds
?
@@ -1327,16 +1328,30 @@ def _create_and_check_torch_fx_tracing(self, config, inputs_dict, output_loss=Fa | |||
(past_mask, inputs_to_test[1]["attention_mask"]), dim=1 | |||
) | |||
|
|||
if "inputs_embeds" in inspect.signature(model.forward).parameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this clash with input_ids
in the inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No as we add a new set of inputs to test, independent of the previous that uses input_ids
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of the test set-up is that the inputs to the model will now include both input_ids and input_embeds, which shouldn't be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, inputs_to_test
is a list of dict, with each dict being one input to the model.
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
|
||
if model.__class__.__name__ in set(MODEL_FOR_SEQUENCE_CLASSIFICATION_MAPPING_NAMES.values()) and ( | ||
not hasattr(model.config, "problem_type") or model.config.problem_type is None | ||
): | ||
model.config.problem_type = "single_label_classification" | ||
|
||
traced_model = symbolic_trace(model, input_names) | ||
model.config.use_cache = "past_key_values" in input_names_to_trace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about mamba here? uses cache_params 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mamba is afaik not supported in symbolic_trace
Hi @fxmarty Could you take a look on the following (see https://app.circleci.com/pipelines/github/huggingface/transformers/97223/workflows/f7cf9ddb-909b-4544-994a-18aaadbb77ac/jobs/1286547)? FAILED tests/models/blenderbot_small/test_modeling_blenderbot_small.py::BlenderbotSmallModelTest::test_torch_fx - ValueError: You have to specify either input_ids or inputs_embeds |
We should prompt users to use
torch.export.export
instead.Fixes #31414
Fixes #31200