[ONNX] Fix Gather replacement in RNN peephole #93120
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/93120
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit e4cb29f: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
release notes: onnx
titaiwangms
added a commit
that referenced
this pull request
Jan 27, 2023
ghstack-source-id: 92eca637ceb06905d7d0c0573f73358979b78588 Pull Request resolved: #93120
titaiwangms
changed the title
titaiwangms
added
topic: bug fixes
From PR: #58691, Replacing the second input of `Gather` 0 to 1 affects other innocent Nodes. In Issue #91526 onnx::range starts from 0, the 0 is changed by this mechanism, as it's shared by onnx::Gather. This PR intends to create a whole independent Constant 0 for replacement. TODO: test [ghstack-poisoned]
titaiwangms
added a commit
that referenced
this pull request
Jan 27, 2023
ghstack-source-id: 2bf91b7228e2c395d1dbd1258dedc395ce991d87 Pull Request resolved: #93120
From PR: #58691, Replacing the second input of `Gather` 0 to 1 affects other innocent Nodes. In Issue #91526 onnx::range starts from 0, the 0 is changed by this mechanism, as it's shared by onnx::Gather. This PR intends to create a whole independent Constant 0 for replacement. NOTE: The PR passes all existing RNN tests locally in case CI doesn't include RNN test. TODO: test [ghstack-poisoned]
titaiwangms
added a commit
that referenced
this pull request
Jan 30, 2023
ghstack-source-id: 4678c349c023996ae561cc95cd35901fc49c3e31 Pull Request resolved: #93120
BowenBao
approved these changes
Jan 31, 2023
From PR: #58691, Replacing the second input of `Gather` 0 to 1 affects other innocent Nodes. In Issue #91526 onnx::range starts from 0, the 0 is changed by this mechanism, as it's shared by onnx::Gather. This PR intends to create a whole independent Constant 0 for replacement. NOTE: The PR passes all existing RNN tests locally in case CI doesn't include RNN test. TODO: test [ghstack-poisoned]
titaiwangms
added a commit
that referenced
this pull request
Jan 31, 2023
ghstack-source-id: 9adc6747be516523bdaf07b7bbbce4ec12d85c41 Pull Request resolved: #93120
From PR: #58691, Replacing the second input of `Gather` 0 to 1 affects other innocent Nodes. In Issue #91526 onnx::range starts from 0, the 0 is changed by this mechanism, as it's shared by onnx::Gather. This PR intends to create a whole independent Constant 0 for replacement. NOTE: The PR passes all existing RNN tests locally in case CI doesn't include RNN test. TODO: test [ghstack-poisoned]
titaiwangms
added a commit
that referenced
this pull request
Jan 31, 2023
ghstack-source-id: feb44c5a3658c38a05df3c38cc013d5102c015cf Pull Request resolved: #93120
|
@pytorchbot merge -g |
Merge startedYour 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 |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / win-vs2019-cuda11.6-py3 / test (default, 2, 5, windows.g5.4xlarge.nvidia.gpu) Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot rebase |
|
@pytorchbot successfully started a rebase job. Check the current status here |
From PR: #58691, Replacing the second input of `Gather` 0 to 1 affects other innocent Nodes. In Issue #91526 onnx::range starts from 0, the 0 is changed by this mechanism, as it's shared with onnx::Gather. This PR intends to create a whole independent Constant 0 for replacement. NOTE: The PR passes all existing RNN tests locally in case CI doesn't include RNN test. ~~TODO: test~~ [ghstack-poisoned]
|
Successfully rebased |
pytorchmergebot
pushed a commit
that referenced
this pull request
Feb 1, 2023
ghstack-source-id: 3415695766a082843cf1218cc73dd6a3435edc96 Pull Request resolved: #93120
|
@pytorchbot merge -g |
Merge startedYour 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
From PR: #58691, Replacing the second input of
Gather0 to 1 affects other innocent Nodes. In Issue #91526 onnx::range starts from 0, the 0 is changed by this mechanism, as it's shared with onnx::Gather. This PR intends to create a whole independent Constant 0 for replacement. NOTE: The PR passes all existing RNN tests locally in case CI doesn't include RNN test.TODO: test