Skip to content

[TF:TRT] ConvertSplit for dynamic shapes. - #48246

Closed
DEKHTIARJonathan wants to merge 1 commit into
tensorflow:masterfrom
DEKHTIARJonathan:jdekhtiar/dynamic_shape/ConvertSplit
Closed

[TF:TRT] ConvertSplit for dynamic shapes.#48246
DEKHTIARJonathan wants to merge 1 commit into
tensorflow:masterfrom
DEKHTIARJonathan:jdekhtiar/dynamic_shape/ConvertSplit

Conversation

@DEKHTIARJonathan

@DEKHTIARJonathan DEKHTIARJonathan commented Apr 1, 2021 •
edited by bixia1
Loading

Copy link
Copy Markdown
Contributor

@tfeher @bixia1 for review
Fix SqueezeTensor to ensure that the TRT layers generated by ConvertSplit have unique names.

Add tests for ConvertSplit dynamic shapes.

@google-ml-butler google-ml-butler Bot added the size:L CL Change Size: Large label Apr 1, 2021
@google-ml-butler
google-ml-butler Bot requested a review from joker-eph April 1, 2021 22:02
@google-cla google-cla Bot added the cla: yes label Apr 1, 2021
@bixia1 bixia1 changed the title [TFTRT] Add Dynamic Shape Testing for ConvertSplit [TFTRT] ConvertSplit for dynamic shapes. Apr 2, 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 Apr 2, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 2, 2021
bixia1
bixia1 requested changes Apr 2, 2021
View reviewed changes
Comment thread tensorflow/compiler/tf2tensorrt/convert/convert_nodes_test.cc Outdated
run_status = Status::OK();
}
else {
auto expected_msg_substr = absl::StrFormat(p.expected_msg_substr, p.axis);

@bixia1 bixia1 Apr 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It turns out that his syntax doesn't work because within google, absl::StrFormat requires the string to be constexpr.

@DEKHTIARJonathan DEKHTIARJonathan Jul 22, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@gbaned gbaned removed the ready to pull PR ready for merge process label Apr 5, 2021
@gbaned

gbaned commented Apr 5, 2021

Copy link
Copy Markdown
Contributor

@DEKHTIARJonathan Can you please check @bixia1's comments and keep us posted ? Thanks!

@tfeher tfeher mentioned this pull request Apr 7, 2021
Closed
@gbaned gbaned added comp:gpu:tensorrt Issues specific to TensorRT stat:awaiting response Status - Awaiting response from author labels Apr 8, 2021
@gbaned

gbaned commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@DEKHTIARJonathan Any update on this PR? Please. Thanks!

@DEKHTIARJonathan

DEKHTIARJonathan commented May 3, 2021

Copy link
Copy Markdown
Contributor Author

Still under work, please do not close

@gbaned
gbaned requested a review from bixia1 May 7, 2021 05:15
@gbaned gbaned added awaiting review Pull request awaiting review and removed stat:awaiting response Status - Awaiting response from author labels May 7, 2021
@gbaned

gbaned commented Jun 25, 2021

Copy link
Copy Markdown
Contributor

@DEKHTIARJonathan Can you please resolve conflicts? Thanks!

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Jun 25, 2021
@tensorflowbutler tensorflowbutler added the stale This label marks the issue/pr stale - to be closed automatically if no activity label Jul 16, 2021
@DEKHTIARJonathan

DEKHTIARJonathan commented Jul 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@bixia1 finally fixed sorry for the delay, please review and let me know if we are good to go

INFO: Build completed successfully, 2592 total actions
//tensorflow/compiler/tf2tensorrt:convert_graph_test                     PASSED in 6.4s
//tensorflow/compiler/tf2tensorrt:convert_graph_test_gpu                 PASSED in 6.2s
//tensorflow/compiler/tf2tensorrt:convert_nodes_test                     PASSED in 153.9s
//tensorflow/compiler/tf2tensorrt:convert_nodes_test_gpu                 PASSED in 168.5s
//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 3.9s
//tensorflow/compiler/tf2tensorrt:tensorrt_test_cc_gpu                   PASSED in 3.9s
//tensorflow/compiler/tf2tensorrt:trt_allocator_test                     PASSED in 0.1s
//tensorflow/compiler/tf2tensorrt:trt_engine_op_test                     PASSED in 2.7s
//tensorflow/compiler/tf2tensorrt:trt_engine_op_test_gpu                 PASSED in 2.4s
//tensorflow/compiler/tf2tensorrt:trt_engine_resource_ops_test           PASSED in 2.4s
//tensorflow/compiler/tf2tensorrt:trt_engine_resource_ops_test_gpu       PASSED in 2.2s
//tensorflow/compiler/tf2tensorrt:trt_lru_cache_test                     PASSED in 0.1s
//tensorflow/compiler/tf2tensorrt:trt_shape_optimization_profiles_test   PASSED in 6.9s
//tensorflow/compiler/tf2tensorrt:trt_shape_optimization_profiles_test_gpu PASSED in 7.0s

INFO: Build completed successfully, 9 total actions
//tensorflow/python/compiler/tensorrt:trt_convert_test                   PASSED in 58.4s
//tensorflow/python/compiler/tensorrt:trt_convert_test_gpu               PASSED in 58.5s

@google-ml-butler google-ml-butler Bot removed stale This label marks the issue/pr stale - to be closed automatically if no activity stat:awaiting response Status - Awaiting response from author labels Jul 22, 2021
@gbaned
gbaned requested review from bixia1 and removed request for bixia1 July 29, 2021 15:15
@gbaned gbaned added the awaiting review Pull request awaiting review label Jul 29, 2021
@bixia1 bixia1 changed the title [TFTRT] ConvertSplit for dynamic shapes. [TF:TRT] ConvertSplit for dynamic shapes. Aug 18, 2021
@bixia1

bixia1 commented Aug 18, 2021

Copy link
Copy Markdown
Contributor

The PR title is : ConvertSplit for dynamic shapes.
On the other hand, this PR does NOT enable dynamic shape for ConvertSplit. Instead, it only fixes a bug that is related to the layer names generated by ConvertSplit. Am I right here?

@gbaned

gbaned commented Aug 26, 2021

Copy link
Copy Markdown
Contributor

@DEKHTIARJonathan Can you please check @bixia1's comments and keep us posted ? Thanks!

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Aug 26, 2021
@tensorflowbutler tensorflowbutler added the stale This label marks the issue/pr stale - to be closed automatically if no activity label Sep 11, 2021
@gbaned

gbaned commented Sep 23, 2021

Copy link
Copy Markdown
Contributor

@DEKHTIARJonathan Can you please resolve conflicts? Thanks!

@google-ml-butler google-ml-butler Bot removed the stat:awaiting response Status - Awaiting response from author label Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joker-eph joker-eph Awaiting requested review from joker-eph
@bixia1 bixia1 Awaiting requested review from bixia1

Assignees

@gbaned gbaned

Labels

cla: yes comp:gpu:tensorrt Issues specific to TensorRT size:L CL Change Size: Large stale This label marks the issue/pr stale - to be closed automatically if no activity

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@DEKHTIARJonathan @gbaned @bixia1 @kokoro-team @tensorflowbutler