Skip to content
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

Tf longformer for sequence classification #8231

Merged

Conversation

elk-cloner
Copy link
Contributor

@elk-cloner elk-cloner commented Nov 2, 2020

What does this PR do?

implement SequenceClassification, MultipleChoice and TokenClassification classes for TFLongformer.
Resolves #6401

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to the it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors which may be interested in your PR.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good, but there are a few things to do before merging.

  • You introduced a bunch of spelling errors in the variable names/documentation?
  • You should run make style and make quality to fix the code quality test
  • You should implement the tests related to the added classes
  • You should add these classes to the TF auto models

…merForTokenClassification, TFLongformerForMultipleChoice
@patrickvonplaten
Copy link
Contributor

@elk-cloner - thanks a lot for taking a look into this!

Would be awesome to fix the TFLongformer related tests. There seem to be some obvious bug: UnboundLocalError: local variable 'input_ids' referenced before assignment .

I'll do a longer review once these tests are fixed :-) Lemme know if you need help at some point.

@elk-cloner
Copy link
Contributor Author

@patrickvonplaten i have passed all the tests but got stuck in test_inputs_embeds when it's checking TFLongformerForMultipleChoice model, i debugged my code and found out that inputs_embeds shape is not same when TFLongformerEmbeddings get call from here(test_inputs_embeds) and here(TFLongformerForMultipleChoice), but don't know how to fix it, can you help me ?

@elk-cloner elk-cloner closed this Nov 10, 2020
@elk-cloner elk-cloner reopened this Nov 10, 2020
@patrickvonplaten
Copy link
Contributor

Hey @elk-cloner,

yeah this problem was not at all obvious! Thanks for letting me know :-) For Multiple Choice, we have to make sure that the position_ids stay 2-dimensional, which is only relevant for TFLongformer, but not for other TF models -> so we need this if fix here.

Feel free to ping me again, when you're ready with the PR or need help :-)

@elk-cloner
Copy link
Contributor Author

@patrickvonplaten all tests have passed, can you take a look ?

@patrickvonplaten
Copy link
Contributor

Hey @elk-cloner - the signature of the function calls should be done analogs to the one in other modeling_tf_....py files. Woud be great if you can fix that before we merge.

@patrickvonplaten
Copy link
Contributor

Good to merge IMO!

@patrickvonplaten
Copy link
Contributor

Checked the slow tests and everything passes. Great job @elk-cloner! Longformer is definitely not the easiest model

@patrickvonplaten
Copy link
Contributor

Would be awesome if @LysandreJik and @sgugger can take a final look, then we're good to merge.

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! This looks good to merge, just a little thing missing on the doc side.

tests/test_modeling_tf_longformer.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_longformer.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_longformer.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_longformer.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_longformer.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_longformer.py Outdated Show resolved Hide resolved
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clean implementation. LGTM.

@LysandreJik LysandreJik merged commit 5362bb8 into huggingface:master Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TF Longformer] Add Multiple Choice, Seq Classification Model
4 participants