-
Notifications
You must be signed in to change notification settings - Fork 173
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
a initial version of executor #61
Conversation
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.
Seems like we should support batch execution? This can be done in a separate PR if you like
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.
Looks good! Only have a few small points for revisino
else: | ||
output = model.generate(input_ids=encoded_input["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.
Should we verify that the model in the else
case is a transformers.AutoModelForCausalLM
?
else: | |
output = model.generate(input_ids=encoded_input["input_ids"]) | |
elif issubclass(model.__class__, transformers.AutoModelForCausalLM): | |
output = model.generate(input_ids=encoded_input["input_ids"]) | |
else: | |
raise ValueError(f"Model class {model.__class__} not supported") |
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.
Hmmm. I agree with you and actually tried this before. But issubclass(model.__class__, transformers.AutoModelForCausalLM)
would fail even for gpt2
. 🤔
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.
In [3]: gpt2_model_name = "gpt2"
...: gpt2_model = AutoModelForCausalLM.from_pretrained(gpt2_model_name)
...: gpt2_tokenizer = AutoTokenizer.from_pretrained(gpt2_model_name)
In [4]: model = gpt2_model
In [5]: issubclass(model.__class__, transformers.AutoModelForCausalLM)
Out[5]: False
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.
Actually, the inherited tree of GPT-2 is:
nn.Module -> PreTrainedModel -> GPT2PreTrainedModel -> GPT2LMHeadModel.
So issubclass(model.__class__, transformers.AutoModelForCausalLM)
would fail.
But do we have better methods of type check? 🤔
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.
ah I see....if there's no general type that can cover all autoregressive LM models then I think it's fine how you have it now
tests/model_executor_test.py
Outdated
if gpt2_tokenizer.pad_token is None: | ||
gpt2_tokenizer.add_special_tokens({"pad_token": "[PAD]"}) | ||
gpt2_model.config.pad_token_id = gpt2_model.config.eos_token_id | ||
gpt2_model.config.attention_mask_fn = lambda 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.
It's a bit weird to have this setup done in the test. Can you explain this a little more?
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.
Just as what happens in #60. Directly use gpt2_tokenizer
is not enough.
Using pad_token, but it is not set yet.
The attention mask and the pad token id were not set. As a consequence, you may observe unexpected behavior. Please pass your input's `attention_mask` to obtain reliable results.
Setting `pad_token_id` to `eos_token_id`:50256 for open-end generation.
else: | ||
output = model.generate(input_ids=encoded_input["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.
ah I see....if there's no general type that can cover all autoregressive LM models then I think it's fine how you have it now
Description
Following the document string, I implement the initial version of
ModelExcuctor
. However, I have some quick question:I think the calculation
probs.mean().item()
does provide a measure that can be loosely interpreted as the model's confidence in generating the output, but not enough.auxiliary_info
, where will we use it, and how to get it?References
Blocked by