Skip to content

TF-TRT test activation converter - #39155

Merged
tensorflow-copybara merged 1 commit into
tensorflow:masterfrom
tfeher:trt_activation_dynamic_shape
May 20, 2020
Merged

tensorflow-copybara merged 1 commit into
tensorflow:masterfrom
tfeher:trt_activation_dynamic_shape

Conversation

@tfeher

@tfeher tfeher commented May 4, 2020

Copy link
Copy Markdown
Contributor

This PR adds dynamic shape and explicit batch tests for the activation ops.

Tagging @bixia1 for review

@google-ml-butler google-ml-butler Bot added the size:L CL Change Size: Large label May 4, 2020
@google-ml-butler
google-ml-butler Bot requested a review from joker-eph May 4, 2020 13:47
@gbaned gbaned self-assigned this May 4, 2020
@gbaned gbaned added the comp:gpu:tensorrt Issues specific to TensorRT label May 4, 2020
@gbaned
gbaned requested a review from bixia1 May 4, 2020 15:34
@tfeher
tfeher force-pushed the trt_activation_dynamic_shape branch from 2147c04 to aafacb8 Compare May 7, 2020 19:52
@tfeher

tfeher commented May 13, 2020

Copy link
Copy Markdown
Contributor Author
  • Added variants of AsTensor and AddTestWeights where the tf_dtype parameter is a function arg instead of a template parameter. Initialization of tf_dtype is not a constexpr (in C++11), and that lead to cumbersome instantiation BuildAndRun. This is simplified now.
  • Modify BuildAndRun to handle runtime errors.
  • Modify BuildAndRun to handle mulitple input/output tensors.
  • Moved the AddTesTensor variant with trt_mode attribute into ParameterizedOpConverterTest. The new dynamic shape tests will use that, and this way the trt_mode and tf_dtype args can be set automatically based on the test parameters, no need to explicitly pass them as args to the subroutines.
  • AddTestTensor initializes a TF input tensor too, not onle a TRT ITensor. Adding both tensors at one place helps to simplify the unit tests.

@tfeher tfeher left a comment

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.

Thanks @bixia1 for the review, I have addressed the issues.

@gbaned

gbaned commented May 19, 2020

Copy link
Copy Markdown
Contributor

@tfeher Can you please resolve conflicts? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label May 19, 2020
Additionally helper functions for dynamic shape tests are improved:
- Added test parameter member variables to ParameterizedOpConverterTestBase
- Single TetOpConverter function introduced
- Type parameter handling for input tensors simplified
@tfeher
tfeher force-pushed the trt_activation_dynamic_shape branch from 418148d to dc0149a Compare May 19, 2020 21:31

@tfeher tfeher left a comment

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.

I have resolved the conflicts and addressed the remaining issue. @bixia1 Please have another look.

@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 20, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 20, 2020
@tensorflow-copybara
tensorflow-copybara merged commit d5eb33b into tensorflow:master May 20, 2020
@gbaned gbaned removed the stat:awaiting response Status - Awaiting response from author label May 20, 2020
@tfeher
tfeher deleted the trt_activation_dynamic_shape branch May 16, 2022 08:45
copybara-service Bot pushed a commit that referenced this pull request Mar 13, 2026
Imported from GitHub PR openxla/xla#39155

📝 Summary of Changes
Replaces `reinterpret_cast` with `std::memcpy` to avoid U.B.
Implementation of Get_State was not setting state pointer when using the backward compatibility path

🎯 Justification
Support state with V0.2 FFI in latest JAX.

🚀 Kind of Contribution
 🐛 Bug Fix

Copybara import of the project:

--
b0bed02a0aa43ff92219ba2b1aa4ddf64306e1ca by Michael Goldfarb <[email protected]>:

Fix UB in state get/set when using backwards compat mode.
Also fixes the implementation to correctly set the state.

--
d18d5b6bc35632fe2984021f43d9b1414d4c7c3b by Michael Goldfarb <[email protected]>:

Fix write to state.

Merging this change closes #39155

Reverts 95aacc5

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#39155 from mgoldfarb-nvidia:mgoldfarb/fix_ffi_backwards_compat_bug_state d18d5b6bc35632fe2984021f43d9b1414d4c7c3b
PiperOrigin-RevId: 883152448
copybara-service Bot pushed a commit that referenced this pull request Mar 16, 2026
Imported from GitHub PR openxla/xla#39155

📝 Summary of Changes
Replaces `reinterpret_cast` with `std::memcpy` to avoid U.B.
Implementation of Get_State was not setting state pointer when using the backward compatibility path

🎯 Justification
Support state with V0.2 FFI in latest JAX.

🚀 Kind of Contribution
 🐛 Bug Fix

Copybara import of the project:

--
b0bed02a0aa43ff92219ba2b1aa4ddf64306e1ca by Michael Goldfarb <[email protected]>:

Fix UB in state get/set when using backwards compat mode.
Also fixes the implementation to correctly set the state.

--
d18d5b6bc35632fe2984021f43d9b1414d4c7c3b by Michael Goldfarb <[email protected]>:

Fix write to state.

Merging this change closes #39155

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#39155 from mgoldfarb-nvidia:mgoldfarb/fix_ffi_backwards_compat_bug_state d18d5b6bc35632fe2984021f43d9b1414d4c7c3b
PiperOrigin-RevId: 883152448
copybara-service Bot pushed a commit that referenced this pull request Mar 16, 2026
Imported from GitHub PR openxla/xla#39155

📝 Summary of Changes
Replaces `reinterpret_cast` with `std::memcpy` to avoid U.B.
Implementation of Get_State was not setting state pointer when using the backward compatibility path

🎯 Justification
Support state with V0.2 FFI in latest JAX.

🚀 Kind of Contribution
 🐛 Bug Fix

Copybara import of the project:

--
b0bed02a0aa43ff92219ba2b1aa4ddf64306e1ca by Michael Goldfarb <[email protected]>:

Fix UB in state get/set when using backwards compat mode.
Also fixes the implementation to correctly set the state.

--
d18d5b6bc35632fe2984021f43d9b1414d4c7c3b by Michael Goldfarb <[email protected]>:

Fix write to state.

Merging this change closes #39155

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#39155 from mgoldfarb-nvidia:mgoldfarb/fix_ffi_backwards_compat_bug_state d18d5b6bc35632fe2984021f43d9b1414d4c7c3b
PiperOrigin-RevId: 883152448
copybara-service Bot pushed a commit that referenced this pull request Mar 16, 2026
Imported from GitHub PR openxla/xla#39155

📝 Summary of Changes
Replaces `reinterpret_cast` with `std::memcpy` to avoid U.B.
Implementation of Get_State was not setting state pointer when using the backward compatibility path

🎯 Justification
Support state with V0.2 FFI in latest JAX.

🚀 Kind of Contribution
 🐛 Bug Fix

Copybara import of the project:

--
b0bed02a0aa43ff92219ba2b1aa4ddf64306e1ca by Michael Goldfarb <[email protected]>:

Fix UB in state get/set when using backwards compat mode.
Also fixes the implementation to correctly set the state.

--
d18d5b6bc35632fe2984021f43d9b1414d4c7c3b by Michael Goldfarb <[email protected]>:

Fix write to state.

Merging this change closes #39155

PiperOrigin-RevId: 884428841
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.

6 participants