Skip to content

Add netstandard2.0 target to OnnxRuntime.Managed package#8600

Merged
skottmckay merged 1 commit intomicrosoft:masterfrom
georg-jung:add-ns2.0-target
Aug 5, 2021
Merged

Add netstandard2.0 target to OnnxRuntime.Managed package#8600
skottmckay merged 1 commit intomicrosoft:masterfrom
georg-jung:add-ns2.0-target

Conversation

@georg-jung
Copy link
Copy Markdown
Contributor

@georg-jung georg-jung commented Aug 3, 2021

Description: Add netstandard2.0 as a target framework for the Microsoft.ML.OnnxRuntime.Managed package.

Motivation and Context

  • Why is this change required? What problem does it solve?

This seems to be a quite easy change, so I'm not sure if it wasn't made for a reason. I tried searching for related PRs/issues but did not find any.

@georg-jung georg-jung requested a review from a team as a code owner August 3, 2021 19:10
@skottmckay
Copy link
Copy Markdown
Contributor

/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

@skottmckay
Copy link
Copy Markdown
Contributor

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

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 9 pipeline(s).


<PropertyGroup>
<TargetFramework>netstandard1.1</TargetFramework>
<TargetFrameworks>netstandard1.1;netstandard2.0</TargetFrameworks>
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.

Does this change increase the size of the binary produced?

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.

Afaik no. I didn't look deeply into the packages generated by our build process here before and after, but in general adding a new target does not change the existing target's binary but produces an additional binary, that is included in the nuget package. See e.g. [0] for an example (screenshot of the latest version of the Netwonsoft.Json package opened in NuGetPackageExplorer). When one adds the reference to their project, the SDK decides during the build which version suits best and just uses that one binary. Specifically, when adding a ns2.0 target, all consuming projects that support ns2.0 will use that ns2.0 binary by default, while consumers that do support ns1.1 but not ns2.0 will use the older target. Thus, this might increase the NuGet package download size a tiny bit (the managed lib is just 115kb before compression in the package), for the advantage of the greatly simplified dependency graph, but afaik multi targeting does not increase the deployed binaries' sizes.

[0]:
image

@skottmckay
Copy link
Copy Markdown
Contributor

/azp run Linux CPU Minimal Build E2E,Linux Nuphar CI Pipeline,Windows WebAssembly CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-ortmodule-distributed

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

@skottmckay
Copy link
Copy Markdown
Contributor

/azp run Linux CPU Minimal Build E2E CI

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

@snnn
Copy link
Copy Markdown
Contributor

snnn commented Aug 3, 2021

/azp run Windows GPU CI Pipeline

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@snnn
Copy link
Copy Markdown
Contributor

snnn commented Aug 3, 2021

/azp run Linux CPU Minimal Build E2E CI Pipeline

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@skottmckay
Copy link
Copy Markdown
Contributor

/azp run Windows GPU CI Pipeline

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@skottmckay skottmckay merged commit e673d2e into microsoft:master Aug 5, 2021
@georg-jung
Copy link
Copy Markdown
Contributor Author

CCing @joperezr and @ericstj, given you authored dotnet/runtime#39099

Also looking at Cross-platform targeting docs:

CONSIDER adding a target for net461 when you're offering a netstandard2.0 target.

Would it be advisable to add a net461 target here too?

Hope it's okay asking you, despite this isn't your project. I thought you might be the best ones to ask, given you were involved in the reasoning of including such targets in the packages of the runtime repo.

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Aug 5, 2021

No trouble to ask, happy to help. You can also add a net461 target if you have a lot of customers using your project on .NETFramework. Doing so will reduce the number of files your customers need to deploy when using your package (ultimately reducing their application size) when targeting older versions of .NETFramework. As you mentioned above your managed library is very small, so you will see some small growth to your package size but this is probably negligible in comparison to your total package size which is over 100 MB.

One thing to be careful about as you add more TargetFrameworks to the package is that compatible frameworks have compatible API. You can add more API on newer frameworks, but not vice-versa. To help you get it correct our team has built a new feature you might want to try: https://devblogs.microsoft.com/dotnet/package-validation/

Feel free to reach out or tag us in issues if you have more questions. Cheers!

@georg-jung
Copy link
Copy Markdown
Contributor Author

Thanks for the recommendations and your willingness to help! I don't know how many consumers are on [4.6.1 - 4.7.1] (if I'd guess I'd say probably not too many but I really don't know), so this is probably something @skottmckay or other team members could consider as a future step if they feel it's relevant. Also thanks for refering to the great new package validation feature. I already tried it in a tiny side project and really like how it works.

yuslepukhin added a commit that referenced this pull request Aug 6, 2021
snnn pushed a commit that referenced this pull request Aug 6, 2021
yuslepukhin added a commit that referenced this pull request Aug 9, 2021
skottmckay added a commit that referenced this pull request Sep 2, 2021
Initially added in #8600 but backed out due to breaking the packaging pipeline. Re-add with update to the nuspec generation script.

There's slighly more structure to the location of the props and targets file in preparation for adding more platforms in the near future.
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