Skip to content

Fix pytorch_struct broken by more strict in-place operation restrictions#408

Merged
xuzhao9 merged 1 commit into
pytorch:masterfrom
xuzhao9:xz9/fix-pytorch-struct
Jun 22, 2021
Merged

Fix pytorch_struct broken by more strict in-place operation restrictions#408
xuzhao9 merged 1 commit into
pytorch:masterfrom
xuzhao9:xz9/fix-pytorch-struct

Conversation

@xuzhao9
Copy link
Copy Markdown
Contributor

@xuzhao9 xuzhao9 commented Jun 22, 2021

Recent CI fails because pytorch restricts the usage of in-place operations. This PR fixes the failure by using copy of data structures instead of in-place operations.

Copy link
Copy Markdown
Contributor

@bitfort bitfort left a comment

Choose a reason for hiding this comment

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

Good fix

@xuzhao9 xuzhao9 requested a review from malfet June 22, 2021 03:47
@xuzhao9 xuzhao9 merged commit 1d88f16 into pytorch:master Jun 22, 2021
@malfet
Copy link
Copy Markdown
Contributor

malfet commented Jun 22, 2021

Most likely caused by pytorch/pytorch#60195
cc: @albanD

@xuzhao9 xuzhao9 requested a review from albanD June 22, 2021 18:22
@albanD
Copy link
Copy Markdown
Contributor

albanD commented Jun 22, 2021

Yes that was a bug on our end were we were not detecting these inplace ops properly :/
This is the right fix!

@xuzhao9
Copy link
Copy Markdown
Contributor Author

xuzhao9 commented Jun 22, 2021

Yes that was a big on our end were we were not detecting these inplace ops properly :/
This is the right fix!

Thanks for the response. This PR fixes the broken run, but will it also break the correctness? I guess in this case the ctx.saved_tensors won't be updated because we are using a copy.

@albanD
Copy link
Copy Markdown
Contributor

albanD commented Jun 22, 2021

Well, in this case, not modifying the saved_tensors inplace is the right fix and the code before had correctness issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants