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

Make DataFormatVecPermute's validation and implementation consistent with each other #50263

Conversation

PatriceVignola
Copy link
Contributor

There's currently a mismatch between what the validation of DataFormatVecPermute and its implementation is doing. While the validation allows src_format and dst_format to be of length 4 or 5, it only allows the input to be of shape [4] or [2, 4]. This leads to a gap in the validation where the following is allowed, but results in undefined behavior or uninitialized garbage data:

tf.raw_ops.DataFormatVecPermute(x=[1,2,3,4], src_format="NCDHW", dst_format="NDHWC")

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Jun 14, 2021
@google-cla google-cla bot added the cla: yes label Jun 14, 2021
@gbaned gbaned self-assigned this Jun 15, 2021
@gbaned gbaned added the comp:core issues related to core part of tensorflow label Jun 15, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Jun 15, 2021
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Jun 15, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jun 15, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 15, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Jun 15, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jun 15, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 15, 2021
@rthadur rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jun 15, 2021
@gbaned gbaned removed the ready to pull PR ready for merge process label Jun 16, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jun 16, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 16, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Jun 17, 2021
@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Jun 18, 2021
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jul 6, 2021
@allenlavoie
Copy link
Member

Looks like these tests are failing in their XLA+GPU configurations (//tensorflow/python:nn_test_xla_gpu). The tf2xla kernel likely needs updating:

void Compile(XlaOpKernelContext* ctx) override {

Alternatively you can use @test_util.disable_xla('reason') on the new tests for now (but please file a bug for the discrepancy to be fixed if so).

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed ready to pull PR ready for merge process labels Jul 29, 2021
@gbaned
Copy link
Contributor

gbaned commented Aug 5, 2021

@PatriceVignola Can you please check @allenlavoie's comments and keep us posted ? Thanks!

@PatriceVignola
Copy link
Contributor Author

@gbaned @allenlavoie I copied the changes to the XLA kernel and re-ran the tests.

@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Aug 13, 2021
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Aug 16, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 16, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 16, 2021
@gbaned gbaned added ready to pull PR ready for merge process and removed awaiting review Pull request awaiting review ready to pull PR ready for merge process labels Aug 17, 2021
@allenlavoie
Copy link
Member

Looks like clang-tidy identifies two issues. Can you take a look? https://github.com/tensorflow/tensorflow/blob/master/CONTRIBUTING.md#c-coding-style

Just in core/kernels/data_format_ops.h; explicit on a single-argument constructor and no trailing underscore for struct members.

@PatriceVignola
Copy link
Contributor Author

Sure I'll fix it, but this is preexisting. I didn't touch this header besides changing the array size.

@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Aug 20, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 20, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 20, 2021
@copybara-service copybara-service bot merged commit 9432e5e into tensorflow:master Aug 24, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Aug 24, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:core issues related to core part of tensorflow size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants