Assert if padding mask type is unexpected (#86353) #87106

zrphercule · 2022-10-17T18:32:00Z

Summary:
Pull Request resolved: #86353

Fix the issue described in
#86120

Test Plan: buck test mode/opt caffe2/test:test_transformers -- test_train_with_long_type_pad

Differential Revision: D40129968

pytorch-bot · 2022-10-17T18:32:03Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/87106

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 7ed2fcf:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot · 2022-10-17T18:32:30Z

This pull request was exported from Phabricator. Differential Revision: D40129968

facebook-github-bot · 2022-10-17T19:34:19Z

This pull request was exported from Phabricator. Differential Revision: D40129968

facebook-github-bot · 2022-10-17T19:59:34Z

This pull request was exported from Phabricator. Differential Revision: D40129968

facebook-github-bot · 2022-10-17T20:51:25Z

This pull request was exported from Phabricator. Differential Revision: D40129968

… & unit test (pytorch#87106) Summary: Pull Request resolved: pytorch#87106 Pull Request resolved: pytorch#86353 Fix the issue described in pytorch#86120 Test Plan: buck test mode/opt caffe2/test:test_transformers -- test_train_with_long_type_pad Differential Revision: D40129968 fbshipit-source-id: 2be6869f2f8a39c780fdfaf3fb16034c4726a571

malfet

LGTM, but please consider the posted feedback

malfet · 2022-10-19T05:51:49Z

torch/nn/modules/transformer.py

@@ -443,6 +448,11 @@ def forward(self, src: Tensor, src_mask: Optional[Tensor] = None,
        """

        # see Fig. 1 of https://arxiv.org/pdf/2002.04745v1.pdf


Perhaps put the assertion above the comment (as Fig1 is unlikely to have anything to do with checks below

malfet · 2022-10-19T05:53:26Z

torch/nn/modules/transformer.py

+            if src_key_padding_mask.dtype != torch.bool:
+                if not torch.is_floating_point(src_key_padding_mask):
+                    raise AssertionError(
+                        "only bool and floating type of key_padding_mask is supported")


English is not my native, but isn't assertion discusses plural types, isn't it?

Suggested change

"only bool and floating type of key_padding_mask is supported")

"only bool and floating types of key_padding_mask are supported")

malfet · 2022-10-19T06:01:25Z

torch/nn/modules/transformer.py

+        if src_key_padding_mask is not None:
+            if src_key_padding_mask.dtype != torch.bool:
+                if not torch.is_floating_point(src_key_padding_mask):


Please consider dismantling pyramid of doom by a bit

Suggested change

if src_key_padding_mask is not None:

if src_key_padding_mask.dtype != torch.bool:

if not torch.is_floating_point(src_key_padding_mask):

if src_key_padding_mask is not None:

_skpm_dtype = src_key_padding_mask.dtype

if _skpm_dtype != torch.bool and not _skpm_dtype.is_floating_point:

malfet · 2022-10-19T06:04:25Z

torch/nn/modules/activation.py

+        if key_padding_mask is not None:
+            if key_padding_mask.dtype != torch.bool:
+                if not torch.is_floating_point(key_padding_mask):
+                    raise AssertionError(
+                        "only bool and floating type of key_padding_mask is supported")


You are adding the same check thrice, which seems to warrant a convenience function

def _assert_if_tensor_is_not_float_or_bool(t: Optional[torch.Tensor], name: str) -> None: if t is None: return if t.dtype != torch.bool and not t.dtype.is_floating_point: raise AssertionError(f"only bool and floating types of {name} are supported")

That is a good idea, but dont want to add new helper functions in this diff anymore in case it will break torch script :(

mikekgfb · 2022-10-19T06:05:50Z

@pytorchbot merge

malfet · 2022-10-19T06:07:09Z

Also, please change title, as this PR is not 1.13 cherry pick, it simply targets master.
Something like: Assert if padding mask type is unexpected?

pytorchmergebot · 2022-10-19T06:07:15Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2022-10-19T06:07:44Z

The merge job was canceled. If you believe this is a mistake,then you can re trigger it through pytorch-bot.

facebook-github-bot · 2022-10-19T19:38:31Z

This pull request was exported from Phabricator. Differential Revision: D40129968

facebook-github-bot · 2022-10-19T20:05:40Z

This pull request was exported from Phabricator. Differential Revision: D40129968

… & unit test (pytorch#87106) Summary: Pull Request resolved: pytorch#87106 Pull Request resolved: pytorch#86353 Fix the issue described in pytorch#86120 Test Plan: buck test mode/opt caffe2/test:test_transformers -- test_train_with_long_type_pad Reviewed By: malfet Differential Revision: D40129968 fbshipit-source-id: 4a3ac5750082b9b1806003e36b04edd96c36f538

facebook-github-bot · 2022-10-19T22:45:11Z

This pull request was exported from Phabricator. Differential Revision: D40129968

facebook-github-bot · 2022-10-19T23:36:21Z

This pull request was exported from Phabricator. Differential Revision: D40129968

malfet · 2022-10-20T00:09:30Z

torch/nn/modules/activation.py

@@ -1062,6 +1061,11 @@ def forward(self, query: Tensor, key: Tensor, value: Tensor, key_padding_mask: O
            `batch_first` argument is ignored for unbatched inputs.
        """
        is_batched = query.dim() == 3
+        if key_padding_mask is not None:
+            _skpm_dtype = key_padding_mask.dtype
+            if _skpm_dtype != torch.bool and not torch.is_floating_point(key_padding_mask):


Why call a function rather than query the property directly?

Suggested change

if _skpm_dtype != torch.bool and not torch.is_floating_point(key_padding_mask):

if _skpm_dtype != torch.bool and not _skpm_dtype.is_floating_point:

@malfet I tagged you in phabricator. It is because some scripted tensors have really weird tensor dtype, like an "int" instead of torch.int. I pasted some links in pharicator, and msged you directly ;)

malfet · 2022-10-20T00:10:37Z

torch/nn/modules/activation.py

@@ -1062,6 +1061,11 @@ def forward(self, query: Tensor, key: Tensor, value: Tensor, key_padding_mask: O
            `batch_first` argument is ignored for unbatched inputs.
        """
        is_batched = query.dim() == 3
+        if key_padding_mask is not None:
+            _skpm_dtype = key_padding_mask.dtype


Variable name supposed to start with first letter of long variable name, i.e. for key_padding_mask is should be _kpm_dtype

Suggested change

_skpm_dtype = key_padding_mask.dtype

_kpm_dtype = key_padding_mask.dtype

Summary: Pull Request resolved: pytorch#87106 Pull Request resolved: pytorch#86353 Fix the issue described in pytorch#86120 Test Plan: buck test mode/opt caffe2/test:test_transformers -- test_train_with_long_type_pad Reviewed By: malfet Differential Revision: D40129968 fbshipit-source-id: d7530d8c4548b9d3a3ef8b9740c93bee000ed809

facebook-github-bot · 2022-10-20T00:29:31Z

This pull request was exported from Phabricator. Differential Revision: D40129968

zrphercule · 2022-10-20T05:31:27Z

@pytorchbot merge

pytorchmergebot · 2022-10-20T05:35:48Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2022-10-20T05:35:52Z

Merge failed

Reason: 3 additional jobs have failed, first few of them are: trunk ,trunk / linux-focal-rocm5.2-py3.7 / test (default, 2, 2, linux.rocm.gpu) ,trunk / cuda11.6-py3.10-gcc7-sm86 / test (default, 1, 4, linux.g5.4xlarge.nvidia.gpu)

Details for Dev Infra team

Raised by workflow job

ZainRizvi · 2022-10-20T16:00:02Z

@pytorchmergebot merge

pytorchmergebot · 2022-10-20T16:01:49Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Summary: Pull Request resolved: pytorch#87106 Pull Request resolved: pytorch#86353 Fix the issue described in pytorch#86120 Test Plan: buck test mode/opt caffe2/test:test_transformers -- test_train_with_long_type_pad Reviewed By: malfet Differential Revision: D40129968 fbshipit-source-id: d7530d8c4548b9d3a3ef8b9740c93bee000ed809

Summary: Pull Request resolved: #87106 Pull Request resolved: #86353 Fix the issue described in #86120 Test Plan: buck test mode/opt caffe2/test:test_transformers -- test_train_with_long_type_pad Reviewed By: malfet Differential Revision: D40129968 fbshipit-source-id: d7530d8c4548b9d3a3ef8b9740c93bee000ed809

zrphercule requested review from albanD and jbschlosser as code owners October 17, 2022 18:32

pytorch-bot bot added the release notes: nn release notes category label Oct 17, 2022

facebook-github-bot added the fb-exported label Oct 17, 2022

albanD removed their request for review October 17, 2022 19:21

zrphercule force-pushed the export-D40129968 branch from 4d9efc3 to 9c54c65 Compare October 17, 2022 19:34

zrphercule force-pushed the export-D40129968 branch from 9c54c65 to 2fa3431 Compare October 17, 2022 19:59

zrphercule force-pushed the export-D40129968 branch from 2fa3431 to ef641ac Compare October 17, 2022 20:51

zrphercule requested a review from malfet October 18, 2022 06:15

malfet approved these changes Oct 19, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 19, 2022

malfet added the topic: bc breaking topic category label Oct 19, 2022

zrphercule force-pushed the export-D40129968 branch from ef641ac to 0f7a52b Compare October 19, 2022 19:38

zrphercule force-pushed the export-D40129968 branch from 0f7a52b to dbf9fb2 Compare October 19, 2022 20:05

zrphercule changed the title ~~[PT1.13 cherry pick]Fix Transformer's issue when padding is long type & unit test (#86353)~~ Assert if padding mask type is unexpected (#86353) Oct 19, 2022

zrphercule force-pushed the export-D40129968 branch from dbf9fb2 to 14af130 Compare October 19, 2022 22:45

zrphercule force-pushed the export-D40129968 branch from 14af130 to 47a676f Compare October 19, 2022 23:36

malfet reviewed Oct 20, 2022

View reviewed changes

zrphercule added this to the 1.13.0 milestone Oct 20, 2022

zrphercule force-pushed the export-D40129968 branch from 47a676f to 7ed2fcf Compare October 20, 2022 00:29

pytorchmergebot added the Merged label Oct 20, 2022

pytorchmergebot closed this in 4b757f4 Oct 20, 2022

zrphercule mentioned this pull request Oct 20, 2022

Assert if padding mask type is unexpected (#87106) #87384

Merged

zrphercule deleted the export-D40129968 branch October 20, 2022 18:09

zrphercule mentioned this pull request Oct 20, 2022

[v.1.13.0] Release Tracker #86312

Closed

malfet mentioned this pull request Nov 23, 2022

Add c10:: namespace in front of optional #89605

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assert if padding mask type is unexpected (#86353) #87106

Assert if padding mask type is unexpected (#86353) #87106

zrphercule commented Oct 17, 2022

pytorch-bot bot commented Oct 17, 2022 •

edited

Loading

facebook-github-bot commented Oct 17, 2022

facebook-github-bot commented Oct 17, 2022

facebook-github-bot commented Oct 17, 2022

facebook-github-bot commented Oct 17, 2022

malfet left a comment

malfet Oct 19, 2022

malfet Oct 19, 2022

malfet Oct 19, 2022

malfet Oct 19, 2022

zrphercule Oct 19, 2022

mikekgfb commented Oct 19, 2022

malfet commented Oct 19, 2022

pytorchmergebot commented Oct 19, 2022

pytorchmergebot commented Oct 19, 2022

facebook-github-bot commented Oct 19, 2022

facebook-github-bot commented Oct 19, 2022

facebook-github-bot commented Oct 19, 2022

facebook-github-bot commented Oct 19, 2022

malfet Oct 20, 2022

zrphercule Oct 20, 2022

malfet Oct 20, 2022

zrphercule Oct 20, 2022

facebook-github-bot commented Oct 20, 2022

zrphercule commented Oct 20, 2022

pytorchmergebot commented Oct 20, 2022

pytorchmergebot commented Oct 20, 2022

ZainRizvi commented Oct 20, 2022

pytorchmergebot commented Oct 20, 2022

		@@ -443,6 +448,11 @@ def forward(self, src: Tensor, src_mask: Optional[Tensor] = None,
		"""

		# see Fig. 1 of https://arxiv.org/pdf/2002.04745v1.pdf

	"only bool and floating type of key_padding_mask is supported")
	"only bool and floating types of key_padding_mask are supported")

	if _skpm_dtype != torch.bool and not torch.is_floating_point(key_padding_mask):
	if _skpm_dtype != torch.bool and not _skpm_dtype.is_floating_point:

	_skpm_dtype = key_padding_mask.dtype
	_kpm_dtype = key_padding_mask.dtype

Assert if padding mask type is unexpected (#86353) #87106

Assert if padding mask type is unexpected (#86353) #87106

Conversation

zrphercule commented Oct 17, 2022

pytorch-bot bot commented Oct 17, 2022 • edited Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/87106

✅ No Failures

facebook-github-bot commented Oct 17, 2022

facebook-github-bot commented Oct 17, 2022

facebook-github-bot commented Oct 17, 2022

facebook-github-bot commented Oct 17, 2022

malfet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikekgfb commented Oct 19, 2022

malfet commented Oct 19, 2022

pytorchmergebot commented Oct 19, 2022

Merge started

pytorchmergebot commented Oct 19, 2022

facebook-github-bot commented Oct 19, 2022

facebook-github-bot commented Oct 19, 2022

facebook-github-bot commented Oct 19, 2022

facebook-github-bot commented Oct 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 20, 2022

zrphercule commented Oct 20, 2022

pytorchmergebot commented Oct 20, 2022

Merge started

pytorchmergebot commented Oct 20, 2022

Merge failed

ZainRizvi commented Oct 20, 2022

pytorchmergebot commented Oct 20, 2022

Merge started

pytorch-bot bot commented Oct 17, 2022 •

edited

Loading