-
Notifications
You must be signed in to change notification settings - Fork 60
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
Fix pipelines #608
Fix pipelines #608
Conversation
The docker package must be updated to be compatible with the latest requests release. In the meantime we pin requests version.
cfa1c39
to
fb1d6fc
Compare
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. |
b88ce62
to
68a6e24
Compare
c942f7c
to
53f7ed4
Compare
This base class will implement transformers PreTrainedModel methods that are not implemented in optimum PreTrainedModel base class.
53f7ed4
to
be54234
Compare
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.
Thx David for opening the PR! Left some nits!
neuron_config: Optional["NeuronDefaultConfig"] = None, | ||
**kwargs, | ||
): | ||
def __init__(self, model: "PreTrainedModel", config: "PretrainedConfig"): |
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.
For traced models, we are passing torch.jit._script.ScriptModule
as model here.
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.
I realized that when the CI failed ... 🫣
huggingface_token = use_auth_token | ||
elif use_auth_token: | ||
huggingface_token = HfFolder.get_token() | ||
if hasattr(model, "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.
Is it necessary, for traced model setting all dummy device as CPU would be fine I think.
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.
Yes, but I was wondering if at some point we wouldn't end up with models on XLA device.
Whether the Neuron model has separated weights and neff graph (by setting `inline_weights_to_neff=False` during the compilation). | ||
""" | ||
return not self.config.neuron.get("inline_weights_to_neff", True) | ||
def to(self, device: Union[str, 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.
Do we need to add content if this is actually a no-op function.
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.
As long as it exist, someone might want to call it, so I added a check.
@@ -0,0 +1,611 @@ | |||
# coding=utf-8 |
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.
Maybe just keep it in modeling_base.py
?
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.
LGTM!
What does this PR do?
This fixes a regression in Neuron pipelines with
transformers == 4.40.2
.The issue is actually fixed in transformers as a side-effect of this pull-request:
huggingface/transformers#30534
But the issue in
optimum-neuron
actually comes from the fact that all neuron models are based onoptimum.OptimizedModel
that reimplementstransformers.PretrainedModel
without providing all its methods (includingto()
).This pull-request therefore introduces a new top-level
NeuronModel
class that inherits fromoptimum.OptimizedModel
and provides the missing methods.All neuron model classes should now inherit from
NeuronModel
.In the process,
NeuronBaseModel
is renamed toNeuronTracedModel
to remove any ambiguity.