Skip to content

Sahar/csharp support openvino#4703

Merged
jywu-msft merged 17 commits intomicrosoft:masterfrom
intel:sahar/csharp_support_openvino
Aug 17, 2020
Merged

Sahar/csharp support openvino#4703
jywu-msft merged 17 commits intomicrosoft:masterfrom
intel:sahar/csharp_support_openvino

Conversation

@sfatimar
Copy link
Contributor

@sfatimar sfatimar commented Aug 4, 2020

Description:
Changes to include csharp support for openvino
Openvino provider factory header is included as part of the nuget package to enable openvino ep for csharp build
Linux shared library included as part of nuget package through msbuild process for easy nuget packing

Motivation and Context

  • Enable building csharp api for openvino .
  • Csharp api are not supported directly on linux so we are documenting the workaround to include linux runtime package.

@sfatimar sfatimar requested a review from a team as a code owner August 4, 2020 04:14
@ghost
Copy link

ghost commented Aug 4, 2020

CLA assistant check
All CLA requirements met.

@jywu-msft
Copy link
Member

@hariharans29 can you please help review? thanks.

sfatimar added 2 commits August 4, 2020 17:13
change in native nuget spec python script for including linux runtime
# Process runtimes
# Process linux
if (args.linux_build != 'None'):
files_list.append('<file src=' + '"' + os.path.join(args.sources_path, args.linux_build) +
Copy link
Member

Choose a reason for hiding this comment

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

linux_build [](start = 84, length = 11)

I think we can just hard-code the shared library name - libonnxruntime.so. I think we have used onnxruntime.dll in a hard-coded fashion in a lot of places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there should be some functionality to copy the file from wherever it is to the correct location.


In reply to: 465305444 [](ancestors = 465305444)

On Windows Machine
```
cp libonnxruntime.so $PATH/onnxruntime/
.\build.bat --config Debug --build --use_openvino $Device --build_csharp
Copy link
Contributor

Choose a reason for hiding this comment

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

What value should $PATH have? There are a few things it could be so a description would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this is clear enough for a new user.

Where is libonnxruntime.so copied from? Assuming the build output directory from the Linux build, but there's no mention of where to find that like <ORT linux repo>/build/Linux/<config>.

Also feels a little hacky to just dump the linux .so file in the root directory of the ORT repo. Would be nicer to put it somewhere closer to where the C# build output would go, like <ORT Windows repo>\csharp\src\Microsoft.ML.OnnxRuntime\bin\<config>\linux.

@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 9 pipeline(s).

@jywu-msft
Copy link
Member

/azp run orttraining-linux-ci-pipeline,orttraining-mac-ci-pipeline,orttraining-linux-gpu-ci-pipeline,centos7_cpu,Linux OpenVINO CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@jywu-msft
Copy link
Member

the python script modifications is failing PEP8 check.
see https://github.com/microsoft/onnxruntime/blob/master/docs/Coding_Conventions_and_Standards.md#python-code-style

device_ = "";
} else {
device_ = device;
device_ = std::string(device);
Copy link
Member

Choose a reason for hiding this comment

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

std::string [](start = 16, length = 11)

Keep the assignment

modification to documentation
@hariharans29
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

@hariharans29
Copy link
Member

/azp run orttraining-linux-ci-pipeline,orttraining-mac-ci-pipeline,orttraining-linux-gpu-ci-pipeline,centos7_cpu,Linux OpenVINO CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

parser.add_argument("--is_release_build", required=False, default=None, type=str,
help="Flag indicating if the build is a release build. Accepted values: true/false.")
parser.add_argument("--linux_build", required=False, default=False, type=bool,
help="specify the libonnxruntime.so lib values")
Copy link
Member

Choose a reason for hiding this comment

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

the help string probably needs to be updated after the change to bool type.

# Process runtimes
# Process linux
if (args.linux_build != 'None'):
files_list.append('<file src=' + '"' + os.path.join(args.sources_path, args.linux_build) +
Copy link
Member

Choose a reason for hiding this comment

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

args.linux_build is now a bool right?
so it should be os.path.join(args.sources_path, 'libonnxruntime.so') instead?


# Process runtimes
# Process linux
if (args.linux_build != 'None'):
Copy link
Member

Choose a reason for hiding this comment

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

args.linux_build is now a bool, so this check for None needs to be changed?

@jywu-msft jywu-msft merged commit 0a0ac70 into microsoft:master Aug 17, 2020
jywu-msft added a commit that referenced this pull request Aug 17, 2020
snnn pushed a commit that referenced this pull request Aug 17, 2020
hariharans29 pushed a commit that referenced this pull request Aug 18, 2020
* Temp changes and include openvino to ensure nuget package is created with linux till we configure azure ci pipeline

* string id change

* native nuget indentation changes

* documentation changes

* Update Openvino_execution_provider.md

Documentation includes openvino execution provider

* Update OpenVino-ExecutionProvider.md

update details to build csharp api for openvino execution provider .

* vadm backend revert

* Update Openvino-Execution-Provider.md

updated for review comments

* Update OpenVino-Execution-Provider.md

* Update OpenVINO-ExecutionProvider.md

* nuget package custome support for openvino
change in native nuget spec python script for including linux runtime

* change to make path to boolean flag

* removed the tab

* Update OpenVINO-ExecutionProvider.md

updated for review comments

* chnages to include pep8 warnings
modification to documentation

Co-authored-by: saharfraza <sfatima.3001@gmail.com>
Co-authored-by: sfatimar <sahar.fatima@intel/com>
jywu-msft pushed a commit that referenced this pull request Aug 18, 2020
* Sahar/csharp support openvino (#4703)

* Temp changes and include openvino to ensure nuget package is created with linux till we configure azure ci pipeline

* string id change

* native nuget indentation changes

* documentation changes

* Update Openvino_execution_provider.md

Documentation includes openvino execution provider

* Update OpenVino-ExecutionProvider.md

update details to build csharp api for openvino execution provider .

* vadm backend revert

* Update Openvino-Execution-Provider.md

updated for review comments

* Update OpenVino-Execution-Provider.md

* Update OpenVINO-ExecutionProvider.md

* nuget package custome support for openvino
change in native nuget spec python script for including linux runtime

* change to make path to boolean flag

* removed the tab

* Update OpenVINO-ExecutionProvider.md

updated for review comments

* chnages to include pep8 warnings
modification to documentation

Co-authored-by: saharfraza <sfatima.3001@gmail.com>
Co-authored-by: sfatimar <sahar.fatima@intel/com>

* Changes to include csharp support for openvino

* Fix flake error

* Fix

Co-authored-by: sfatimar <64512376+sfatimar@users.noreply.github.com>
Co-authored-by: saharfraza <sfatima.3001@gmail.com>
Co-authored-by: sfatimar <sahar.fatima@intel/com>
@sfatimar sfatimar deleted the sahar/csharp_support_openvino branch September 16, 2020 06:22
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.

6 participants