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

[TFTRT - Dynamic Shape Phase 3] Add Dynamic Shape Testing for ConvertClipByValue #45589

Conversation

DEKHTIARJonathan
Copy link
Contributor

@bixia1 @tfeher for review

Feature Tracker: #45481

Please merge this PR after: #45587

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Dec 11, 2020
@google-cla google-cla bot added the cla: yes label Dec 11, 2020
@gbaned gbaned self-assigned this Dec 11, 2020
@gbaned gbaned added the comp:gpu:tensorrt Issues specific to TensorRT label Dec 11, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Dec 11, 2020
@gbaned gbaned requested a review from bixia1 December 11, 2020 02:38
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.

Looks good to me. We will come back to this after the parent PR is merged.

@DEKHTIARJonathan DEKHTIARJonathan force-pushed the jdekhtiar/dynamic_shape/ConvertClipByValue branch 3 times, most recently from 2d71150 to 6ee0d05 Compare December 12, 2020 02:06
@DEKHTIARJonathan
Copy link
Contributor Author

@bixia1 rebased on master (with the new OpConverterTest name).
Good for review / merge

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Dec 14, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 14, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 14, 2020
@bixia1
Copy link
Contributor

bixia1 commented Dec 14, 2020

The test doesn't even compiled and here is just one example of the error message:
third_party/tensorflow/compiler/tf2tensorrt/convert/convert_nodes_test.cc:6637:5: error: no matching member function for call to 'AddTestTensor'
AddTestTensor("t", {1, 2, 3}, trt_type_);
^~~~~~~~~~~~~
third_party/tensorflow/compiler/tf2tensorrt/convert/convert_nodes_test.cc:1811:8: note: candidate template ignored: could not match 'vector' against 'nvinfer1::DataType'
void AddTestTensor(const string& name, const std::vector& dims,
^

@gbaned gbaned removed the ready to pull PR ready for merge process label Dec 14, 2020
@DEKHTIARJonathan DEKHTIARJonathan force-pushed the jdekhtiar/dynamic_shape/ConvertClipByValue branch from 6ee0d05 to 623dece Compare December 15, 2020 01:25
@DEKHTIARJonathan
Copy link
Contributor Author

The bug is fixed and we are good.

INFO: 318 processes: 318 local.
INFO: Build completed successfully, 325 total actions
//tensorflow/compiler/tf2tensorrt:convert_graph_test                     PASSED in 16.7s
//tensorflow/compiler/tf2tensorrt:convert_graph_test_gpu                 PASSED in 14.9s
//tensorflow/compiler/tf2tensorrt:convert_nodes_test                     PASSED in 96.0s
//tensorflow/compiler/tf2tensorrt:convert_nodes_test_gpu                 PASSED in 86.8s
//tensorflow/compiler/tf2tensorrt:segment_test                           PASSED in 0.2s
//tensorflow/compiler/tf2tensorrt:segment_test_gpu                       PASSED in 0.1s
//tensorflow/compiler/tf2tensorrt:tensorrt_test_cc                       PASSED in 16.7s
//tensorflow/compiler/tf2tensorrt:tensorrt_test_cc_gpu                   PASSED in 15.9s
//tensorflow/compiler/tf2tensorrt:trt_allocator_test                     PASSED in 0.1s
//tensorflow/compiler/tf2tensorrt:trt_engine_op_test                     PASSED in 13.0s
//tensorflow/compiler/tf2tensorrt:trt_engine_op_test_gpu                 PASSED in 12.4s
//tensorflow/compiler/tf2tensorrt:trt_engine_resource_ops_test           PASSED in 13.1s
//tensorflow/compiler/tf2tensorrt:trt_engine_resource_ops_test_gpu       PASSED in 12.4s
//tensorflow/compiler/tf2tensorrt:trt_lru_cache_test                     PASSED in 0.1s
//tensorflow/compiler/tf2tensorrt:trt_shape_optimization_profiles_test   PASSED in 15.9s
//tensorflow/compiler/tf2tensorrt:trt_shape_optimization_profiles_test_gpu PASSED in 12.1s

Executed 16 out of 16 tests: 16 tests pass.
INFO: Build completed successfully, 325 total actions

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 15, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 15, 2020
@bixia1
Copy link
Contributor

bixia1 commented Dec 15, 2020

I am still seeing this error when using TRT7:
third_party/tensorflow/compiler/tf2tensorrt/convert/convert_nodes_test.cc:6687:44: error: type 'float' cannot be narrowed to 'int' in initializer list [-Wc++11-narrowing]
AddTestWeights("clip_value_min", {1}, {p.clip_value_min}, tf_type_);
^~~~~~~~~~~~~~~~
third_party/tensorflow/compiler/tf2tensorrt/convert/convert_nodes_test.cc:6687:44: note: insert an explicit cast to silence this issue
AddTestWeights("clip_value_min", {1}, {p.clip_value_min}, tf_type_);
^~~~~~~~~~~~~~~~
static_cast( )

@DEKHTIARJonathan DEKHTIARJonathan force-pushed the jdekhtiar/dynamic_shape/ConvertClipByValue branch from 623dece to af62894 Compare December 15, 2020 22:28
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Dec 15, 2020
@DEKHTIARJonathan DEKHTIARJonathan force-pushed the jdekhtiar/dynamic_shape/ConvertClipByValue branch from af62894 to 2f368c1 Compare December 15, 2020 23:06
@DEKHTIARJonathan DEKHTIARJonathan force-pushed the jdekhtiar/dynamic_shape/ConvertClipByValue branch from 2f368c1 to 5166076 Compare December 15, 2020 23:13
@DEKHTIARJonathan
Copy link
Contributor Author

Hum... I'm not exactly sure why it fails on your side. It works perfectly on my end...

I changed the dtype of clip_value_min and clip_value_max to int. That should fix the error message you saw: https://github.com/tensorflow/tensorflow/pull/45589/files#diff-b9ae60f058d9ae72ddc0625fa93ff1b8441e57db5695e8444d18becdb3abea2dR6655-R6656

INFO: Build completed successfully, 21 total actions
//tensorflow/compiler/tf2tensorrt:convert_graph_test                     PASSED in 14.7s
//tensorflow/compiler/tf2tensorrt:convert_graph_test_gpu                 PASSED in 14.6s
//tensorflow/compiler/tf2tensorrt:convert_nodes_test                     PASSED in 91.2s
//tensorflow/compiler/tf2tensorrt:convert_nodes_test_gpu                 PASSED in 91.8s
//tensorflow/compiler/tf2tensorrt:segment_test                           PASSED in 0.3s
//tensorflow/compiler/tf2tensorrt:segment_test_gpu                       PASSED in 0.1s
//tensorflow/compiler/tf2tensorrt:tensorrt_test_cc                       PASSED in 13.2s
//tensorflow/compiler/tf2tensorrt:tensorrt_test_cc_gpu                   PASSED in 13.1s
//tensorflow/compiler/tf2tensorrt:trt_allocator_test                     PASSED in 0.1s
//tensorflow/compiler/tf2tensorrt:trt_engine_op_test                     PASSED in 12.6s
//tensorflow/compiler/tf2tensorrt:trt_engine_op_test_gpu                 PASSED in 12.7s
//tensorflow/compiler/tf2tensorrt:trt_engine_resource_ops_test           PASSED in 12.4s
//tensorflow/compiler/tf2tensorrt:trt_engine_resource_ops_test_gpu       PASSED in 12.2s
//tensorflow/compiler/tf2tensorrt:trt_lru_cache_test                     PASSED in 0.1s
//tensorflow/compiler/tf2tensorrt:trt_shape_optimization_profiles_test   PASSED in 12.1s
//tensorflow/compiler/tf2tensorrt:trt_shape_optimization_profiles_test_gpu PASSED in 12.0s

Executed 16 out of 16 tests: 16 tests pass.
INFO: Build completed successfully, 21 total actions

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 16, 2020
@copybara-service copybara-service bot merged commit 66f670d into tensorflow:master Dec 16, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Dec 16, 2020
@DEKHTIARJonathan DEKHTIARJonathan deleted the jdekhtiar/dynamic_shape/ConvertClipByValue branch January 14, 2021 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:gpu:tensorrt Issues specific to TensorRT ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants