[Proposal] Copy Pasting modeling_bart.py
We are considering copy pasting the contents of modeling_bart.py into modeling_pegasus.py, modeling_mbart.py and  modeling_marian.py.  and wanted to see what the community thinks.Currently, here ismodeling_pegasus.py:github.comhuggingface/transformers/blob/master/src/transformers/modeling_pegasus.py# coding=utf-8
# Copyright 2020 Google and The HuggingFace Inc. team.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""PyTorch Pegasus model, ported from https://github.com/google-research/pegasus"""


from .configuration_pegasus import PegasusConfig
from .file_utils import add_start_docstrings
from .modeling_bart import BART_START_DOCSTRING, BartForConditionalGenerationThis file has been truncated.show originalmarian and mbart are similar. They do nothing and inherit the complete implementation from bart. They are only their own classes so thatAutoConfig/AutoTokenizerwork properly.Arguments for copy pasteif you want to know how a model works you can read it in one file.This is how the rest of the repo works and there is no good reason to introduce a stylistic inconsistency here.we can delete 6 configuration parameters, that would become always True/always False.# params needed for mbart
        self.scale_embedding = scale_embedding  # scale factor will be sqrt(d_model) if True
        self.normalize_embedding = normalize_embedding  # True for mbart, False otherwise
        self.normalize_before = normalize_before  # combo of fairseq's encoder_ and decoder_normalize_before
        self.add_final_layer_norm = add_final_layer_norm

        # Params introduced for Marian
        self.add_bias_logits = add_bias_logits
        self.static_position_embeddings = static_position_embeddingswe could also delete 6 correspondingifstatementsit might be easier to make one off fixes or modify one model without unintended consequences.in the survey, 65% of respondents said they wanted more “Self-contained” code. This is what users want!Arguments against copy pastewe have to add and maintain about 4,500 lines of code. Could slow us down on adding new features/models.this might make the code harder to maintain. Some bugs/features that require 1 change will require 4 changes.the implementations could drift, making it more difficult for the same finetuning code to work with multiple models.Integration tests enforce that you can’t have big unintended consequences when you are refactoring, so the “unintended side effects” point is moot.It is actually harder to understand what is going on in pegasus/mbart/marian if the code is duplicated. Seeing that pegasus state dict is identical to bart makes reading the codeeasier.  “It is annoying to have to read so many similar implementations and have no easy way to see the delta.”Do not repeat yourself/rule of 3 are best practice and there is no good reason for this lib to deviate.In the survey, people said they wanted better examples/more models faster, having to maintain more code would distract the team from those objectives.Maintaining equivalent test coverage after copy paste would require adding 111 tests!Would love to hear other people’s opinions as there is some disagreement within the team.You can votehereand comment below.I am probably missing alternative solutions, pros and cons. I would love to hear!
2020-09-15T02:40:54Z
I really like keeping all four models combined into one file (as it is the case right now). I believe this is much better than copy-pasting. In addition to the software engineering advantages that are mentioned above about maintainability (which are not small things with a library that’s growing so fast like yours), I want to add two points from personal experience,Easier to understand and compare models: Here’s a recent use case; I have a working model that uses BART and I wanted to try Pegasus. Running the same training code results in NaNs with fp16. With a quickdiffof the two configs, I can find out the model design changes that might be the reason for the fp16 issues. Now to compare with T5 (implemented in a separate file), it is many hours of code-reading to find the differences (compared with 1 minute to diff the configs).Easier to try new ideas: I was recently experimenting with adding Longformer self-attention to various models on the HF/transformers repo. Once I have it implemented for BART, I get it for free for Pegasus and the other models sharing the same file. On the other hand, you can check the Longformer Github repo for the number of issues requesting adding longformer self-attention to BERT, ALBERT, ELECTRA, … etc. Each one requires its own implementation, and each one is a ton more work. Many similar use cases exist, experimenting with a new activation function, a new position embedding, a new type of layer normalization … etc.I also want to comment on some of the arguments for the copy-paste mentioned above:delete 6 configuration parameters and delete 6 corresponding if statementsI think the benefits mentioned above outweigh the gains from deleting these lines. Also, remember that removing these lines comes at the expense of a massive amount of added code.if you want to know how a model works you can read it in one file.It is still one file of code, but with an accompanying config file. If the additional config file is undesirable, maybe it can be clarified in code via comments so that the user doesn’t even need the config. Something like:if config.position_embedding == `learned`: # BART, mBART
   self.embedding = LearnedPositionalEmbedding()
elif config.position_embedding == `sinusoidal`:: # Pegasus, Marina, T5
   self.embedding = SinusoidalPositionalEmbedding()The annoying code design that makes reading model code difficult is when you introduce higher-level abstractions and move them to their own files. Having a separate file for self-attention, another file for the embedding layer, and a third for objective functions means you keep moving from one abstraction to another and jumping between files, which makes it challenging to grasp the model details. This is not the case here.in the survey, 65% of respondents said they wanted more “Self-contained” code. This is what users want!This code design satisfies the self-containment requirement. The few if-else statements with code for other models won’t be a deal-breaker. As mentioned above, I think the design that the users were voting against is something like fairseq and allennlp that have lots of layers of abstractions and lots of magic that happens behind the scene. I think the current design strikes a nice balance between fairseq/allennlp as one extreme, and absolute self-containment with tons of duplicate code on the other extreme.
2020-09-15T03:54:44Z
Wow,@sshleifer- that’s an amazing very balanced list of pros and cons! You made it hard to choose sides. To add to your listFSMTcould also re-use about half of Bart’s components as is.@beltagy’s comment is very insightful - I agree with a lot of what you said, thank you!Here are just a few ideas from me:1. unfolding toolIn the most recent discussion, that I’m aware of, where we tried to introducesome refactoring, it was mentioned that a tool has been in works (or being discussed?) that will be able to unfold the code for those who want it. If that is possible my vote would go there.Such tool would permit for a much easier development and maintenance, while allowing those who want to read a single file with all the code in it by unfolding the abstractions.2. generic component libraryIf such tool is not possible, I’d vote for creating more generic components, so there will be a set of Encoders and Decoders, create as many as needed and then plug those in. One doesn’t go and read the code fortorch.nn.Linearmost of the time, you quickly learn what it does. Perhaps the same can be done for more complex components and just create a variety of them so that they could be easily plugged in. i.e. ifBartEncoderis a thing (and it is), then how is it different fromtorch.nn.Linearif there are many who want to use it.So what I’m proposing is not to copyBartEncoder, but to put it intotransformers_encoders.pywith all the other encoders and thus creating the encoders library, so evenmodeling_bart.pyimports it as a generic component.Maintaining equivalent test coverage after copy paste would require adding 111 tests!Not so if you copy all the testsWhich btw, is also a problem to add to your list - as the test suite run-time will be much much longer - and that’s a problem.p.s. I’m not sure that your voting tool is not game-able - once I voted it asked me if I wanted to vote again. But in any case I’m not sure how valid the results would be, how do we know which groups of people were reached by this post and whether the voting process doesn’t get gamed by having an unbalanced sample.
2020-09-15T14:05:55Z
I  agree with Iz here.As long as the code which the model inherits is in single file, then it’s readable and personally for me don’t make much sense to copy paste.And if the models share the code then adding some new functionality (gradient checkpoiting, new heads) to the base model gives me the same functionality for free for other sub-classed  models. This actually helps more with experimentation. For example I wanted to tryMBartfor seq classification and as it inherits fromBARTcompletely all I had to do was just subclassBartForSequenceClassfication.Without this, I would have to copy paste the head, test it again which slows down things.Another example,  I wanted to experiment withCamembertwithEncoderDecoderand as it inherited fromRobertawhich was already inEncoderDecoder, it was very simple and fast change without requiring much extra code and tests as the tests also came free with base model.And IMO in  some cases such refactoring might even introduce unintended effects, IIRC after refactoring longformer to remove Roberta abstraction a major slowdown was introduced,@beltagymight remember this.
2020-09-15T18:47:10Z
I am a big huggingface fan!I want to voice more support for not copy pasting. I think it makes the code much easier to develop and maintain. I also think it makes it much easier to read and understand, and would disagree with the claim that copy-pasting code makes it easier to read.When we read code we implicitly delegate understanding to other portions of a code base. When we see something likeself.calc_mse_losswe understand what’s going on, even if the function lives in another file. The same is true when we seenn.Linear- we don’t need to copy all of the torch code and instead rely on torch to have implemented it correctly. By copying lots of code between models, we have it all in one place, but we make it hard for readers to read and understand the code at a higher-level of abstraction.It is helpful when reading code to understand exactly where a model differs from similar models, but if all the code is copy-pasted into a model class, it is hard to figure this out. For example, when we seeclass RobertaEmbeddings(BertEmbeddings)we know thatRobertaEmbeddingsis largely similar to aBertEmbeddings, and the code that follows in the definition ofRobertaEmbeddingsis only the ways in which Roberta is different from Bert. Instead, if we copy paste a lot of code from BERT into Roberta, we don’t know how they differ without carefully reading both, and it is difficult to even figure out which classes are more similar to Roberta. I think this is more cognitive burden on someone reading the code, not less.(really 2a) Requiring that code be identical leads to cases where naming conventions are confusing. For example T5 calls thingsblockswhile other models refer to them aslayers. This leads to cases where we need nearly identical methods just because things are named differently (e.g.copy_to_student). As a reader, this further obfuscates the similarities and differences between T5 and other models.