Skip to content

Patch for FlanT5-XXL 8bit support#20760

Merged
sgugger merged 4 commits intohuggingface:mainfrom
larsmennen:workaround-for-20287-flant5-8bit
Dec 15, 2022
Merged

Patch for FlanT5-XXL 8bit support#20760
sgugger merged 4 commits intohuggingface:mainfrom
larsmennen:workaround-for-20287-flant5-8bit

Conversation

@larsmennen
Copy link
Contributor

@larsmennen larsmennen commented Dec 14, 2022

What does this PR do?

Fixes #20287 .

In #20287 , 3 patches were proposed here: #20287 (comment)

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.
  • [n/a] 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?

@younesbelkada @sgugger

@larsmennen larsmennen changed the title Workaround for #20287: FlanT5-XXL 8bit support Patch for FlanT5-XXL 8bit support Dec 14, 2022
@larsmennen larsmennen force-pushed the workaround-for-20287-flant5-8bit branch from f90b269 to 55f8fcc Compare December 14, 2022 02:45
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Dec 14, 2022 •
edited
Loading

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

younesbelkada
younesbelkada reviewed Dec 14, 2022
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks so much for the fix @larsmennen !
I would personally advocate to focus only on T5, and we can add these patches later on if we figure out that the same issue occur for all subsidiary models! Can you revert the changes for longt5/perceiver & switch (ideally also keep the copy mechanism, so maybe add the # Copied from statements but use another model as t5 as reference (for e.g. for perceiver # Copied from transformers.src.models.longt5. ...)
Also don't forget to run the styling changes ;) (make fixup)
Thanks again!

sgugger
sgugger approved these changes Dec 14, 2022
View reviewed changes
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! This looks good to me apart from the unrelated change in perceiver.

src/transformers/models/perceiver/modeling_perceiver.py Outdated
... trainable_position_encoding_kwargs=dict(
... num_channels=256,
... index_dims=config.image_size**2,
... index_dims=config.image_size ** 2,
Copy link
Collaborator

@sgugger sgugger Dec 14, 2022

Choose a reason for hiding this comment

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

Let's leave this as is, it's not linked to this PR.

Copy link
Contributor Author

@larsmennen larsmennen Dec 14, 2022

Choose a reason for hiding this comment

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

Reverted, thanks (think that somehow came with one of the make scripts, but my env may not've been fully properly setup)

@larsmennen
Copy link
Contributor Author

larsmennen commented Dec 15, 2022

Thanks so much for the fix @larsmennen ! I would personally advocate to focus only on T5, and we can add these patches later on if we figure out that the same issue occur for all subsidiary models! Can you revert the changes for longt5/perceiver & switch (ideally also keep the copy mechanism, so maybe add the # Copied from statements but use another model as t5 as reference (for e.g. for perceiver # Copied from transformers.src.models.longt5. ...) Also don't forget to run the styling changes ;) (make fixup) Thanks again!

That makes sense! done

@sgugger sgugger merged commit b9b70b0 into huggingface:main Dec 15, 2022
gsarti added a commit to gsarti/transformers that referenced this pull request Dec 16, 2022
@gsarti
Merge branch 'main' of ssh://github.com/huggingface/transformers into…
468faa7 
… add_get_encoder_decoder_fsmt

* 'main' of ssh://github.com/huggingface/transformers: (1433 commits)
  Add Universal Segmentation class + mapping (huggingface#20766)
  Stop calling expand_1d on newer TF versions (huggingface#20786)
  Fix object detection2 (huggingface#20798)
  [Pipeline] skip feature extraction test if in `IMAGE_PROCESSOR_MAPPING` (huggingface#20790)
  Recompile `apex` in `DeepSpeed` CI image (huggingface#20788)
  Move convert_to_rgb to image_transforms module (huggingface#20784)
  Generate: use `GenerationConfig` as the basis for `.generate()` parametrization (huggingface#20388)
  Install video dependency for pipeline CI (huggingface#20777)
  Fixing object detection with `layoutlm` (huggingface#20776)
  [Pipeline] fix failing bloom `pipeline` test (huggingface#20778)
  Patch for FlanT5-XXL 8bit support (huggingface#20760)
  Install vision for TF pipeline tests (huggingface#20771)
  Even more validation. (huggingface#20762)
  Add Swin backbone (huggingface#20769)
  Install `torch-tensorrt 1.3.0` for DeepSpeed CI (huggingface#20764)
  Replaces xxx_required with requires_backends (huggingface#20715)
  [CI-Test] Fixes but also skips the mT5 tests (huggingface#20755)
  Fix attribute error problem  (huggingface#20765)
  [Tests] Improve test_attention_outputs (huggingface#20701)
  Fix missing `()` in some usage of `is_flaky` (huggingface#20749)
  ...
@younesbelkada younesbelkada mentioned this pull request Dec 22, 2022
Merged
@younesbelkada younesbelkada mentioned this pull request Jan 24, 2023
Merged
@younesbelkada younesbelkada mentioned this pull request May 21, 2023
Closed
4 tasks
@Victordongy Victordongy mentioned this pull request Aug 28, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sgugger sgugger Awaiting requested review from sgugger

1 more reviewer

@younesbelkada younesbelkada younesbelkada left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flan-T5-XXL generates non-sensical text when load_in_8bit=True

4 participants

@larsmennen @HuggingFaceDocBuilderDev @sgugger @younesbelkada