Fix TFBert embedding tf variables with the same name - Fixes problems with checkpoints under tf.distribute.Strategy
Author: marhlder
Pull request: yes

# What does this PR do?

Removes usage of tf.name_scope() in BERT like models and replaces it with layers. 
Ideally all erroneous use of tf.name_scope() should be fixed across all models, but this PR will at least make the TFBert like models work. 

<!-- Remove if not applicable -->

Fixes # (issue)
#11169 

## Models:

- bert: @LysandreJik



Comments:
Hi! I'm the new Tensorflow maintainer at Hugging Face. Your PR looks good, and you're right that unique weight names is a better strategy than relying on `name_scope`. Now that you've raised the issue, doing a check for that across the whole codebase is definitely on my to-do list.

The main issue for us is backward compatibility and ensuring that cross-loading weights from PyTorch checkpoints still works as expected after the change. Can you leave this with me for a few days until I get a chance to review that properly? Hopefully there are no issues, but I don't want to be the guy who breaks the whole codebase in his first week, lol.

> Hi! I'm the new Tensorflow maintainer at Hugging Face. Your PR looks good, and you're right that unique weight names is a better strategy than relying on `name_scope`. Now that you've raised the issue, doing a check for that across the whole codebase is definitely on my to-do list.
> 
> The main issue for us is backward compatibility and ensuring that cross-loading weights from PyTorch checkpoints still works as expected after the change. Can you leave this with me for a few days until I get a chance to review that properly? Hopefully there are no issues, but I don't want to be the guy who breaks the whole codebase in his first week, lol.

Hi @Rocketknight1 that sounds great! Thx for taking a look at this :) Just to clarify a bit: It is my current understanding that  tf.name_scope() makes absolutely no difference when it comes to variables in tf 2.x and is not comparable to the old tf.compat.v1.variable_scope. There is, to the best of my knowledge, no point in adding these name scopes for variables in tf 2.x.
They might still be useful for grouping certain ops in the graph under logical names, but TensorFlow 2.x generally relies on the object hierarchy of tf.Module subclass objects rather than global variable names / name spaces. See: https://www.tensorflow.org/guide/migrate#2_use_python_objects_to_track_variables_and_losses

I checked the test logs and we have several failing tests involving loading model weights, so it seems like there might be backward compatibility issues with this change, even though you're totally right about the `name_scope()` issues. So unfortunately, I probably can't merge this PR as is.

I'd like to resolve the underlying problem, though - if you want to try to figure out the compatibility issues yourself you can, or if not (which would be completely understandable, lol) I'll try to take a look when I get a chance.

@Rocketknight1 Hey, I think I have pretty much fixed the issues. But all of this arcane template usage is giving me a headache, any ideas where/how I might be make the final test pass?
(Model templates runner / run_tests_templates)

The error message form the test suggests running "make fix-copies" but that does not seem to do anything in the current state.

Hey! Don't worry too much about the template issues, we can fix those up for you before we merge it.

This is something that will affect a few teams, though - we're currently in the process of making sure everyone knows about it and they don't think it'll catastrophically break anything, but we might have to make some changes, which will probably be Monday because it's 7pm at the French office on a Friday right now!

Thanks again for the work you put into this and for identifying the problem, though - I'll try to keep you updated as we figure out if we can use your solution, or which tweaks we'll have to make to make it fit in with our other projects.

> Hey! Don't worry too much about the template issues, we can fix those up for you before we merge it.
> 
> This is something that will affect a few teams, though - we're currently in the process of making sure everyone knows about it and they don't think it'll catastrophically break anything, but we might have to make some changes, which will probably be Monday because it's 7pm at the French office on a Friday right now!
> 
> Thanks again for the work you put into this and for identifying the problem, though - I'll try to keep you updated as we figure out if we can use your solution, or which tweaks we'll have to make to make it fit in with our other projects.

Have a great weekend :) 

So this seems to be taking quite a while. Is there anything I can do to help and/or expedite this process? Thx in advance. 

I'm sorry about the delay! I've checked with everyone and we think it's okay, but there's an issue with ensuring this code stays in sync with the other BERT-based models. It's going slowly because I only started a couple of weeks ago, so I'm very paranoid about breaking things, and I'm double-checking things as I go.

> I'm sorry about the delay! I've checked with everyone and we think it's okay, but there's an issue with ensuring this code stays in sync with the other BERT-based models. It's going slowly because I only started a couple of weeks ago, so I'm very paranoid about breaking things, and I'm double-checking things as I go.

No worries :) You should be moving fast and breaking things. That's what you have tests for ;) 
Also why on earth are you guys doing all of this templating in the first place? It seems like a total maintenance nightmare and a textbook example of what not to do? You could cleanup and remove like 80% of your code for your MLMs with some standard object oriented programming? Or is there something I'm not seeing here?

It's a good question! The underlying idea is that we want code to be self-contained and easy to separate from the rest of the library, so that users can work on the model they care about in isolation without needing to understand our whole hierarchy of abstractions and imports. 

It's also helpful because we care a lot about supporting a variety of models that were trained outside of Hugging Face, which often involves reproducing their particular quirks rather than just importing the same single function in every case.

Any updates? I would like to contribute in any way I can :) Other people in my organisation are starting to use HugginFace transformers at the same scale as me and will likely face the same issues as I did. 
Is there a branch I can follow?

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](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md) are likely to be ignored.

Was this ever fixed?

No, but we're seeing other issues being caused by the same underlying problem, such as #12245 . I'm very aware of it, but finding a way to fix it without breaking changes to backward compatibility is difficult! It might be something that'll have to wait until a major release when we can break a lot of things at once.