add position_ids by jiqing-feng · Pull Request #984 · huggingface/optimum

jiqing-feng · 2023-04-18T08:00:07Z

Some models like gpt2 and llama have "position_ids" in their generation inputs. We can add "position_ids" in the model's config to fix it.

@echarlaix @fxmarty Would you please help to review it? Thanks!

fxmarty · 2023-04-18T10:19:01Z

Hi @jiqing-feng , just to be sure, is this concerning the ONNX export or ONNX Runtime integration? Or both? In the ONNX export, we should give the option, yes.

echarlaix · 2023-04-20T17:11:14Z

Currently position_ids is ignored during ORT inference for causal LM https://github.com/huggingface/optimum/blob/v1.8.2/optimum/onnxruntime/modeling_decoder.py#L683, so when enabling it as inputs for the ONNX export we should enable it for ORTModelForCausalLM as well

jiqing-feng · 2023-04-21T02:20:05Z

Hi @echarlaix @fxmarty thanks for your comments. I have added position_ids on ORTModelForCausalLM. Would you please help to review it? Thanks.

fxmarty · 2023-04-21T09:30:59Z

Hi @jiqing-feng , could you comment a bit on your use case? I'm not sure what's the benefit of this in the ORT integration.

While attention_mask is updated in _update_model_kwargs_for_generation, position_ids is not. Thus, in my understanding, if we are to pass a position_ids to the generate() method, it would be kept as a constant at each prepare_inputs_for_generation call in the generation, e.g. here (since we'd go into this piece of controlflow). Which is something we don't wish.

Am I misunderstanding something @gante ?

gante · 2023-04-21T10:01:56Z

@fxmarty What I write next is from a transformers point of view :)

At the moment, position_ids is a required input to support left padding. For legacy reasons, we consider it an optional input the same way attention_mask is an optional input -- if not passed, we assume all tokens are valid, and position_ids is a torch.arange. In .generate(), we delegate its creation to the model's prepare_inputs_for_generation, where it is computed from the attention_mask (e.g. here).

This means that while position_ids is never passed into .generate(), it is passed from .generate() to the model and is updated at each iteration.

fxmarty · 2023-04-21T10:10:39Z

In .generate(), we delegate its creation to the model's prepare_inputs_for_generation, where it is computed from the attention_mask (e.g. here).

So this means that executing this controlflow is absolutely necessary, right?

KexinFeng · 2023-04-24T19:57:36Z

Hi @jiqing-feng , could you comment a bit on your use case? I'm not sure what's the benefit of this in the ORT integration.

While attention_mask is updated in _update_model_kwargs_for_generation, position_ids is not. Thus, in my understanding, if we are to pass a position_ids to the generate() method, it would be kept as a constant at each prepare_inputs_for_generation call in the generation, e.g. here (since we'd go into this piece of controlflow). Which is something we don't wish.

Am I misunderstanding something @gante ?

@fxmarty Yeah, I was about to request for exactly the same feature. As @gante mentioned above, the position_ids (along with attention_mask) are necessary inputs to transformer based models like gpt2 to deal with left-padded input. More specifically, my feature request comes from the context described in #972.

In .generate(), we delegate its creation to the model's prepare_inputs_for_generation, where it is computed from the attention_mask (e.g. here).

So this means that executing this controlflow is absolutely necessary, right?

I don't think the controlflow of computing the position_ids from attention_mask is necessary, if this is what you referred to. As long as the traced transformer model (*.onnx) can take position_ids as an effective input, it should be good enough.

fxmarty · 2023-04-24T20:48:36Z

cc @echarlaix @michaelbenayoun

michaelbenayoun

On my side, I am ok with adding this as long as we make sure we do not:

Add any break changes
Do not break .generate, it should work with and without the position_ids (except for the left padding case of course)

michaelbenayoun · 2023-04-25T09:01:57Z

+    @property
+    def inputs(self) -> Dict[str, Dict[int, str]]:
+        if self.use_past_in_inputs:
+            common_inputs = {"input_ids": {0: "batch_size"}}
+            self.add_past_key_values(common_inputs, direction="inputs")
+            common_inputs["attention_mask"] = {0: "batch_size", 1: "past_sequence_length + 1"}
+            common_inputs["position_ids"] = {0: "batch_size"}
+        else:
+            common_inputs = {
+                "input_ids": {0: "batch_size", 1: "sequence_length"},
+                "attention_mask": {0: "batch_size", 1: "sequence_length"},
+                "position_ids": {0: "batch_size", 1: "sequence_length"},
+            }
+        return common_inputs
+


I guess we can just make LlamaOnnxConfig imherint from GPT2OnnxConfig and override the NORMALIZED_CONFIG_CLASS class attribute?

michaelbenayoun · 2023-04-25T09:04:25Z

                input_ids=input_ids,
                attention_mask=attention_mask,
                past_key_values=past_key_values,
+                position_ids=position_ids,


self.decoder is an ORTDecoder right?
If so, we also need to update its forward method to handle position_ids.

michaelbenayoun · 2023-04-25T09:04:33Z

+                position_ids=position_ids,
            )
        else:
            outputs = self.decoder_with_past(
                input_ids=input_ids[:, -1:],
                past_key_values=past_key_values,
                attention_mask=attention_mask,
+                position_ids=position_ids,


Same comment.

jiqing-feng · 2023-04-26T03:04:34Z

Hi @michaelbenayoun , thanks for your comment. I have updated position_ids in ORTdecoder forward. Could you please review it? Thanks! cc @fxmarty @gante

HuggingFaceDocBuilderDev · 2023-04-26T08:39:09Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

KexinFeng · 2023-08-09T16:56:10Z

Hi everyone, @jiqing-feng @fxmarty @gante
is there any updates on this feature?

jiqing-feng · 2023-08-10T01:34:33Z

Hi everyone, @jiqing-feng @fxmarty @gante is there any updates on this feature?

cc @michaelbenayoun

fxmarty · 2023-09-08T13:51:20Z

@jiqing-feng @KexinFeng I now realize I misunderstood your argument and motivation for the PR. This appears to me to be a major bug in Optimum which should be fixed ASAP.

fxmarty · 2023-09-14T09:50:36Z

Closing in favor of #1381

add position_ids

2a3a33e

add position_ids on ORTcasualLM

4b72c04

Merge branch 'huggingface:main' into main

ab03d61

michaelbenayoun reviewed Apr 25, 2023

View reviewed changes

jiqing-feng and others added 2 commits April 26, 2023 10:21

Merge branch 'huggingface:main' into main

ddc71d3

add position_ids on ORTdecoder forward

345ba64

jiqing-feng and others added 2 commits April 26, 2023 01:54

fix lint and tests(gpt2 inputs contain position_ids)

1d1ab7c

Merge branch 'huggingface:main' into main

c8ab3c2

fxmarty mentioned this pull request Sep 13, 2023

Add position ids in ONNX export and ORT #1381

Merged

3 tasks

fxmarty closed this Sep 14, 2023

Uh oh!

Conversation

jiqing-feng commented Apr 18, 2023

Uh oh!

fxmarty commented Apr 18, 2023

Uh oh!

echarlaix commented Apr 20, 2023

Uh oh!

jiqing-feng commented Apr 21, 2023

Uh oh!

fxmarty commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gante commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fxmarty commented Apr 21, 2023

Uh oh!

KexinFeng commented Apr 24, 2023

Uh oh!

fxmarty commented Apr 24, 2023

Uh oh!

michaelbenayoun left a comment

Choose a reason for hiding this comment

Uh oh!

michaelbenayoun Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

michaelbenayoun Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

michaelbenayoun Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

jiqing-feng commented Apr 26, 2023

Uh oh!

HuggingFaceDocBuilderDev commented Apr 26, 2023

Uh oh!

KexinFeng commented Aug 9, 2023

Uh oh!

jiqing-feng commented Aug 10, 2023

Uh oh!

fxmarty commented Sep 8, 2023

Uh oh!

fxmarty commented Sep 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fxmarty commented Apr 21, 2023 •

edited

Loading

gante commented Apr 21, 2023 •

edited

Loading