-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Add Luke to Onnx #16562
Add Luke to Onnx #16562
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
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 this contribution @aakashb95 🤗 ! Overall this is looking good and I think we can minimally add support for the masked-lm
feature (the other features need a discussion)
Can you also please fix the merge conflicts and test that the slow tests pass with:
RUN_SLOW=1 pytest tests/onnx/test_onnx_v2.py -k "luke"
Thanks!
@@ -260,6 +261,10 @@ class FeaturesManager: | |||
"token-classification", | |||
onnx_config_cls=LayoutLMOnnxConfig, | |||
), | |||
"luke": supported_features_mapping( | |||
"default", |
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.
We should include the other heads that LUKE supports:
masked-lm
entity-classification
entity-pair-classification
entity-span-classification
Now, masked-lm
is easy to add, but for the entity-*
tasks you'll need to add new entries in the _TASKS_TO_AUTOMODELS
mapping in the FeaturesManager
here. One somewhat annoying thing about LUKE is that these new mappings are specific to just one model and we lose the nice correspondence with autoclasses.
@michaelbenayoun do you think it's OK to include LUKE-specific classes inside the FeaturesManager
?
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'm not very familiar with LUKE, but if it does not fit what is currently being done, I think it is okay to add those classes in the FeaturesManager
I have all requirements installed and yet, I get the following output when I run the slow test:
What am I missing? |
You need to add the luke architecture to be tested in |
the slow tests resulted in the following errors: |
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
@lewtun could you revive this PR? Seems like it was already in a good state. |
Hey @aakashb95 sorry for the slow reply! The reason your tests are failing is because:
"entity-classification": OrderedDict({"logits": {0: "batch"}}),
"entity-pair-classification": OrderedDict({"logits": {0: "batch"}}),
"entity-span-classification": OrderedDict({"logits": {0: "batch"}}), After that, I think this PR should be in a good shape and the tests will pass :) |
Thanks for reviving this PR! The tests fail with errors similar to the one below:
If my understanding is correct, the Attaching logs for reference: |
66eab11
to
d973335
Compare
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
cc'ing @lewtun here |
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
What does this PR do?
I added lines to make Luke models available for Onnx conversion.
Who can review?
adding @NielsRogge since he worked on luke