Skip to content

Fix NodeDefDataTypeAttributeName for Unary and Binary op converters.#56942

Merged
copybara-service[bot] merged 1 commit intotensorflow:masterfrom
drivanov:convert_unary_bug
Aug 24, 2022
Merged

Fix NodeDefDataTypeAttributeName for Unary and Binary op converters.#56942
copybara-service[bot] merged 1 commit intotensorflow:masterfrom
drivanov:convert_unary_bug

Conversation

@drivanov
Copy link
Contributor

@drivanov drivanov commented Jul 29, 2022

  • To check the input data type, we need to know which attribute of the node defines it. The op converter defines NodeDefDataTypeAttributeName() to return the name of the attribute.
  • The actual converters can override this. If an empty string is returned by NodeDefDataTypeAttributeName then type checking is skipped.
  • Previously the unary and binary op converters returned empty string.
    This bug is corrected in this PR.

@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Jul 29, 2022
@google-ml-butler google-ml-butler bot requested a review from bixia1 July 29, 2022 04:50
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Jul 29, 2022
@drivanov drivanov marked this pull request as draft July 29, 2022 04:50
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks Andrei for the fix! The PR looks good to me.

Please change the PR Title: Fix NodeDafDataTypeAttributeName for Unary and Binary op converters.

Please change the PR description:

  • To check the input data type, we need to know which attribute of the node defines it. The op converter defines NodeDefDataTypeAttributeName() to return the name of the attribute.
  • The actual converters can override this. If an empty string is returned by NodeDefDataTypeAttributeName then type checking is skipped.
  • Previously the unary and binary op converters returned empty string.
    This bug is corrected in this PR.

We will create a separate PR for improving the unit test to catch such errors.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 29, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 29, 2022
@gbaned gbaned removed ready to pull PR ready for merge process awaiting review Pull request awaiting review labels Jul 29, 2022
@drivanov drivanov changed the title Fixing bug in Converter for Unary operators Fix NodeDafDataTypeAttributeName for Unary and Binary op converters. Jul 30, 2022
@drivanov drivanov marked this pull request as ready for review July 30, 2022 01:34
@drivanov drivanov marked this pull request as draft August 2, 2022 17:58
@drivanov drivanov force-pushed the convert_unary_bug branch from 885d3f1 to b06e7b8 Compare August 7, 2022 04:18
@bixia1 bixia1 changed the title Fix NodeDafDataTypeAttributeName for Unary and Binary op converters. Fix NodeDefDataTypeAttributeName for Unary and Binary op converters. Aug 8, 2022
@drivanov drivanov force-pushed the convert_unary_bug branch 2 times, most recently from e318171 to 7e7eacc Compare August 8, 2022 20:18
@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 9, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 9, 2022
@drivanov drivanov force-pushed the convert_unary_bug branch from 7e7eacc to 2ad795a Compare August 9, 2022 18:04
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Aug 9, 2022
@drivanov drivanov marked this pull request as ready for review August 9, 2022 18:19
@drivanov drivanov force-pushed the convert_unary_bug branch from 2ad795a to 1111583 Compare August 9, 2022 19:34
@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 10, 2022
@google-ml-butler google-ml-butler bot added the ready to pull PR ready for merge process label Aug 19, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 19, 2022
Copy link
Contributor

@bixia1 bixia1 left a comment

Choose a reason for hiding this comment

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

Compilation error:
third_party/tensorflow/compiler/tf2tensorrt/convert/ops/einsum.cc:706:40: error: no matching function for call to 'ConvertMatMulImpl'
StatusOr result = ConvertMatMulImpl(
^~~~~~~~~~~~~~~~~
./third_party/tensorflow/compiler/tf2tensorrt/convert/convert_nodes.h:566:27: note: candidate function not viable: 1st argument ('const OpConverterParams const') would lose const qualifier
StatusOr ConvertMatMulImpl(OpConverterParams
params,
^

@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Aug 22, 2022
@drivanov
Copy link
Contributor Author

CORRECTED.

@gbaned gbaned requested a review from bixia1 August 23, 2022 07:59
@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 23, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 23, 2022
Copy link
Contributor

@bixia1 bixia1 left a comment

Choose a reason for hiding this comment

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

There are still similar errors:
third_party/tensorflow/compiler/tf2tensorrt/convert/ops/einsum.cc:713:24: error: no matching function for call to 'ShuffleEinsumOutput'
TF_RETURN_IF_ERROR(ShuffleEinsumOutput(
^~~~~~~~~~~~~~~~~~~
./third_party/tensorflow/core/platform/errors.h:125:37: note: expanded from macro 'TF_RETURN_IF_ERROR'
::tensorflow::Status _status = (VA_ARGS);
^~~~~~~~~~~
third_party/tensorflow/compiler/tf2tensorrt/convert/ops/einsum.cc:513:8: note: candidate function not viable: 1st argument ('const OpConverterParams const') would lose const qualifier
Status ShuffleEinsumOutput(OpConverterParams
params, EinsumDescriptor desc_a,
^

@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Aug 23, 2022
@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 23, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 23, 2022
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Aug 23, 2022
@drivanov
Copy link
Contributor Author

FIXED

@gbaned gbaned requested a review from bixia1 August 24, 2022 05:51
@gbaned gbaned added the comp:gpu:tensorrt Issues specific to TensorRT label Aug 24, 2022
@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 24, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 24, 2022
@copybara-service copybara-service bot merged commit 1a17caa into tensorflow:master Aug 24, 2022
@drivanov drivanov deleted the convert_unary_bug branch August 24, 2022 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review Pull request awaiting review comp:gpu:tensorrt Issues specific to TensorRT ready to pull PR ready for merge process size:XS CL Change Size: Extra Small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants