Skip to content

Conversation

@chilo-ms
Copy link
Contributor

Description:
Enable TRT EP for C#.
This PR is derived from #7179 but without supporting provider options configuration.

@chilo-ms chilo-ms requested a review from jywu-msft April 28, 2021 13:43
@chilo-ms chilo-ms requested a review from a team as a code owner April 28, 2021 13:43
@jywu-msft
Copy link
Member

wonder why CI isn't automatically kicked off. are you missing some membership

@jywu-msft
Copy link
Member

/azp run orttraining-linux-ci-pipeline,orttraining-mac-ci-pipeline,orttraining-linux-gpu-ci-pipeline,centos7_cpu,Linux CPU Minimal Build E2E CI Pipeline,Linux Nuphar CI Pipeline,MacOS NoContribops CI Pipeline,Linux OpenVINO CI Pipeline,orttraining-distributed, orttraining-amd-gpu-ci-pipeline

@jywu-msft
Copy link
Member

/azp run Linux CPU CI Pipeline,Linux CPU x64 NoContribops CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,MacOS CI Pipeline,MacOS NoContribops CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@jywu-msft
Copy link
Member

/azp run orttraining-ortmodule, orttraining-ortmodule-distributed

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

SessionOptions options = new SessionOptions();
NativeApiStatus.VerifySuccess(NativeMethods.OrtSessionOptionsAppendExecutionProvider_Tensorrt(options.Handle, deviceId));
NativeApiStatus.VerifySuccess(NativeMethods.OrtSessionOptionsAppendExecutionProvider_CUDA(options.Handle, deviceId));
NativeApiStatus.VerifySuccess(NativeMethods.OrtSessionOptionsAppendExecutionProvider_CPU(options.Handle, 1));
Copy link
Member

Choose a reason for hiding this comment

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

We are not guarding against the options leak on exception here.

Copy link
Member

Choose a reason for hiding this comment

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

looks like all the other MakeSessionOption static methods in this file are following the same pattern.
what's your suggestion. how should we recover here. this method returns options.
prevent the leak but propagate the exception?

Copy link
Member

Choose a reason for hiding this comment

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

We just try/catch. In catch we do options.Dispose() and then rethrow.

Copy link
Member

@yuslepukhin yuslepukhin Apr 28, 2021

Choose a reason for hiding this comment

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

Need to fix the other locations (2?)

Copy link
Contributor Author

@chilo-ms chilo-ms Apr 28, 2021

Choose a reason for hiding this comment

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

@yuslepukhin are you suggesting using try/catch as previous discussion? #7315 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added try/catch around native methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think others should also properly address this options leak, I can help modify in another PR.

@chilo-ms chilo-ms merged commit 0dbe51b into master Apr 29, 2021
@chilo-ms chilo-ms deleted the c_sharp_trt branch April 29, 2021 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants