Skip to content

Add option to exclude support for loading ORT format models in full build.#5129

Merged
skottmckay merged 4 commits into
masterfrom
skottmckay/MakeSupportForOrtModelFormatOptionalInFullBuild
Sep 12, 2020
Merged

Add option to exclude support for loading ORT format models in full build.#5129
skottmckay merged 4 commits into
masterfrom
skottmckay/MakeSupportForOrtModelFormatOptionalInFullBuild

Conversation

@skottmckay
Copy link
Copy Markdown
Contributor

Description:
Add option to exclude support for loading ORT format models in full build.

Disable support for ORT format models in packages that are sensitive to binary size. Did not disable for nodejs, python or nuget packages at this point as the saving is only 60KB. Can do so if needed.

Motivation and Context
Reduce ORT package size.

Disable support for ORT format models in packages that are sensitive to binary size. Did not disable for nodejs, python or nuget packages at this point as the saving is only 60KB.  Can do so if necessary.
@skottmckay skottmckay requested a review from a team as a code owner September 11, 2020 10:11
#if defined(ENABLE_ORT_FORMAT_LOAD)
return LoadOrtModel(model_uri);
#else
return ORT_MAKE_STATUS(ONNXRUNTIME, INVALID_ARGUMENT, "ORT format model is not supported in this build.");
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.

this build [](start = 96, length = 10)

nit: instead of 'this build', would it be nicer to have:

"is not supported when ENABLE_ORT_FORMAT_LOAD is off"?

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.

I guess you don't really want to show compile options to the end users?


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

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.

Yeah. Depends how you build as to what disabled it. Could be a flag to build.py (called through build.bat or build.sh). Could be direct use of cmake.

--volume $HOME/.onnx:/home/onnxruntimedev/.onnx -e NIGHTLY_BUILD onnxruntimeregistry.azurecr.io/internal/azureml/onnxruntimecpubuild:ch2j /opt/python/cp37-cp37m/bin/python3 \
/onnxruntime_src/tools/ci_build/build.py --build_dir /build --config Release --enable_lto \
--skip_submodule_sync --parallel --build_shared_lib --use_openmp
--skip_submodule_sync --parallel --build_shared_lib --disable_ort_format_load --use_openmp
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.

If we're disabling it in one pipeline, I suggest doing it in all pipelines that generate public pkgs so that the binary produced in different packages is generated from the same set of build flags. Having the DLL in the zip pkgs (produced by this pipeline) different from the one in the nuget can cause confusion; we'd one such case before. If the savings are not huge (only 60kb), we can just keep the status quo.

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.

I agree 60kb saving is not worth the confusion, maybe in a future release we can publish a standalone conversion tool depends on how popular the mobile format will go, and disable the save/load flatbuffer ort format completely in the full ORT (only enable it in minimal ORT)

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.

Please update the nocontrib ops pipelines as well as they're used by vscode and Edge. If you search for --enable_msvc_static_runtime, you'll know the places where an update is required.

@skottmckay skottmckay merged commit 323a1ba into master Sep 12, 2020
@skottmckay skottmckay deleted the skottmckay/MakeSupportForOrtModelFormatOptionalInFullBuild branch September 12, 2020 02:21
yuslepukhin pushed a commit that referenced this pull request Sep 15, 2020
…uild. (#5129)

* Add ability to exclude support for loading ORT format models.
Disable support for ORT format models in packages
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