Adding a new model to Transformers with additional dependencies
Hi,currently I am implementing a PyTorch version of theTapas algorithmproposed by Google AI. Since the algorithm is based on BERT, I am using parts ofmodeling_bert.pyto implement it. However, the original implementation of the Tapas algorithm (which is written in Tensorflow 1) contains operations on segmented tensors, such as:tf.math.unsorted_segment_meantf.math.unsorted_segment_sumtf.math.unsorted_segment_maxtf.math.unsorted_segment_minThese operations do not exist in PyTorch. However, there’s a small extension library calledpytorch_scatterwhich implements these operations. So I would use this library in my implementation.Is there any chance my implementation will be added to the Transformers library if it relies on libraries other than Pytorch? I read in the templates README of the Transformers repository that “the package is also designed to be as self-consistent and with a small and reliable set of packages dependencies. In consequence, additional dependencies are usually not allowed when adding a model but can be allowed for the inclusion of a new tokenizer (recent examples of dependencies added for tokenizer specificities includesentencepieceandsacremoses).”Thanks!
2020-08-31T13:59:39Z
You should copy and paste the code you need in an utils file associated to your model. There is little chance of having your model merged if it introduces a new dependency.
2020-09-13T19:06:52Z
Thank you for your answer. Where can I find utils files of other models in the Transformers repository?
2020-09-14T13:40:18Z
There `modeling_trasnfo_xl_utilities for instance.
2020-09-29T08:30:56Z
Thank you for your reply. I have another question. I have implemented the model, but now I want to implementTapasTokenizer, which prepares the data for the model. The API would look like this:from transformers import TapasTokenizer

data = {'Actors': ["Brad Pitt", "Leonardo Di Caprio"], 'Number of movies': ["87", "53"]}
table = pd.DataFrame.from_dict(data)
queries = ["What is the name of the first actor?", "How many movies has he played in?"]
answer_coordinates = [(0, 0), (0, 1)]
answer_text = ["Brad Pitt", "87"]

tokenizer = TapasTokenizer.from_pretrained("tapas-base-finetuned-sqa")
inputs = tokenizer(table, queries, answer_coordinates, answer_text)The “inputs” dictionary should contain the following:{'input_ids': [[  101,  2054,  2003,  1996,  2171,  1997,  1996,  2034,  3364,  1029, 102,  3364,  2193,  1997,  5691,  8226, 15091,  5187, 14720,  4487, 6178,  9488,  6584], 
                [101,  2129,  2116,  5691,  2038, 14720,  4487,  6178,  9488,  2209, 1999,  1029,   102,  3364,  2193,  1997,  5691,  8226, 15091,  5187, 14720,  4487,  6178,  9488,  6584],
'token_type_ids': [[0, 0, 0, 0, 0, 0, 0, 0, 0],
                            [0, 0, 0, 0, 0, 0, 0, 0, 0]],
'attention_mask': [[1, 1, 1, 1, 1, 1, 1, 1, 1],
                             [1, 1, 1, 1, 1, 0, 0, 0, 0]],
'label_ids':  [[...],[...]],
'answer': [[nan], ["87"]],
'aggregation_function_id': [[0],[0]],
'numeric_values': [...],
'numeric_values_scale': [...]
}TapasTokenizeruses the same vocabulary as BERT, but it encodes tables and associated queries. The inputs ids are [CLS] query [SEP] flattened table, and it creates additionaltoken type idsto encode tabular structure. In case one also wants to compute the loss, it createsanswer,aggregation function id,numeric valuesandnumeric values scalelists/tensors. What is the best way to implement this? Which function ofBertTokenizershould I overwrite to useTapasTokenizeras shown above? Or should I letTapasTokenizerjust inherit fromPretrainedTokenizerand define theencodefunction?
2020-09-29T13:54:10Z
Normally you can use theprepare_for_modelto add any new keys. Thetoken_type_idsthemselves are defined in thecreate_token_type_ids_from_sequencesfunction, so you could probably use theBertTokenizerand subclass it while implementing those two.
2020-10-12T11:01:20Z
Hey@sgugger, could you please consider running my notebook?https://colab.research.google.com/drive/159Sjlc1sQE1Cq3aLZeui4kB9JDxLeoRN#scrollTo=w8vysLPe_eIzIt showcases my current implementation of Tapas. It is still WIP, but I am interested in whether you are interested in adding it to the Transformers libraryThere is a requirement for thetorch-scatterlibrary as PyTorch currently does not include operations on segmented tensors. Could it be possible to add something likethiswhen initializingTapasForQuestionAnswering?
2020-10-12T13:29:12Z
Looks like you made a lot of progress, great work! I think havingtorch-scatteras a soft dependency required for this model is OK. Look at what was done in RAG recently, but basically, you should implement functions likeis_scatter_availableandrequires_scatterinfiles_utils.pyand have arequires_scatterin the init of your models (so it fails with a nice error message if a user doesn’t have the dep).It looks like you only have to do the final clean-up before opening a PR. Make sure you copy what is done in other modeling files for documentation to have clean docstrings for your models.One bit of work you have left is to remove imports form themodeling_bertfile: for easy customization by researchers, we want all model files to be independent (at the price of dupe code). We have a script that checks copies form other modeling files stay consistent (and can be automatically updated). Check what was done inmodeling_roberta.pyfor an example.
2020-10-13T09:12:00Z
That’s great to hearHowever, the main thing which should be improved istokenization_tapas.py. I have 2 questions about it:Currently, I’ve only implemented_batch_prepare_for_model, but it is not compatible with the different padding/truncation strategies of Transformers. Currently, the way it works is as follows: there’s a function_get_token_budgetintokenization_tapas.pythat calculates the number of tokens left for the flattened table after tokenizing a question. This is currently set to self.model_max_length - (len(question_tokens) + 2) (+ 2 for the CLS and SEP tokens), as was done in the original implementation. There is a hyperparameter when initializingTapasTokenizercalleddrop_rows_to_fitwhich drops rows of the table to fit into the token budget if set toTrue. If it’s set toFalseand a table is too big, it throws aValueErrorindicating'too many rows'. Should I make TapasTokenizer fully compatible with the padding/truncation strategies of Transformers? Or can I leave it like that with the hyperparameterdrop_rows_to_fit? Also, padding is currently simply done up toself.model_max_length. It’s currently not possible to pad up to the longest sequence in a batch. Again, this is because of the complex way examples are created, I cannot simply overwrite.pad()(maybe I can, but then I definitely need some help)The way data is prepared is a bit different compared to BERT and other models. If you provide more than 1 question related to a table as a batch to the tokenizer, then theprev_label_ids(which is one of the 7 token type ids the authors of Tapas introduce) are set to thelabel_idsof the previous table + question pair. These indicate which tokens of the current table  where an answer to the previous question (which is useful in a conversational setup - SQA is a dataset of conversational questions to a table). So I cannot simply overwrite thecreate_token_type_ids_from_sequencesfunction, because that function already expects ids. The token type ids are made based on the table and several already created token type ids.TLDR: the tokenizer implementation should be improved a lot and I would definitely need some assistance in doing so.Also, adding the docstrings and removing the BERT imports at the top ofmodeling_tapas.pyis maybe better to do at the very end, for readability/debugging/code reviewing  purposes? Becausemodeling_tapas.pyis currently already quite extensive. Can I maybe open up a pull request and assist the developers of HuggingFace? I’ve been working on the model since the end of August so I can definitely help in explaining how everything works. The original authors from Google also helped me while implementing the model (seethis Github thread).Thank you for your help!
2020-10-13T13:52:13Z
I’m not expert enough in the tokenizers to best advise you for this model. Maybe@thomwolfor@anthonycould have more insight?Documentation is a must before opening the PR, and no it does not hinder readability, it’s actually supposed to explain what the code doesFor the decoupling we can help if you need it, but it will need to be done before the PR is merged anyway.
2020-10-13T13:59:54Z
Haha okay, I understandin the meantime I’m gonna add a lot of documentation such that people can understand what the code does.
2020-10-15T14:17:13Z
@sguggerall docstrings are added (except for the model_doc, i.e. the .rst file).I tried replacing the bert imports with original code blocks, but I encountered an error which probably has to do with the fact that my branch is not properly synced with the master branch (it’s a lot of commits behind). Do I simply have togit checkout master
git add upstream https://github.com/huggingface/transformers.git
git fetch upstream
git rebase upstream/master? Should this be done from my local master branch as shown above (which I never worked on) or my local modeling_tapas branch? Currently when I type git remote -v I only getorigin  https://github.com/NielsRogge/transformers.git (fetch)
origin  https://github.com/NielsRogge/transformers.git (push)I’m not a git pro, but I just want to make sure I don’t mess things up. Or is it maybe better to save the Python files I added (modeling_tapas,tokenization_tapas, etc.), create a new branch, properly sync and then add those files?
2020-10-15T18:22:58Z
Just save the files you added in case something goes wrong and you need to recreate a branch. Then try the rebase with the instructions you set and fingers crossed, it should work.
2020-10-19T10:16:09Z
First of all, thank you for taking the time responding to my questions!Secondly, I created a new branchtapas_up_to_datethat is more up-to-date withupstream/masterin which I copied all the files I added and made more improvements.Currently, all things of thechecklistare done except for 2 things:tokenization_tapas.pyshould be improved (help/opinions needed)tests should be written appropriately (also help needed). Currently I’ve writtentest_modeling_tapas.pyof which 20 tests pass and 6 fail, but testing outTapasForQuestionAnsweringwould require Pandas, which obviously cannot be added to Transformers as an extra dependency. I’ve currently mainly tested out this model in Google Colab (such asthis one)by pip installing my branch. Note that pandas is not a dependency of the model itself, it’s a dependency to test out the model.All the rest is done (docstrings are added including the model doc which I checked usingmake html, a specialTableQuestionAnsweringOutputis defined, the BERT imports are replaced by original code blocks and the model still works, tapas has been added to theauto._.pyfiles, the soft dependency on torch-scatter has been added in the__init__ofTapasModel,…)You can find the branch here:https://github.com/NielsRogge/transformers/tree/tapas_up_to_dateCould you maybe check out my work (or point me to someone) to get some feedback? I don’t really know if my code is meeting the quality standards of Transformers to make a PR.Thank you!
2020-10-19T15:38:58Z
I don’t see where you need pandas in your branch? For the tests, make sure that you pass all common tests as this is what will make sure your models work well with the library.On the tokenizer side, pinging@thomwolfagain as he may have some more insight. Or@lysandremaybe?
2020-10-19T16:02:43Z
Hi@nielsr! Thanks for your work on the implementation of Tapas, and thanks for your help on github issues, it helps a lot!Regarding this current work, I think it would be simpler for everybody if you opened a PR, so that we may iterate with comments directly on the code. Opening PRs even if they’re in an unfinished state is completely okay, and we’ll be more than happy to guide you. We don’t have a quality standard for open PRs, only quality standards for merged PRs :)!Feel free to ping@sguggerand I (@LysandreJik) on the PR directly so that we may follow!