make sure inputs live on CPU for ctc decoder #2289
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mthrok
reviewed
Mar 24, 2022
| B, T, N = emissions.size() | ||
| if lengths is None: | ||
| lengths = torch.full((B,), T) | ||
|
|
||
| assert not emissions.is_cuda | ||
| assert not lengths.is_cuda |
There was a problem hiding this comment.
The logic it self looks good. Couple of suggestions.
- For a better UX, could you provide an error message that tell users what to do (as opposed to what was wrong)?
- Please perform the input validation as soon as possible, before any operation is performed. Otherwise the operation before validation could be wasteful
- This is not a written rule, but
assertis more fore internal assertion. (although it is used in L140, which I guess was missed at a review time) So can replace it withif <condition>: raise <something>?
| @@ -129,20 +129,24 @@ def __call__(self, emissions: torch.FloatTensor, lengths: Optional[torch.Tensor] | |||
|
|
|||
| Args: | |||
| emissions (torch.FloatTensor): tensor of shape `(batch, frame, num_tokens)` storing sequences of | |||
| probability distribution over labels; output of acoustic model | |||
| probability distribution over labels; output of acoustic model. It must lives on CPU. | |||
There was a problem hiding this comment.
Suggested change
| probability distribution over labels; output of acoustic model. It must lives on CPU. | |
| probability distribution over labels; output of acoustic model. It must live on CPU. |
| lengths (Tensor or None, optional): tensor of shape `(batch, )` storing the valid length of | ||
| in time axis of the output Tensor in each batch | ||
| in time axis of the output Tensor in each batch. It must lives on CPU. |
There was a problem hiding this comment.
Suggested change
| in time axis of the output Tensor in each batch. It must lives on CPU. | |
| in time axis of the output Tensor in each batch. It must live on CPU. |
|
thanks @carolineechen and @mthrok for the thorough review of my first PR! |
hwangjeff
reviewed
Mar 24, 2022
| raise ValueError('emissions must be float32.') | ||
|
|
||
| if emissions.is_cuda: | ||
| raise Exception('emissions must live on CPU.') |
There was a problem hiding this comment.
it'd be better to raise a more specific exception, e.g. RuntimeError at min
| @@ -129,20 +129,28 @@ def __call__(self, emissions: torch.FloatTensor, lengths: Optional[torch.Tensor] | |||
|
|
|||
| Args: | |||
| emissions (torch.FloatTensor): tensor of shape `(batch, frame, num_tokens)` storing sequences of | |||
| probability distribution over labels; output of acoustic model | |||
| probability distribution over labels; output of acoustic model. It must live on CPU. | |||
There was a problem hiding this comment.
perhaps just "CPU tensor of shape (batch, frame, num_tokens) storing sequences of probability distribution over labels; output of acoustic model."? sounds a little more concrete than "live on CPU"
and so forth below
| B, T, N = emissions.size() | ||
| if lengths is None: | ||
| lengths = torch.full((B,), T) | ||
|
|
||
There was a problem hiding this comment.
can remove extra whitespace
mthrok
approved these changes
Mar 29, 2022
hwangjeff
reviewed
Mar 29, 2022
| raise ValueError("emissions must be float32.") | ||
|
|
||
| if emissions.is_cuda: | ||
| raise RuntimeError("emissions must live on CPU.") |
There was a problem hiding this comment.
similar nit as above
Suggested change
| raise RuntimeError("emissions must live on CPU.") | |
| raise RuntimeError("emissions must be a CPU tensor.") |
| raise RuntimeError("emissions must live on CPU.") | ||
|
|
||
| if lengths is not None and lengths.is_cuda: | ||
| raise RuntimeError("lengths must live on CPU.") |
There was a problem hiding this comment.
and here
Suggested change
| raise RuntimeError("lengths must live on CPU.") | |
| raise RuntimeError("lengths must be a CPU tensor.") |
nateanl
approved these changes
Mar 30, 2022
|
@xiaohui-zhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Hey @xiaohui-zhang. |
xiaohui-zhang
added a commit
to xiaohui-zhang/audio
that referenced
this pull request
May 4, 2022
Summary: Addressing the issue pytorch#2274: Raise Runtime errors when the input tensors to the CTC decoder are GPU tensors since the CTC decoder only runs on CPU. Also update the data type check to use "raise" rather than "assert". --- Pull Request resolved: pytorch#2289 Reviewed By: mthrok Differential Revision: D35255630 Pulled By: xiaohui-zhang fbshipit-source-id: d6c6e88d9ad4b9690bb741557fa9a9504e60872e
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addressing the issue #2274:
Raise Runtime errors when the input tensors to the CTC decoder are GPU tensors since the CTC decoder only runs on CPU. Also update the data type check to use "raise" rather than "assert".
Pull Request resolved: #2289
GitHub Author: xiaohui-zhang xiaohuizhang@fb.com