Skip to content

[jit] Fix flipped PackedSequence outputs in script #32955

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
Closed
wants to merge 12 commits into from
Closed

[jit] Fix flipped PackedSequence outputs in script #32955

wants to merge 12 commits into from

Conversation

driazati
Copy link
Contributor

@driazati driazati commented Feb 4, 2020 •
edited
Loading

Stacked PRs

Fixes #32605

Differential Revision: D20165514

@driazati driazati requested a review from apaszke as a code owner February 4, 2020 00:25
@driazati driazati mentioned this pull request Feb 4, 2020
Closed
@driazati driazati changed the title update [jit] Fix flipped PackedSequence outputs in script Feb 4, 2020
@driazati driazati requested a review from eellison February 11, 2020 23:23
eellison
eellison reviewed Feb 11, 2020
View reviewed changes
torch/nn/utils/rnn.py
@@ -61,37 +61,6 @@ class PackedSequence(PackedSequence_):
#
# See the note above in doc string (starting with ":attr:`data` can be on
# arbitrary device...").

def __new__(cls, data, batch_sizes=None, sorted_indices=None, unsorted_indices=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you have edited construction of PackedSequence within our library, but doesn't removing this break BC for any user code that uses this API?

facebook-github-bot pushed a commit that referenced this pull request Feb 13, 2020
@facebook-github-bot
[jit] Support properties on Device (#32953)
f61b45f 
Summary:
Stacked PRs
 * #32955 - [jit] Fix flipped PackedSequence outputs in script
 * **#32953 - [jit] Support properties on `Device`**

PyTorch devices have a `index` and `type` property. This PR adds support for both to TorchScript
](https://our.intern.facebook.com/intern/diff/19849320/)
Pull Request resolved: #32953

Pulled By: driazati

Differential Revision: D19849320

fbshipit-source-id: ce845258c6110058dd9ea1f759ef74b7ed2e786e
@driazati driazati changed the base branch from driazati/devtype to master February 13, 2020 21:06
@driazati driazati requested a review from eellison February 21, 2020 21:00
eellison
eellison approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

I guess this is a fine short-term fix. But ignoring __new__ style constructors means that this a silently-wrong semantics issue (& why we've had all these confusing LSTM bugs). When classes have __new__ constructors we error. Can you file an issue for this ?

cc @jamesr66a

torch/csrc/jit/script/sugared_value.cpp Outdated
@@ -108,6 +108,19 @@ std::shared_ptr<SugaredValue> SimpleValue::attr(
}
}

if (value_->type()->isSubtypeOf(DeviceObjType::get())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is no longer meant to be here

@facebook-github-bot
Copy link
Contributor

@driazati merged this pull request in 2b9fa4a.

@nairbv
Copy link
Collaborator

nairbv commented Feb 27, 2020

Test failures on master look relevant:

Feb 27 05:25:12 ERROR [0.027s]: test_script_pack_padded_sequence (test_jit.TestScript)
Feb 27 05:25:12 ----------------------------------------------------------------------
Feb 27 05:25:12 Traceback (most recent call last):
Feb 27 05:25:12   File "/var/lib/jenkins/workspace/test/test_jit.py", line 10873, in test_script_pack_padded_sequence
Feb 27 05:25:12     script_seq, script_lengths = scripted_pack_padded_seq(x, seq_lens)
Feb 27 05:25:12   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 88, in prof_func_call
Feb 27 05:25:12     return prof_callable(func_call, *args, **kwargs)
Feb 27 05:25:12   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 85, in prof_callable
Feb 27 05:25:12     return callable(*args, **kwargs)
Feb 27 05:25:12 RuntimeError: mutableType(from) && mutableType(to) INTERNAL ASSERT FAILED at /var/lib/jenkins/workspace/torch/csrc/jit/passes/alias_analysis.cpp:788, please report a bug to PyTorch.

will unland.

@driazati driazati reopened this Feb 27, 2020
hczhu pushed a commit that referenced this pull request Feb 28, 2020
[jit] Fix flipped PackedSequence outputs in script (#32955)
34664b4 
Summary:
Stacked PRs
 * **#32955 - [jit] Fix flipped PackedSequence outputs in script**
 * #32953 - [jit] Support properties on `Device`

Fixes #32605
](https://our.intern.facebook.com/intern/diff/20103905/)
Pull Request resolved: #32955

Pulled By: driazati

Differential Revision: D20103905

fbshipit-source-id: 84081213ed214846e563b9f05bcab0210bb1a71b
ttumiel pushed a commit to ttumiel/pytorch that referenced this pull request Mar 4, 2020
@ttumiel
[jit] Support properties on Device (pytorch#32953)
f0d0935 
Summary:
Stacked PRs
 * pytorch#32955 - [jit] Fix flipped PackedSequence outputs in script
 * **pytorch#32953 - [jit] Support properties on `Device`**

PyTorch devices have a `index` and `type` property. This PR adds support for both to TorchScript
](https://our.intern.facebook.com/intern/diff/19849320/)
Pull Request resolved: pytorch#32953

Pulled By: driazati

Differential Revision: D19849320

fbshipit-source-id: ce845258c6110058dd9ea1f759ef74b7ed2e786e
ttumiel pushed a commit to ttumiel/pytorch that referenced this pull request Mar 4, 2020
@ttumiel
[jit] Fix flipped PackedSequence outputs in script (pytorch#32955)
b2e4778 
Summary:
Stacked PRs
 * **pytorch#32955 - [jit] Fix flipped PackedSequence outputs in script**
 * pytorch#32953 - [jit] Support properties on `Device`

Fixes pytorch#32605
](https://our.intern.facebook.com/intern/diff/20103905/)
Pull Request resolved: pytorch#32955

Pulled By: driazati

Differential Revision: D20103905

fbshipit-source-id: 84081213ed214846e563b9f05bcab0210bb1a71b
ttumiel pushed a commit to ttumiel/pytorch that referenced this pull request Mar 4, 2020
@ttumiel
[jit] Fix flipped PackedSequence outputs in script (pytorch#32955)
df9b841 
Summary:
Stacked PRs
 * **pytorch#32955 - [jit] Fix flipped PackedSequence outputs in script**
 * pytorch#32953 - [jit] Support properties on `Device`

Fixes pytorch#32605

Pull Request resolved: pytorch#32955

Pulled By: driazati

Differential Revision: D20165514

fbshipit-source-id: a130c438b40e51ec27d36f021b0dc7869570aa6a
@facebook-github-bot facebook-github-bot deleted the driazati/pack branch July 13, 2020 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eellison eellison eellison approved these changes

@apaszke apaszke Awaiting requested review from apaszke

Assignees
No one assigned
Labels
Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[jit] PackedSequence flips some outputs
5 participants
@driazati @facebook-github-bot @nairbv @eellison @mruberry