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

feat: Add validators for dynamic shapes in converter registration #2796

Merged
merged 13 commits into from May 16, 2024

Conversation

peri044
Copy link
Collaborator

@peri044 peri044 commented Apr 29, 2024

Description

This PR does the following

  1. The default behavior is all converters are marked dynamic=False.
  2. We conservatively/explicitly mark dynamic=True for the nodes we know that support dynamic shapes.
  3. If a converter is marked dynamic=True, nothing happens. Both static and dynamic shapes are supported.
  4. If a converter is marked dynamic=False, it checks if the node received dynamic inputs. If it received dynamic inputs, we disable this converter and the node will fallback to PyTorch. If it received static inputs, compilation proceeds fine with no warnings.
  5. For static inputs to the converter, this flag doesn't affect anything.

Type of change

Please delete options that are not relevant and/or add your own.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project (You can use the linters)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas and hacks
  • I have made corresponding changes to the documentation
  • I have added tests to verify my fix or my feature
  • New and existing unit tests pass locally with my changes
  • I have added the relevant labels to my PR in so that relevant reviewers are notified

@github-actions github-actions bot added component: conversion Issues re: Conversion stage component: api [Python] Issues re: Python API component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths labels Apr 29, 2024
@github-actions github-actions bot requested a review from gs-olive April 29, 2024 22:54
@peri044 peri044 requested a review from narendasan April 29, 2024 22:56
Copy link
Collaborator

@gs-olive gs-olive left a comment

Choose a reason for hiding this comment

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

Overall looks great, added a few suggestions

@narendasan
Copy link
Collaborator

WARNING:torch_tensorrt.dynamo.conversion._ConverterRegistry:The converter for node aten.convolution.default received dynamic shaped inputs but the static version of the converter is being used. Please report this issue at https://github.com/pytorch/TensorRT/issues

This is a severe warning, it implies that it probably wont work. Is the intention for us to treat it as something that probably would work but might fail like (70/30) or like (20/80) pass/fail? If its the latter this should be an error, if its the former, we need to adjust the warning

@peri044
Copy link
Collaborator Author

peri044 commented Apr 30, 2024

WARNING:torch_tensorrt.dynamo.conversion._ConverterRegistry:The converter for node aten.convolution.default received dynamic shaped inputs but the static version of the converter is being used. Please report this issue at https://github.com/pytorch/TensorRT/issues

This is a severe warning, it implies that it probably wont work. Is the intention for us to treat it as something that probably would work but might fail like (70/30) or like (20/80) pass/fail? If its the latter this should be an error, if its the former, we need to adjust the warning

Intention is "would work but might fail". How about the following message ?

The converter for node aten.convolution.default received dynamic shaped inputs although it was designed for static inputs. This shouldn't likely cause issues unless there are some dimensions which are dynamic (excluding the batch). If you encounter any issues, please post at https://github.com/pytorch/TensorRT/issues`

@github-actions github-actions bot added the component: tests Issues re: Tests label May 2, 2024
Copy link
Collaborator

@gs-olive gs-olive left a comment

Choose a reason for hiding this comment

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

Overall looks good to me - very useful feature for the Converter Registry. Added a few comments + suggestions

py/torch_tensorrt/dynamo/conversion/_ConverterRegistry.py Outdated Show resolved Hide resolved
py/torch_tensorrt/dynamo/conversion/aten_ops_converters.py Outdated Show resolved Hide resolved
py/torch_tensorrt/dynamo/_defaults.py Show resolved Hide resolved
@gs-olive gs-olive mentioned this pull request May 10, 2024
Copy link
Collaborator

@gs-olive gs-olive left a comment

Choose a reason for hiding this comment

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

Looks good to me

@peri044 peri044 merged commit 722457b into release/2.3 May 16, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed component: api [Python] Issues re: Python API component: conversion Issues re: Conversion stage component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths component: tests Issues re: Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants