Skip to content

[dynamo] Reserve the tensorrt backend name for torch-tensorrt#94632

Closed
narendasan wants to merge 3 commits intopytorch:masterfrom
narendasan:reserve_tensorrt_dynamo
Closed

[dynamo] Reserve the tensorrt backend name for torch-tensorrt#94632
narendasan wants to merge 3 commits intopytorch:masterfrom
narendasan:reserve_tensorrt_dynamo

Conversation

@narendasan
Copy link
Copy Markdown
Contributor

@narendasan narendasan commented Feb 10, 2023

In PR #93822 the fx2trt backend was removed which registered the tensorrt backend names to point to fx2trt / torch_tensorrt and move the name to onnxrt. We want to reserve the name tensorrt for torch_tensorrt to prevent any confusion but due to code-freeze we cannot complete the integration and set up testing for the next release. So we propose leaving out the tensorrt name until we can set up the backend and testing for it.

cc @mlazos @soumith @voznesenskym @yanboliang @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @desertfire

@pytorch-bot
Copy link
Copy Markdown

pytorch-bot bot commented Feb 10, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/94632

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 0156a81:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Feb 10, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: narendasan / name: Naren Dasan (29972a9)

@pytorch-bot pytorch-bot bot added the release notes: onnx torch.onnx related changes that should show up in the release notes label Feb 10, 2023
Copy link
Copy Markdown
Contributor

@frank-wei frank-wei left a comment

Choose a reason for hiding this comment

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

LGTM! We'd keep tensorrt for torch-tensorrt.

@narendasan
Copy link
Copy Markdown
Contributor Author

@frank-wei How can i fix the CI issues?

@frank-wei
Copy link
Copy Markdown
Contributor

@narendasan looks like not this PR's fault. Can you rebase?

@huydhn
Copy link
Copy Markdown
Contributor

huydhn commented Feb 22, 2023

@pytorchbot rebase

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Successfully rebased reserve_tensorrt_dynamo onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout reserve_tensorrt_dynamo && git pull --rebase)

@frank-wei
Copy link
Copy Markdown
Contributor

The failed tests are flaky which are not related to this PR.

@frank-wei
Copy link
Copy Markdown
Contributor

@pytorchbot rebase

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Successfully rebased reserve_tensorrt_dynamo onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout reserve_tensorrt_dynamo && git pull --rebase)

@frank-wei
Copy link
Copy Markdown
Contributor

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 26, 2023
@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@gottbrath
Copy link
Copy Markdown
Contributor

@seemethere @malfet @atalman I know this is coming late in the process but can we review this for Cherry picking into the release branch. cc: @frank-wei

@malfet
Copy link
Copy Markdown
Contributor

malfet commented Feb 27, 2023

At the very quick glance at the PR, title does not correspond to what the code is actually doing (I.e. the registration is commented out)

@narendasan
Copy link
Copy Markdown
Contributor Author

We didnt want to introduce a significant functional component this close to release. There doesnt seem to be a central registry for backend names so this was the closest I could think of to reserving the name i.e removing the tensorrt -> ONNX-RT mapping and putting in a placeholder file for tensorrt -> torch-trt

@malfet malfet added this to the 2.0.0 milestone Feb 27, 2023
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 5, 2023
In PR #93822 the `fx2trt` backend was removed which registered the `tensorrt` backend names to point to `fx2trt` / `torch_tensorrt` and move the name to `onnxrt`. We want to reserve the name `tensorrt` for `torch_tensorrt` to prevent any confusion but due to code-freeze we cannot complete the integration and set up testing for the next release. So we propose leaving out the `tensorrt` name until we can set up the backend and testing for it.

Pull Request resolved: pytorch/pytorch#94632
Approved by: https://github.com/frank-wei
pruthvistony added a commit to ROCm/pytorch that referenced this pull request May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo open source release notes: onnx torch.onnx related changes that should show up in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants