Repository navigation
[TF-TRT] Fix TopK Combined NMS - #40062
copybara-service[bot] merged 1 commit into
Conversation
| bool share_location = (boxes_dims.d[1] == 1); | ||
| const bool pad_per_class = attrs.get<bool>("pad_per_class"); | ||
| int top_k; | ||
| const int top_k = boxes_dims.d[0]; |
There was a problem hiding this comment.
Can you think of a way to show the effect of this change, such as modifying OpConverterTest.ConvertCombinedNMS?
There was a problem hiding this comment.
I have a test case that shows the error, but it's a bit convoluted and complex to be packaged in a unittest. I'll try to see what I can do ;)
|
@bixia1 as requested here is a small script to reproduce the issue: import os
import time
os.environ["TF_CPP_MIN_LOG_LEVEL"]="3"
import numpy as np
import tensorflow.compat.v1 as tf
tf.disable_v2_behavior()
tf.compat.v1.logging.set_verbosity(tf.compat.v1.logging.ERROR)
N = 2048
num_classes = 90
pre_nms_boxes = tf.placeholder(tf.float32, name='boxes', shape=[1, N, 1, 4])
pre_nms_scores = tf.placeholder(tf.float32, name='scores', shape=[1, N, num_classes])
max_detetion_points = N
max_boxes_to_draw = 30
iou_threshold = 0.1
min_score_thresh = 0.001
post_nms_boxes, post_nms_scores, post_nms_classes, post_nms_num_valid_boxes = \
tf.image.combined_non_max_suppression(
pre_nms_boxes,
pre_nms_scores,
max_output_size_per_class=max_detetion_points,
max_total_size=max_boxes_to_draw,
iou_threshold=iou_threshold,
score_threshold=min_score_thresh,
pad_per_class=False
)
np.random.seed(666)
np_boxes = np.random.rand(1, 2048, 1, 4)
np_scores = np.random.rand(1, 2048, 90)
with tf.Session() as sess:
tf_output = sess.run([
post_nms_boxes,
post_nms_scores,
post_nms_classes,
post_nms_num_valid_boxes
],
feed_dict={pre_nms_boxes: np_boxes, pre_nms_scores: np_scores}
)
with tf.Session() as sess:
output = [
tf.identity(i)
for i in [
post_nms_boxes,
post_nms_scores,
post_nms_classes,
post_nms_num_valid_boxes
]
]
output_node_names = [node.name.split(':')[0] for node in output]
graphdef = tf.graph_util.convert_variables_to_constants(
sess, sess.graph_def, output_node_names
)
fetches = [pre_nms_boxes.name, pre_nms_scores.name] + [i.name for i in output]
from tensorflow.python.compiler.tensorrt import trt_convert as trt
converter = trt.TrtGraphConverter(
nodes_blacklist=[t.split(':')[0] for t in fetches],
input_graph_def=graphdef,
max_workspace_size_bytes=1 << 32, # 4Gb
minimum_segment_size=1,
precision_mode='FP32')
infer_graph = converter.convert()
goutput = tf.import_graph_def(infer_graph, return_elements=fetches)
inputs, output = goutput[:2], goutput[2:]
trt_output = sess.run(
output,
feed_dict={inputs[0]: np_boxes, inputs[1]: np_scores}
)
print("tf_output - post_nms_classes:", tf_output[2])
print("trt_output - post_nms_classes:", trt_output[2])If you run with current upstream tensorflow: $ python main.py
WARNING:tensorflow:From /usr/local/lib/python3.6/dist-packages/tensorflow_core/python/compat/v2_compat.py:88: disable_resource_variables (from tensorflow.python.ops.variable_scope) is deprecated and will be removed in a future version.
Instructions for updating:
non-resource variables are not supported in the long term
tf_output - post_nms_classes: [[89. 6. 85. 51. 68. 1. 1. 78. 51. 27. 61. 63. 30. 49. 78. 45. 8. 46.
76. 10. 35. 37. 24. 77. 13. 16. 35. 27. 28. 25.]]
trt_output - post_nms_classes: [[89. 6. 85. 51. 68. 1. 1. 1. 78. 51. 27. 85. 61. 63. 30. 49. 78. 45.
8. 46. 76. 10. 35. 37. 24. 77. 13. 16. 35. 27.]]As you can see the two results are different. $ python main.py
WARNING:tensorflow:From /usr/local/lib/python3.6/dist-packages/tensorflow/python/compat/v2_compat.py:96: disable_resource_variables (from tensorflow.python.ops.variable_scope) is deprecated and will be removed in a future version.
Instructions for updating:
non-resource variables are not supported in the long term
tf_output - post_nms_classes: [[89. 6. 85. 51. 68. 1. 1. 78. 51. 27. 61. 63. 30. 49. 78. 45. 8. 46.
76. 10. 35. 37. 24. 77. 13. 16. 35. 27. 28. 25.]]
trt_output - post_nms_classes: [[89. 6. 85. 51. 68. 1. 1. 78. 51. 27. 61. 63. 30. 49. 78. 45. 8. 46.
76. 10. 35. 37. 24. 77. 13. 16. 35. 27. 28. 25.]]And now with this change we are good ;) |
|
Thanks for the test case. Can we find a way to turn in into a test in the tree? |
|
@DEKHTIARJonathan Can you please check @bixia1's comments and keep us posted. Thanks! |
google-ml-butler
Bot
added
kokoro:force-run
|
@DEKHTIARJonathan Can you please address Ubuntu Sanity errors? Thanks! |
|
@gbaned styling fixed ;) |
google-ml-butler
Bot
added
kokoro:force-run
gbaned
added
awaiting review
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
One more test failure, tensorflow/python/compiler/tensorrt:combined_nms_test with TRT 7.1.3: Traceback (most recent call last): Missing entries: |
|
@DEKHTIARJonathan Can you please check @bixia1's comments and keep us posted ? Thanks! |
gbaned
added
stat:awaiting response
|
@bixia1 the C++ unittest wasn't properly deactivated with TRT < 7.1.3. Fixed has been applied and commits squashed: https://github.com/tensorflow/tensorflow/pull/40062/files#diff- Please run the CI again ;) And we can merge if good |
google-ml-butler
Bot
added
kokoro:force-run
|
It seems to me that this failure is still reproducible |
|
@DEKHTIARJonathan Can you please check @bixia1's comments and keep us posted ? Thanks! |
gbaned
added
stat:awaiting response
|
Reassigning this issue to @tfeher |
tensorflowbutler
removed
the
stat:awaiting response
gbaned
added
stat:awaiting response
|
@tfeher could we finalized the last few bits of that one ? |
tensorflowbutler
removed
the
stat:awaiting response
tensorflowbutler
added
the
stale
|
It has been 15 days with no activity and the |
- We have to set top_k so that keep_top_k <= top_k, otherwise the plugin does not work with TRT 7.0 - Enabled explicit batch mode for the converter, added test for it. Dynamic shapes are not supported by the current plugin. - Added test to check the clib_boxes attribute usage - Added test with nonzero score threshold - Added test where the boxes are defined as y_max, x_max, y_min, x_min. This test will fail with TRT 7.1.2 or earlier and highlights that the CombinedNMS plugin is not compatible with the TF definition of the operator in these versions of TRT.
|
I have updated the PR. I could not reproduce the failing test yet. According to the log it is caused by the V1 graph def verification ( |
CC: @bixia1
in tftrt, the tf.image.combined_non_max_suppression op is replaced by a trt engine using batchedNMSplugin, with the minimal reproducible code piece, there's something not aligned.
TRT nms plugin has argument "topK", and we will only do IOU operation on the topK box in each class, so we set topK to boxes count in each class, to fix mismatch between TF and TRT.