Skip to content

[TFTRT - Dynamic Shape Phase 3] Add Dynamic Shape Testing for ConvertArgMinMax - #45862

Merged
copybara-service[bot] merged 1 commit into
tensorflow:masterfrom
DEKHTIARJonathan:jdekhtiar/dynamic_shape/ConvertArgMinMax
Jan 12, 2021
Merged

copybara-service[bot] merged 1 commit into
tensorflow:masterfrom
DEKHTIARJonathan:jdekhtiar/dynamic_shape/ConvertArgMinMax

Conversation

@DEKHTIARJonathan

Copy link
Copy Markdown
Contributor

@bixia1 @tfeher for review

Feature Tracker: #45481

@google-ml-butler google-ml-butler Bot added the size:L CL Change Size: Large label Dec 18, 2020
@google-ml-butler
google-ml-butler Bot requested a review from joker-eph December 18, 2020 22:18
@google-cla google-cla Bot added the cla: yes label Dec 18, 2020
@DEKHTIARJonathan DEKHTIARJonathan changed the title [TFTRT - Dynamic Shape Phase 3] Add Dynamic Shape Testing for ConvertClipByValue [TFTRT - Dynamic Shape Phase 3] Add Dynamic Shape Testing for ConvertArgMinMax Dec 18, 2020
@gbaned gbaned self-assigned this Dec 20, 2020
@gbaned gbaned added the comp:gpu:tensorrt Issues specific to TensorRT label Dec 20, 2020
@gbaned
gbaned requested a review from bixia1 December 20, 2020 02:11

@bixia1 bixia1 left a comment

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.

Looks good to me. Thanks for working on this!
The test passes with TRT7 but fails with TRT5 though.

@gbaned

gbaned commented Dec 23, 2020

Copy link
Copy Markdown
Contributor

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

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Dec 23, 2020
@DEKHTIARJonathan

DEKHTIARJonathan commented Jan 5, 2021 •

Copy link
Copy Markdown
Contributor Author

@bixia1 I found the issue:

DefaultLogger my_arg-topk: implementation limit for 4D and higher tensors -- reduceAxes must specify one of last three dimensions

I pushed the fix and now it passes on TRT5 & TRT7. Can you please run the CI ?

return Status::OK();
}

string TrtDimsToTensorShapeString(const nvinfer1::Dims trt_dims,

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.

How about using absl::StrCat and absl::StrJoin like here

return absl::StrCat("[",
absl::StrJoin(dimvec, ",",
[](std::string* out, nvinfer1::Dims in) {
out->append(DebugString(in));
}),
"]");

?

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.

Oh right. I didn't see that function, my bad.

It's fixed ;)

@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Jan 7, 2021
@DEKHTIARJonathan

Copy link
Copy Markdown
Contributor Author

@bixia1 we should be good to go ;)

@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jan 12, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jan 12, 2021
@copybara-service
copybara-service Bot merged commit 7ebdb54 into tensorflow:master Jan 12, 2021
@DEKHTIARJonathan
DEKHTIARJonathan deleted the jdekhtiar/dynamic_shape/ConvertArgMinMax branch January 12, 2021 22:54
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:L CL Change Size: Large

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants