Fix Bart type hints #16297

gchhablani · 2022-03-21T13:41:05Z

What does this PR do?

This PR adds type-hints to PLBart PyTorch.

A few queries:

What about the configuration files? Do they not need type-hinting?
I understand someone is working on the BART model: https://github.com/huggingface/transformers/pull/16270/files. There are some copies on there, can I wait for that PR to be merged and see if there are any fixes needed after doing make fixup from their PR?

Rocketknight1 · 2022-03-21T15:19:34Z

Hi @gchhablani that is correct. We have just merged the BART PR, which has overwritten parts of modeling_plbart.py. Please check if any further changes are needed to modeling_plbart.py - if they are, you can either submit a new PR for them, or rebase this PR onto the updated main/master branch.

Rocketknight1 · 2022-03-21T15:19:50Z

But also, thanks for doing this - your PR looks very solid otherwise!

…bart_pt

gchhablani · 2022-03-21T17:42:25Z

@Rocketknight1 I have tried to make things a bit more consistent with Bart.
For example:

One place was using torch.FloatTensor for encoder_attention_mask, which should be torch.LongTensor.
Another discrepancy was that encoder_hidden_states were FloatTensor in one place, while Tensor in another.
input_ids are actually optional because we can also pass input_embeds but it was not present.
past_key_values was List[torch.Tensor] in some places, while Tuple[torch.Tensor] in some. Not 100% sure which is correct. I used List for now.

PLBart has some additional changes, which may be included. Wdyt?

HuggingFaceDocBuilderDev · 2022-03-21T18:04:51Z

The documentation is not available anymore as the PR was closed or merged.

Rocketknight1 · 2022-03-22T14:18:25Z

Hi @gchhablani I think this PR is quite difficult to review right now because of all the changes! If possible, could you close this PR, make sure your local repo is updated to the latest changes, then make a new branch and make the plBART PR there? I think if you do that, and only add annotations to the methods that are missing them, then we shouldn't have the problem of multiple affected files.

Thanks, and I'm sorry for the confusion here!

gchhablani · 2022-03-22T14:31:55Z

Hi @Rocketknight1!

Sure!
What about the issues with BART?

I think the changes in BART and PLBart are the only ones that need reviewing. Wdyt?

Rocketknight1 · 2022-03-22T15:12:32Z

I think it's fine to focus on plBART for this PR. The issues with BART aren't serious, and if we decide to be consistent with Tuple or List across the whole repo, we can do that with a simple find-replace. So in your plBART PR, don't worry about making changes that will also require changes to BART - just annotate missing types for methods that don't have any yet, and that's perfect!

…rt_pt

Rocketknight1

This looks good! I made one suggestion (adding the output type), but other than that ping me when you're ready and I'll merge it!

Rocketknight1 · 2022-03-31T12:30:41Z

src/transformers/models/plbart/modeling_plbart.py

        output_attentions: Optional[bool] = None,
        output_hidden_states: Optional[bool] = None,
-        return_dict: Optional[bool] = None
+        return_dict: Optional[bool] = None,
    ):


Suggested change

):

) -> Union[Tuple[torch.Tensor], Seq2SeqLMOutput]:

gchhablani · 2022-04-01T10:51:52Z

@Rocketknight1 Can you please review it now?
Thanks!

Rocketknight1 · 2022-04-01T13:50:10Z

Looks great to me, thanks for the PR!

* Add type hints to PLBart PyTorch * Remove pending merge conflicts * Fix PLBart Type Hints * Add changes from review

* 📝 add image/vision classification and asr * 🖍 minor formatting fixes * Fixed a typo in legacy seq2seq_trainer.py (#16531) * Add ONNX export for BeiT (#16498) * Add beit onnx conversion support * Updated docs * Added cross reference to ViT ONNX config * call on_train_end when trial is pruned (#16536) * Type hints added (#16529) * Fix Bart type hints (#16297) * Add type hints to PLBart PyTorch * Remove pending merge conflicts * Fix PLBart Type Hints * Add changes from review * Add VisualBert type hints (#16544) * Adding missing type hints for mBART model (PyTorch) (#16429) * added type hints for mbart tensorflow tf implementation * Adding missing type hints for mBART model Tensorflow Implementation model added with missing type hints * Missing Type hints - correction For TF model * Code fixup using make quality tests * Hint types - typo error * make fix-copies and make fixup * type hints * updated files * type hints update * making dependent modesls coherent Co-authored-by: matt <[email protected]> * Remove MBart subclass of XLMRoberta in tokenzier docs (#16546) * Remove MBart subclass of XLMRoberta in tokenzier * Fix style * Copy docs from MBart50 tokenizer * Use random_attention_mask for TF tests (#16517) * use random_attention_mask for TF tests * Fix for TFCLIP test (for now). Co-authored-by: ydshieh <[email protected]> * Improve code example (#16450) Co-authored-by: Niels Rogge <[email protected]> * Pin tokenizers version <0.13 (#16539) * Pin tokenizers version <0.13 * Style * Add code samples for TF speech models (#16494) Co-authored-by: ydshieh <[email protected]> * [FlaxSpeechEncoderDecoder] Fix dtype bug (#16581) * [FlaxSpeechEncoderDecoder] Fix dtype bug * more fixes * Making the impossible to connect error actually report the right URL. (#16446) * Fix flax import in __init__.py: modeling_xglm -> modeling_flax_xglm (#16556) * Add utility to find model labels (#16526) * Add utility to find model labels * Use it in the Trainer * Update src/transformers/utils/generic.py Co-authored-by: Matt <[email protected]> * Quality Co-authored-by: Matt <[email protected]> * Enable doc in Spanish (#16518) * Reorganize doc for multilingual support * Fix style * Style * Toc trees * Adapt templates * Add use_auth to load_datasets for private datasets to PT and TF examples (#16521) * fix formatting and remove use_auth * Add use_auth_token to Flax examples * add a test checking the format of `convert_tokens_to_string`'s output (#16540) * add new tests * add comment to overridden tests * TF: Finalize `unpack_inputs`-related changes (#16499) * Add unpack_inputs to remaining models * removed kwargs to `call()` in TF models * fix TF T5 tests * [SpeechEncoderDecoderModel] Correct Encoder Last Hidden State Output (#16586) * initialize the default rank set on TrainerState (#16530) * initialize the default rank set on TrainerState * fix style * Trigger doc build * Fix CI: test_inference_for_pretraining in ViTMAEModelTest (#16591) Co-authored-by: ydshieh <[email protected]> * add a template to add missing tokenization test (#16553) * add a template to add missing tokenization test * add cookiecutter setting * improve doc * Update templates/adding_a_missing_tokenization_test/README.md Co-authored-by: Sylvain Gugger <[email protected]> Co-authored-by: Sylvain Gugger <[email protected]> * made _load_pretrained_model_low_mem static + bug fix (#16548) * handle torch_dtype in low cpu mem usage (#16580) * [Doctests] Correct filenaming (#16599) * [Doctests] Correct filenaming * improve quicktour * make style * Adding new train_step logic to make things less confusing for users (#15994) * Adding new train_step logic to make things less confusing for users * DO NOT ASK WHY WE NEED THAT SUBCLASS * Metrics now working, at least for single-output models with type annotations! * Updates and TODOs for the new train_step * Make fixup * Temporary test workaround until T5 has types * Temporary test workaround until T5 has types * I think this actually works! Needs a lot of tests though * MAke style/quality * Revert changes to T5 tests * Deleting the aforementioned unmentionable subclass * Deleting the aforementioned unmentionable subclass * Adding a Keras API test * Style fixes * Removing unneeded TODO and comments * Update test_step too * Stop trying to compute metrics with the dummy_loss, patch up test * Make style * make fixup * Docstring cleanup * make fixup * make fixup * Stop expanding 1D input tensors when using dummy loss * Adjust T5 test given the new compile() * make fixup * Skipping test for convnext * Removing old T5-specific Keras test now that we have a common one * make fixup * make fixup * Only skip convnext test on CPU * Update src/transformers/modeling_tf_utils.py Co-authored-by: Sylvain Gugger <[email protected]> * Update src/transformers/modeling_tf_utils.py Co-authored-by: Sylvain Gugger <[email protected]> * Avoiding TF import issues * make fixup * Update compile() to support TF 2.3 * Skipping model.fit() on template classes for now * Skipping model.fit() on template class tests for now * Replace ad-hoc solution with find_labels * make fixup Co-authored-by: Sylvain Gugger <[email protected]> * Adding missing type hints for BigBird model (#16555) * added type hints for mbart tensorflow tf implementation * Adding missing type hints for mBART model Tensorflow Implementation model added with missing type hints * Missing Type hints - correction For TF model * Code fixup using make quality tests * Hint types - typo error * make fix-copies and make fixup * type hints * updated files * type hints update * making dependent modesls coherent * Type hints for BigBird * removing typos Co-authored-by: matt <[email protected]> * [deepspeed] fix typo, adjust config name (#16597) * 🖍 apply feedback Co-authored-by: Cathy <[email protected]> Co-authored-by: Jim Rohrer <[email protected]> Co-authored-by: Ferdinand Schlatt <[email protected]> Co-authored-by: Dahlbomii <[email protected]> Co-authored-by: Gunjan Chhablani <[email protected]> Co-authored-by: Rishav Chandra Varma <[email protected]> Co-authored-by: matt <[email protected]> Co-authored-by: Yih-Dar <[email protected]> Co-authored-by: ydshieh <[email protected]> Co-authored-by: NielsRogge <[email protected]> Co-authored-by: Niels Rogge <[email protected]> Co-authored-by: Lysandre Debut <[email protected]> Co-authored-by: Patrick von Platen <[email protected]> Co-authored-by: Nicolas Patry <[email protected]> Co-authored-by: Daniel Stancl <[email protected]> Co-authored-by: Sylvain Gugger <[email protected]> Co-authored-by: Matt <[email protected]> Co-authored-by: Karim Foda <[email protected]> Co-authored-by: SaulLu <[email protected]> Co-authored-by: Joao Gante <[email protected]> Co-authored-by: Sanchit Gandhi <[email protected]> Co-authored-by: Andres Codas <[email protected]> Co-authored-by: Sylvain Gugger <[email protected]> Co-authored-by: Francesco Saverio Zuppichini <[email protected]> Co-authored-by: Suraj Patil <[email protected]> Co-authored-by: Stas Bekman <[email protected]>

Add type hints to PLBart PyTorch

2f2d77c

gchhablani added 2 commits March 21, 2022 22:48

Merge remote-tracking branch 'upstream/master' into add_type_hints_pl…

84a4833

…bart_pt

Remove pending merge conflicts

cc4fa88

gchhablani changed the title ~~Add type hints to PLBart PyTorch~~ Add type hints to Bart and copies Mar 21, 2022

gchhablani changed the title ~~Add type hints to Bart and copies~~ Fix Bart type hints Mar 21, 2022

gchhablani force-pushed the add_type_hints_plbart_pt branch from 88683c1 to cc4fa88 Compare March 30, 2022 19:35

gchhablani added 2 commits March 31, 2022 01:25

Merge remote-tracking branch 'upstream/main' into add_type_hints_plba…

b89d2dc

…rt_pt

Fix PLBart Type Hints

aa8c73e

Rocketknight1 approved these changes Mar 31, 2022

View reviewed changes

Add changes from review

5982977

Rocketknight1 merged commit 59a9c83 into huggingface:main Apr 1, 2022

stevhliu pushed a commit to stevhliu/transformers that referenced this pull request Apr 5, 2022

Fix Bart type hints (huggingface#16297)

a9425ec

* Add type hints to PLBart PyTorch * Remove pending merge conflicts * Fix PLBart Type Hints * Add changes from review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bart type hints #16297

Fix Bart type hints #16297

gchhablani commented Mar 21, 2022

Rocketknight1 commented Mar 21, 2022

Rocketknight1 commented Mar 21, 2022

gchhablani commented Mar 21, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Mar 21, 2022 •

edited

Loading

Rocketknight1 commented Mar 22, 2022

gchhablani commented Mar 22, 2022

Rocketknight1 commented Mar 22, 2022

Rocketknight1 left a comment

Rocketknight1 Mar 31, 2022

gchhablani commented Apr 1, 2022

Rocketknight1 commented Apr 1, 2022

Fix Bart type hints #16297

Fix Bart type hints #16297

Conversation

gchhablani commented Mar 21, 2022

What does this PR do?

Rocketknight1 commented Mar 21, 2022

Rocketknight1 commented Mar 21, 2022

gchhablani commented Mar 21, 2022 • edited Loading

HuggingFaceDocBuilderDev commented Mar 21, 2022 • edited Loading

Rocketknight1 commented Mar 22, 2022

gchhablani commented Mar 22, 2022

Rocketknight1 commented Mar 22, 2022

Rocketknight1 left a comment

Choose a reason for hiding this comment

Rocketknight1 Mar 31, 2022

Choose a reason for hiding this comment

gchhablani commented Apr 1, 2022

Rocketknight1 commented Apr 1, 2022

gchhablani commented Mar 21, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Mar 21, 2022 •

edited

Loading