Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Torchscript typing in transformer_encoder.py #4847

Merged
merged 1 commit into from Nov 8, 2022
Merged

Fix Torchscript typing in transformer_encoder.py #4847

merged 1 commit into from Nov 8, 2022

Conversation

zhxchen17
Copy link
Contributor

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
  • Did you read the contributor guideline?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes # (issue).

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 馃檭

@zhxchen17
Copy link
Contributor Author

zhxchen17 commented Nov 3, 2022 •
edited

@alexeib

I think this should fix the Torchscript compilation error introduced in #4818

meanwhile is there recommended test to run in addition to the CI tests?

@alexeib
Copy link
Contributor

alexeib commented Nov 3, 2022

some tests are still failing

@zhxchen17
Copy link
Contributor Author

some tests are still failing

@alexeib ok after some investigation, seems these build failures are introduced by PyTorch 1.13.0 version release. (which is on Oct 28)

After I install pytorch version 1.12.1 locally and rerun the tests by pytest, seems the errors from
tests/test_binaries.py::TestTranslation::test_transformer_pointer_generator and tests/test_multihead_attention.py::test_xformers_single_forward_parity are gone.

failures under tests/test_plasma_utils.py::TestPlasmaView seems irrelevant:

OSError: Could not connect to socket /tmp/tmphatl4hub

I guess we could proceed with this PR and fix other issues separately?

tugsbayasgalan
tugsbayasgalan reviewed Nov 5, 2022
View reviewed changes
fairseq/models/transformer/transformer_encoder.py
torch.tensor(src_tokens.device.type == "xla") or encoder_padding_mask.any()
)
# Torchscript doesn't handle bool Tensor correctly, so we need to work around.
if torch.jit.is_scripting():
Copy link

Choose a reason for hiding this comment

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

Can you also add torch.jit.is_tracing()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tugsbayasgalan after second thought I don't think we need to handle jit.tracing case. If you take a look at this line in the same function:

if return_all_hiddens and not torch.jit.is_scripting():

it doesn't account for tracing case as well. proactively adding a tracing guard here might not have any real use case.

@alexeib alexeib merged commit b8ac3fa into facebookresearch:main Nov 8, 2022
@alexeib
Copy link
Contributor

alexeib commented Nov 8, 2022

thanks for investigating @zhxchen17 . Shall we open another issue for the pytorch test breakage and reference it here?

@zhxchen17
Copy link
Contributor Author

thanks for investigating @zhxchen17 . Shall we open another issue for the pytorch test breakage and reference it here?

sure lemme open an issue.

@zhxchen17 zhxchen17 mentioned this pull request Nov 8, 2022
Open
cbalioglu pushed a commit that referenced this pull request Feb 23, 2023
@franznowak @zhxchen17
@tuanh208 @kit1980 @felixkreuk @alexeib @padentomasello @000Justin000 @arbabu123 @dianaml0 @Mortimerp9 @uralik @xutaima
fix imports referencing moved metrics.py file (#4840)
0338cdc 
* fix imports referencing moved metrics.py file

* Make representation computation branchless in TransformerEncoderBase (#4818)

Summary:
We want to make the computation branchless here because fairseq code may be
exported and traced for deployment purposes, and tracing mechanisms can
break the correctness for a captured program if it's dependent on input data.
In this diff we try to rewrite the code to remove one branch so that tracer
can proceed here and preserve the correct semantics of the model.

Test Plan:
CI

Reviewers:

Subscribers:

Tasks:

Tags:

* Fix Torchscript typing in transformer_encoder.py (#4847)

* Add Generative Spoken Dialogue Language Modeling (#4879)

* Update deprecated torch.qr in glow.py example (#4685)

torch.qr is deprecated for a long time and is being removed by pytorch/pytorch#70989.

This PR makes the example compatible with new and old PyTorch versions.

* Emotion Conversion Paper Open Source (#4895)

* data2vec v2.0 (#4903)

data2v2c 2.0
Co-authored-by: Arun Babu <arbabu@fb.com>
Co-authored-by: Wei-Ning Hsu <wnhsu@csail.mit.edu>

* remove missing config entries when loading task from checkpoint (#4905)

* make apex optional (#4906)

* Add file to generate manifests for stop dataset. (#4891)

* Update STOP dataset README to include proper link. (#4892)

* Update README.md (#4893)

* using foreach to reduce kernel (#4904)

* using foreach to reduce kernel

* set reproducibility to looser threshold

* revert optimzer

* update

* update

* update

* update

* update

* update

* update

Co-authored-by: juntengjia <juntengjia@fb.com>

* Update README.md to add data2vec blog post (#4913)

* Update README.md

* Update config to fix circleci failure (#4949)

https://app.circleci.com/pipelines/github/fairinternal/fairseq-py/12635/workflows/3befbae2-79c4-458d-9fc4-aad4484183b4/jobs/26767

* Generative Spoken Dialogue Language Modeling Paper Open Source (#4957)

* wav2vec2_laser (#4968)

* ASR BLEU tool copied from ust branch into main (#4914)

* Add transcript option for asr-bleu (#4981)

---------

Co-authored-by: zhxchen17 <zhxchen17@outlook.com>
Co-authored-by: zhxchen17 <zhxchen17@fb.com>
Co-authored-by: Nguyen Tu Anh <nguyentuanh208@gmail.com>
Co-authored-by: Sergii Dymchenko <kit1980@gmail.com>
Co-authored-by: Felix Kreuk <felixkreuk@gmail.com>
Co-authored-by: Alexei Baevski <alexei.b@gmail.com>
Co-authored-by: padentomasello <pdtomasello@gmail.com>
Co-authored-by: Junteng Jia <juntengjia@hotmail.com>
Co-authored-by: juntengjia <juntengjia@fb.com>
Co-authored-by: arbabu123 <arbabu@fb.com>
Co-authored-by: dianaml0 <82468439+dianaml0@users.noreply.github.com>
Co-authored-by: Pierre Andrews <mortimer@fb.com>
Co-authored-by: Ilia Kulikov <kulikov@cs.nyu.edu>
Co-authored-by: Xutai Ma <xutaima@gmail.com>
lwb2099 pushed a commit to lwb2099/fairseq that referenced this pull request Apr 26, 2023
@zhxchen17
Fix Torchscript typing in transformer_encoder.py (facebookresearch#4847)
374bd75
lwb2099 pushed a commit to lwb2099/fairseq that referenced this pull request Apr 26, 2023
@franznowak @zhxchen17
@tuanh208 @kit1980 @felixkreuk @alexeib @padentomasello @000Justin000 @arbabu123 @dianaml0 @Mortimerp9 @uralik @xutaima
fix imports referencing moved metrics.py file (facebookresearch#4840)
9ff8560 
* fix imports referencing moved metrics.py file

* Make representation computation branchless in TransformerEncoderBase (facebookresearch#4818)

Summary:
We want to make the computation branchless here because fairseq code may be
exported and traced for deployment purposes, and tracing mechanisms can
break the correctness for a captured program if it's dependent on input data.
In this diff we try to rewrite the code to remove one branch so that tracer
can proceed here and preserve the correct semantics of the model.

Test Plan:
CI

Reviewers:

Subscribers:

Tasks:

Tags:

* Fix Torchscript typing in transformer_encoder.py (facebookresearch#4847)

* Add Generative Spoken Dialogue Language Modeling (facebookresearch#4879)

* Update deprecated torch.qr in glow.py example (facebookresearch#4685)

torch.qr is deprecated for a long time and is being removed by pytorch/pytorch#70989.

This PR makes the example compatible with new and old PyTorch versions.

* Emotion Conversion Paper Open Source (facebookresearch#4895)

* data2vec v2.0 (facebookresearch#4903)

data2v2c 2.0
Co-authored-by: Arun Babu <arbabu@fb.com>
Co-authored-by: Wei-Ning Hsu <wnhsu@csail.mit.edu>

* remove missing config entries when loading task from checkpoint (facebookresearch#4905)

* make apex optional (facebookresearch#4906)

* Add file to generate manifests for stop dataset. (facebookresearch#4891)

* Update STOP dataset README to include proper link. (facebookresearch#4892)

* Update README.md (facebookresearch#4893)

* using foreach to reduce kernel (facebookresearch#4904)

* using foreach to reduce kernel

* set reproducibility to looser threshold

* revert optimzer

* update

* update

* update

* update

* update

* update

* update

Co-authored-by: juntengjia <juntengjia@fb.com>

* Update README.md to add data2vec blog post (facebookresearch#4913)

* Update README.md

* Update config to fix circleci failure (facebookresearch#4949)

https://app.circleci.com/pipelines/github/fairinternal/fairseq-py/12635/workflows/3befbae2-79c4-458d-9fc4-aad4484183b4/jobs/26767

* Generative Spoken Dialogue Language Modeling Paper Open Source (facebookresearch#4957)

* wav2vec2_laser (facebookresearch#4968)

* ASR BLEU tool copied from ust branch into main (facebookresearch#4914)

* Add transcript option for asr-bleu (facebookresearch#4981)

---------

Co-authored-by: zhxchen17 <zhxchen17@outlook.com>
Co-authored-by: zhxchen17 <zhxchen17@fb.com>
Co-authored-by: Nguyen Tu Anh <nguyentuanh208@gmail.com>
Co-authored-by: Sergii Dymchenko <kit1980@gmail.com>
Co-authored-by: Felix Kreuk <felixkreuk@gmail.com>
Co-authored-by: Alexei Baevski <alexei.b@gmail.com>
Co-authored-by: padentomasello <pdtomasello@gmail.com>
Co-authored-by: Junteng Jia <juntengjia@hotmail.com>
Co-authored-by: juntengjia <juntengjia@fb.com>
Co-authored-by: arbabu123 <arbabu@fb.com>
Co-authored-by: dianaml0 <82468439+dianaml0@users.noreply.github.com>
Co-authored-by: Pierre Andrews <mortimer@fb.com>
Co-authored-by: Ilia Kulikov <kulikov@cs.nyu.edu>
Co-authored-by: Xutai Ma <xutaima@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tugsbayasgalan tugsbayasgalan tugsbayasgalan approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zhxchen17 @alexeib @tugsbayasgalan @facebook-github-bot