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.

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

Fixes a bug in TensorStridedSliceUpdate #27803

Merged
merged 4 commits into from Apr 15, 2019
Merged

Fixes a bug in TensorStridedSliceUpdate #27803

merged 4 commits into from Apr 15, 2019

Conversation

eaplatanios
Copy link
Contributor

@alextp Fixes a bug introduced in #27327 .

alextp
alextp previously approved these changes Apr 12, 2019
@alextp alextp added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 12, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 12, 2019
@eaplatanios
Copy link
Contributor Author

@alextp Sorry I had to make an additional small change to pass the backwards compatibility test. This is necessary because I forgot to add the output definition in the op registration before.

@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Apr 15, 2019
@rthadur rthadur self-assigned this Apr 15, 2019
@rthadur rthadur added the prtype:bugfix PR to fix a bug label Apr 15, 2019
@rthadur rthadur moved this from Assigned Reviewer to Approved by Reviewer in PR Queue Apr 15, 2019
@rthadur rthadur requested a review from alextp April 15, 2019 00:43
@rthadur rthadur added the comp:ops OPs related issues label Apr 15, 2019
@alextp
Copy link
Contributor

alextp commented Apr 15, 2019

You shouldn't need to make the change to op_history.pbtxt because adding an output is backward-compatible and manual edits to that file are only required for non-backwards-compatible changes.

That said it's not harmful.

@alextp alextp added the kokoro:force-run Tests on submitted change label Apr 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 15, 2019
@eaplatanios
Copy link
Contributor Author

You shouldn't need to make the change to op_history.pbtxt because adding an output is backward-compatible and manual edits to that file are only required for non-backwards-compatible changes.

Thanks Alex! It's good to know that. The reason I actually did it is because this edit is technically not backwards-compatible as it changes the signature of the TensorStridedSliceAssign op (I had previously forgotten to declare that it has an output), thus resulting in a test failure.

@alextp
Copy link
Contributor

alextp commented Apr 15, 2019

Adding an output to an op is a backwards-compatible change though as it does not change the behavior of existing graphdefs.

@alextp alextp added the kokoro:force-run Tests on submitted change label Apr 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 15, 2019
@eaplatanios
Copy link
Contributor Author

I see. Shouldn't the backwards compatibility check test take that into account then? Because it was failing until I made that change to op_history.pbtxt which added the output declaration.

yongtang pushed a commit to yongtang/tensorflow that referenced this pull request Apr 15, 2019
@tensorflow-copybara tensorflow-copybara merged commit bda1cc3 into tensorflow:master Apr 15, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:ops OPs related issues prtype:bugfix PR to fix a bug ready to pull PR ready for merge process
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants