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.

Sign up for GitHub

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

Jump to bottom

chore: fix ValueRanges computation in symbolic nodes #2918

Merged
merged 4 commits into from
Jun 17, 2024
Merged

chore: fix ValueRanges computation in symbolic nodes #2918

merged 4 commits into from
Jun 17, 2024

Conversation

peri044
Copy link
Collaborator

@peri044 peri044 commented Jun 13, 2024 •
edited
Loading

Description

This PR fixes the following:

  1. Sometimes there are subgraphs which receive a SymInt node (so*s1) as input ( that's the product of two other SymInt s0 and s1). However, this node (so*s1) does not have ValueRanges computed automatically but it just has the data for s0 and s1

  2. It also adds support for operator.mul. This operator gets added in the added test_linear test case where it computes product of first two dimensions (which are SymInt nodes). Since SymInt nodes are handled as shape tensors in TensorRT, our existing converter for mul operators can be used for this operator.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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: tests Issues re: Tests 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 Jun 13, 2024
@peri044 peri044 requested a review from narendasan June 13, 2024 19:36
@peri044 peri044 mentioned this pull request Jun 13, 2024
Closed
@peri044
Copy link
Collaborator Author

peri044 commented Jun 14, 2024

Context: pytorch/pytorch#128640

gs-olive
gs-olive reviewed Jun 14, 2024
View reviewed changes
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 - added two clarifying comments

py/torch_tensorrt/dynamo/conversion/aten_ops_converters.py Show resolved Hide resolved
py/torch_tensorrt/dynamo/partitioning/common.py Show resolved Hide resolved
gs-olive
gs-olive approved these changes Jun 14, 2024
View reviewed changes
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, pending CI

@peri044 peri044 merged commit f422159 into main Jun 17, 2024
55 of 61 checks passed
@chohk88 chohk88 mentioned this pull request Jul 3, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@narendasan narendasan Awaiting requested review from narendasan

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.
3 participants
@peri044 @gs-olive @facebook-github-bot