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

Add ESM to huggingface #13662

Closed
wants to merge 27 commits into from
Closed

Conversation

liujas000
Copy link

@liujas000 liujas000 commented Sep 21, 2021

What does this PR do?

Adding ESM-1b to huggingface following the steps in https://huggingface.co/transformers/add_new_model.html

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 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 who may be interested in your PR.
@sgugger

@liujas000 liujas000 marked this pull request as ready for review September 21, 2021 00:17
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 a lot for your PR! I left a few comments but it's already in pretty good shape.

I have a few more general interrogations:

  • The tokenizer fast is imported in the main inits and used in the doc, but it does not exist in the files, so you should either add the tokenizer fast or remove any mention of it.
  • Do all the model head make sense for this new architecture? From what I understand it's linked to proteins so I don't know if the task-specific heads like multiple choice or question answering really are useful.
  • There should be a file to test the tokenizer

docs/source/index.rst Outdated Show resolved Hide resolved
docs/source/model_doc/esm.rst Outdated Show resolved Hide resolved
docs/source/model_doc/esm.rst Outdated Show resolved Hide resolved
src/transformers/__init__.py Outdated Show resolved Hide resolved
src/transformers/__init__.py Outdated Show resolved Hide resolved
src/transformers/models/esm/modeling_esm.py Outdated Show resolved Hide resolved
src/transformers/models/esm/modeling_esm.py Outdated Show resolved Hide resolved
tests/test_modeling_esm.py Show resolved Hide resolved
tests/test_modeling_esm.py Show resolved Hide resolved
@liujas000
Copy link
Author

@sgugger , thanks for the feedback!
There's two common tests that I'm failing; do you have any insight into what the proper fix would be?

❯ pytest tests/test_modeling_esm.py --disable-warnings
==================================================== test session starts ====================================================
platform linux -- Python 3.7.10, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /private/home/jasonliu/work-huggingface/transformers-dev, configfile: setup.cfg
plugins: dash-1.21.0, forked-1.3.0, xdist-2.3.0, timeout-1.4.2, hydra-core-1.1.0
collected 67 items

tests/test_modeling_esm.py .....................................s..............FF.....sss..ss.                        [100%]

========================================================= FAILURES ==========================================================
______________________________________ ESMModelTest.test_save_load_fast_init_from_base ______________________________________

self = <tests.test_modeling_esm.ESMModelTest testMethod=test_save_load_fast_init_from_base>

    def test_save_load_fast_init_from_base(self):
        config, inputs_dict = self.model_tester.prepare_config_and_inputs_for_common()
>       base_class = MODEL_MAPPING[config.__class__]

tests/test_modeling_common.py:208:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = _LazyAutoMapping(), key = <class 'transformers.models.esm.configuration_esm.ESMConfig'>

    def __getitem__(self, key):
>       model_type = self._reverse_config_mapping[key.__name__]
E       KeyError: 'ESMConfig'

src/transformers/models/auto/auto_factory.py:513: KeyError
_______________________________________ ESMModelTest.test_save_load_fast_init_to_base _______________________________________

self = <tests.test_modeling_esm.ESMModelTest testMethod=test_save_load_fast_init_to_base>

    def test_save_load_fast_init_to_base(self):
        config, inputs_dict = self.model_tester.prepare_config_and_inputs_for_common()
>       base_class = MODEL_MAPPING[config.__class__]

tests/test_modeling_common.py:253:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = _LazyAutoMapping(), key = <class 'transformers.models.esm.configuration_esm.ESMConfig'>

    def __getitem__(self, key):
>       model_type = self._reverse_config_mapping[key.__name__]
E       KeyError: 'ESMConfig'

src/transformers/models/auto/auto_factory.py:513: KeyError
================================================== short test summary info ==================================================
FAILED tests/test_modeling_esm.py::ESMModelTest::test_save_load_fast_init_from_base - KeyError: 'ESMConfig'
FAILED tests/test_modeling_esm.py::ESMModelTest::test_save_load_fast_init_to_base - KeyError: 'ESMConfig'
============================== 2 failed, 59 passed, 6 skipped, 30 warnings in 99.74s (0:01:39) ==============================

@sgugger
Copy link
Collaborator

sgugger commented Oct 19, 2021

It doesn't look like you added your model in the configuration_auto mappings, just the modeling_auto mappings. That's why you get this error.

@liujas000
Copy link
Author

Thanks! I think this is ready for review again (rebased to upstream)

import torch

import esm as esm_module
from transformers.models.bert.modeling_bert import (
Copy link
Contributor

Choose a reason for hiding this comment

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

could we maybe replace those classes by the corresponding ESM... classes, e.g. ESMIntermediate, ...

Copy link
Contributor

Choose a reason for hiding this comment

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

This would still be nice to change before merging

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

I fixed a couple of things (corrected the config and removed dead code of ESMForQuestionAnswering) - hope that was ok!

PR is good for merge IMO. It would be nice if we could replace the BERT modules with ESM modules in the conversion script and then we should probably also flip the private model repo to public so that the integration test can run :-)

Should we upload the other checkpoitns as well: https://github.com/facebookresearch/esm#pre-trained-models-

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.

Nice, this looks good to me! Thank you for working on this!

@patrickvonplaten
Copy link
Contributor

Hey @liujas000,

Can we help you in any way to get this PR merged? :-)

@liujas000
Copy link
Author

@patrickvonplaten sorry for the delay; I will land this week!

@patrickvonplaten
Copy link
Contributor

Test failure seems unrelated

@patrickvonplaten
Copy link
Contributor

Thanks a lot for making this PR more or less mergeable @liujas000 . I think there are just some final comments from @sgugger and @patrickvonplaten to be taken care of and the PR is good to go :-)

@github-actions
Copy link

github-actions bot commented Feb 8, 2022

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.

@github-actions github-actions bot closed this Feb 17, 2022
@gianhiltbrunner
Copy link

I'd love to use this model, what still needs to be done to get this merged?

@patrickvonplaten
Copy link
Contributor

cc @liujas000 @Rocketknight1

@Rocketknight1
Copy link
Member

@gianhiltbrunner We're still waiting on an internal review from Facebook from the contributors, I believe! I'll let you know if there's any update.

@franzigeiger
Copy link

I would also be very happy if this gets merged! Any progress?

@patrickvonplaten
Copy link
Contributor

Any updates here?

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.

8 participants