Skip to content

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

Merged
peri044 merged 13 commits intorelease/2.3from
dyn_validator
May 16, 2024
Merged

feat: Add validators for dynamic shapes in converter registration#2796
peri044 merged 13 commits intorelease/2.3from
dyn_validator

Conversation

@peri044
Copy link
Copy Markdown
Collaborator

@peri044 peri044 commented Apr 29, 2024 •
edited
Loading

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
gs-olive
gs-olive reviewed Apr 29, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Collaborator

narendasan commented Apr 29, 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

narendasan
narendasan reviewed Apr 29, 2024
View reviewed changes
@peri044
Copy link
Copy Markdown
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`

narendasan
narendasan reviewed Apr 30, 2024
View reviewed changes
@github-actions github-actions bot added the component: tests Issues re: Tests label May 2, 2024
gs-olive
gs-olive reviewed May 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@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

@gs-olive gs-olive mentioned this pull request May 10, 2024
Merged
gs-olive
gs-olive approved these changes May 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
peri044 added a commit that referenced this pull request May 17, 2024
@peri044
feat: Add validators for dynamic shapes in converter registration (#2796
b2dab1b 
)
@peri044 peri044 mentioned this pull request May 17, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@narendasan narendasan narendasan left review comments

+1 more reviewer

@gs-olive gs-olive gs-olive approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@peri044 @narendasan @gs-olive @facebook-github-bot