utterworks / fast-bert

Super easy library for BERT based NLP models
Apache License 2.0
1.85k stars 342 forks source link

Added electra to list of models for cls and lm #229

Closed lingdoc closed 3 years ago

lingdoc commented 4 years ago

Not sure if this conflicts with commit e9976584dfe39f3606ff28f1294595e30c2b22e1.

kaushaltrivedi commented 4 years ago

Yes it will conflict with using Electra for sequence classification. Can you get the latest changes and submit again.

lingdoc commented 4 years ago

Ok, rebased. From what I can tell (correct me if there's more to this), in e9976584dfe39f3606ff28f1294595e30c2b22e1 ElectraConfig is imported from Transformers in learner_cls.py and the following lines of code are added: https://github.com/kaushaltrivedi/fast-bert/blob/7b1bac2cba9a6203a640f197a23e0fac8dbe2e5e/fast_bert/learner_cls.py#L158-L166 My code removes lines 158-165 but adds the following imports from Transformers, in line with the other models: https://github.com/lingdoc/fast-bert/blob/f3034681442c8e943935ec1b9b06e987a7206c3f/fast_bert/learner_cls.py#L53-L55

    ElectraConfig,
    ElectraForSequenceClassification,
    ElectraTokenizer,

and creates an Electra bundle: https://github.com/lingdoc/fast-bert/blob/f3034681442c8e943935ec1b9b06e987a7206c3f/fast_bert/learner_cls.py#L103-L107

    "electra": (
        ElectraConfig,
        (ElectraForSequenceClassification, ElectraForMultiLabelSequenceClassification),
        ElectraTokenizer,
    ),

So it seems like for the basic ElectraForSequenceClassification class it's just preference whether you want the version in line with the other imports (importing directly from Transformers), or whether you prefer an if/else statement that uses ElectraConfig.

Otherwise, I introduce code in modeling.py to allow an ElectraForMultilabelSequenceClassification class, which also imports ElectraForSequenceClassification directly from Transformers.

The additional code in learner_lm.py just enables Electra to be used for language modeling.

lingdoc commented 4 years ago

I should add that to use the new ElectraForSequenceClassification from Transformers, you need to upgrade to Transformers 2.10.0 or greater, and that then your fast tokenizers might not work (see #222). So maybe we should wait until that bug is fixed before merging..?

kaushaltrivedi commented 4 years ago

ok. lets wait then.

lingdoc commented 4 years ago

Okay, finally updated and rebased. Should be good for merge, as #222 is resolved.

kaushaltrivedi commented 3 years ago

merged