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.

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

Already on GitHub? Sign in to your account

chore: minor cleanup #6311

Merged
merged 3 commits into from
Apr 3, 2023
Merged

chore: minor cleanup #6311

merged 3 commits into from
Apr 3, 2023

Conversation

messiaen
Copy link
Contributor

What does this PR do ?

Minor cleaning, followup from #5837

@VahidooX I think I've address you comments. Let me know if I missed something.

Collection: [Note which collection this PR will affect]

Changelog

  • Add specific line by line info of high level changes in this PR.

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this 

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

  • New Feature
  • Bugfix
  • Documentation

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

@VahidooX

Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

  • Related to # (issue)

@github-actions github-actions bot added the ASR label Mar 28, 2023
VahidooX
VahidooX previously approved these changes Mar 29, 2023
Copy link
Collaborator

@VahidooX VahidooX left a comment

Choose a reason for hiding this comment

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

please add max_dim argument to the original get_initial_cache_state in the StreamingEncoder class.

VahidooX
VahidooX previously approved these changes Mar 30, 2023
@jvcrnc
Copy link

jvcrnc commented Mar 30, 2023

There is still a problem with this onnx export
File "C:\NeMo-cleanup-cache-aware\nemo\collections\asr\parts\submodules\causal_convs.py", line 142, in update_cache
cache_next[self._cache_id, :, :, -cache_keep_size:] = x[:, :, :cache_keep_size]
RuntimeError: The expanded size of the tensor (30) must match the existing size (63) at non-singleton dimension 2. Target sizes: [1, 512, 30]. Tensor sizes: [512, 63]

Before there was code in causal_convs.py to prevented this issue, but for some reason it was removed.

@VahidooX
Copy link
Collaborator

There is still a problem with this onnx export File "C:\NeMo-cleanup-cache-aware\nemo\collections\asr\parts\submodules\causal_convs.py", line 142, in update_cache cache_next[self._cache_id, :, :, -cache_keep_size:] = x[:, :, :cache_keep_size] RuntimeError: The expanded size of the tensor (30) must match the existing size (63) at non-singleton dimension 2. Target sizes: [1, 512, 30]. Tensor sizes: [512, 63]

Before there was code in causal_convs.py to prevented this issue, but for some reason it was removed.

Would you please send a reference to the exact lines which are dropped?

@@ -29,7 +29,7 @@
pass

@abstractmethod
def get_initial_cache_state(self, batch_size, dtype, device):
def get_initial_cache_state(self, batch_size, dtype, device, max_dim):

Check notice

Code scanning / CodeQL

Mismatch between signature and use of an overridden method

Overridden method signature does not match [call](1), where it is passed too few arguments. Overriding method [method ConformerEncoder.get_initial_cache_state](2) matches the call.
@VahidooX VahidooX merged commit 6fc5d02 into NVIDIA:main Apr 3, 2023
@jvcrnc
Copy link

jvcrnc commented Apr 4, 2023

There is still a problem with this onnx export File "C:\NeMo-cleanup-cache-aware\nemo\collections\asr\parts\submodules\causal_convs.py", line 142, in update_cache cache_next[self._cache_id, :, :, -cache_keep_size:] = x[:, :, :cache_keep_size] RuntimeError: The expanded size of the tensor (30) must match the existing size (63) at non-singleton dimension 2. Target sizes: [1, 512, 30]. Tensor sizes: [512, 63]
Before there was code in causal_convs.py to prevented this issue, but for some reason it was removed.

Would you please send a reference to the exact lines which are dropped?

After my comment code was added to prevent the export issue:
cache_keep_size = torch.clip(cache_keep_size, min=1, max=cache_next.size(-1))

However at runtime using this exported onnx model there is a onnx exception
Could not find an implementation for Clip(13) node with name 'Clip_907' exception code 9. I'm using onnxruntime 13.1.
Edit: Just tested with onnxruntime 14.1 here there is no exception.

@VahidooX
Copy link
Collaborator

VahidooX commented Apr 4, 2023

There is still a problem with this onnx export File "C:\NeMo-cleanup-cache-aware\nemo\collections\asr\parts\submodules\causal_convs.py", line 142, in update_cache cache_next[self._cache_id, :, :, -cache_keep_size:] = x[:, :, :cache_keep_size] RuntimeError: The expanded size of the tensor (30) must match the existing size (63) at non-singleton dimension 2. Target sizes: [1, 512, 30]. Tensor sizes: [512, 63]
Before there was code in causal_convs.py to prevented this issue, but for some reason it was removed.

Would you please send a reference to the exact lines which are dropped?

After my comment code was added to prevent the export issue: cache_keep_size = torch.clip(cache_keep_size, min=1, max=cache_next.size(-1))

However at runtime using this exported onnx model there is a onnx exception Could not find an implementation for Clip(13) node with name 'Clip_907' exception code 9. I'm using onnxruntime 13.1. Edit: Just tested with onnxruntime 14.1 here there is no exception.

So the issue is now fixed?

@jvcrnc
Copy link

jvcrnc commented Apr 5, 2023

There is still a problem with this onnx export File "C:\NeMo-cleanup-cache-aware\nemo\collections\asr\parts\submodules\causal_convs.py", line 142, in update_cache cache_next[self._cache_id, :, :, -cache_keep_size:] = x[:, :, :cache_keep_size] RuntimeError: The expanded size of the tensor (30) must match the existing size (63) at non-singleton dimension 2. Target sizes: [1, 512, 30]. Tensor sizes: [512, 63]
Before there was code in causal_convs.py to prevented this issue, but for some reason it was removed.

Would you please send a reference to the exact lines which are dropped?

After my comment code was added to prevent the export issue: cache_keep_size = torch.clip(cache_keep_size, min=1, max=cache_next.size(-1))
However at runtime using this exported onnx model there is a onnx exception Could not find an implementation for Clip(13) node with name 'Clip_907' exception code 9. I'm using onnxruntime 13.1. Edit: Just tested with onnxruntime 14.1 here there is no exception.

So the issue is now fixed?

Yes, I still have to adapt my decoder to work with the changed caching, to see if decoding works too.

@jvcrnc
Copy link

jvcrnc commented Apr 5, 2023

There is still a problem, even the built in verify_runtime fails:
[ONNXRuntimeError] : 6 : RUNTIME_EXCEPTION : Non-zero status code returned while running Reshape node. Name:'Reshape_1530' Status Message: D:\a_work\1\s\onnxruntime\core\providers\cpu\tensor\reshape_helper.h:26 onnxruntime::ReshapeHelper::ReshapeHelper i < input_shape.NumDimensions() was false. The dimension with value zero exceeds the dimension size of the input tensor.

(I had to change CUDAExecutionProvider to CPUExecutionProvider)

hsiehjackson pushed a commit to hsiehjackson/NeMo that referenced this pull request Jun 2, 2023
* chore: minor cleanup

Signed-off-by: Greg Clark <[email protected]>

* Adding docs and missing param

Signed-off-by: Greg Clark <[email protected]>

* Clip cache_keep_size in causal_convs

Signed-off-by: Greg Clark <[email protected]>

---------

Signed-off-by: Greg Clark <[email protected]>
Signed-off-by: hsiehjackson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants