[onnx] Add features to use ONNX build options #38879

luncliff · 2024-05-22T14:54:11Z

Changes

Make the following CMake build options to features in vcpkg.json

https://github.com/onnx/onnx/blob/b86cc54efce19530fb953e4b21f57e6b3888534c/CMakeLists.txt#L27-L28

References

Checklist

Changes comply with the maintainer guide.
SHA512s are updated for each updated download.
The "supports" clause reflects platforms that may be fixed by this new version.
Any fixed CI baseline entries are removed from that file.
Any patches that are no longer applied are deleted from the port's directory.
The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
Only one version is added to each modified port's versions file.

ports/onnx/portfile.cmake

Neumann-A · 2024-05-22T15:22:01Z

ports/onnx/portfile.cmake

@@ -24,6 +24,8 @@ endif()
 vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    FEATURES
        pybind11 BUILD_ONNX_PYTHON
+        disable-exception ONNX_DISABLE_EXCEPTIONS
+        disable-static-generation ONNX_DISABLE_STATIC_REGISTRATION


should probably be made a positive feature instead since it gives a shorter feature name.

Good idea. I will change them

Changed their names in 02fe04d

Based on the discussion, the revert feature here may cause fail when it's being enabled by other ports (means NO ONNX_DISABLE_STATIC_REGISTRATION).

Back to disable-static-registration in f339729
(note disable-static-generation was wrong name 😱 )

WangWeiLin-MV

Looks like it disables function, will enabling these two features cause failed for consumers? - do-not-use-features-to-control-alternatives-in-published-interfaces

luncliff · 2024-05-25T05:14:35Z

Looks like it disables function, will enabling these two features cause failed for consumers? - do-not-use-features-to-control-alternatives-in-published-interfaces

Honestly. I didn't check deeply for the part because I haven't encountered build errors in my team.
Let me check it

luncliff · 2024-05-25T06:59:48Z

ONNX_DISABLE_STATIC_REGISTRATION doesn't affect the public interface, but the implementation's behavior.

https://github.com/onnx/onnx/blob/b86cc54efce19530fb953e4b21f57e6b3888534c/onnx/defs/schema.cc#L1100-L1102

luncliff · 2024-05-30T02:27:15Z

Sorry for being late.
I added static-registration for default-features to make ONNX_DISABLE_STATIC_REGISTRATION=OFF just like the upstream's default option

dg0yt · 2024-05-30T05:21:50Z

I added static-registration for default-features to make ONNX_DISABLE_STATIC_REGISTRATION=OFF just like the upstream's default option

Note that there are three ports which currently depend on plain "onnx", i.e. implicitly with default features enabled.

luncliff · 2024-05-30T05:24:42Z

Yes. static-registration is the default feature. The user ports won't be affected

dg0yt · 2024-05-30T05:26:53Z

The point is that without updating the other ports, it is not possible to disable the feature when you use one of these ports.

luncliff · 2024-05-30T05:31:04Z

Understood. I can change the `static-generation` to `disable-static-generation` if the part can cause some problem. Do you think it's a better option?

…

On Thu, May 30, 2024 at 2:27 PM Kai Pastor ***@***.***> wrote: The point is that without updating the other ports, it is not possible to disable the feature when you use one of these ports. — Reply to this email directly, view it on GitHub <#38879 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLSYE6JO5PEXVF32EAKWJ3ZE22DHAVCNFSM6AAAAABID2NBY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZYG4YDQOBSGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- *Profile*Facebook: fb.com/luncliff GitHub: github.com/luncliff LinkedIn: linkedin.com/in/dong-ha-park-171481229/

dg0yt · 2024-05-30T05:35:44Z

?

No. Non-additive features aren't accepted, for good reasons.
It is probably the best to add "default-features": false to the other three ports. (Use git grep '"onnx"' ports/*/vcpkg.json.)

WangWeiLin-MV

The revert feature may cause failure when it's being enabled by other ports (enable inverted feature static-registrationmeans means NO ONNX_DISABLE_STATIC_REGISTRATION=0).

It is probably the best to add "default-features": false to the other three ports.

This is a fragile workaround causes incompatibility in a case vcpkg install onnx[static-registrationmeans] onnxruntime according to the discussion.

dg0yt · 2024-05-30T09:59:35Z

AFAIU:
(A) It is convenient and expected to have static registration in onnx when it is used directly.
(B) It is not possible to have static registration in onnx when onnxruntime is used.

(A) can only be achieved by a default-enabled capability, like a default-feature.
(B) requires the user (i.e. the top-level manifest) to disable onnx static registration when the project uses onnxruntime.

In vcpkg CI, we have both (A) and (B).

It is probably the best to add "default-features": false to the other three ports.

This is a fragile workaround causes incompatibility in a case vcpkg install onnx[static-registration] onnxruntime according to the discussion.

When you ask for failure, you get failure. Maybe the diagnostics could signal this early.

The alternative is to not change anything and require triplet customization in order to use onnxruntime.

luncliff · 2024-05-31T02:48:03Z

For existing ports and vcpkg CI,
I think removing the default-features (like what we did previously) and adding disable-static-registration feature is more simple because the other ports don't need to care about onnx changes.

Thankfully, ONNX_DISABLE_STATIC_REGISTRATION doesn't change the interface.
Unless vcpkg CI runs executables which requires ONNX, the port builds & installations will be fine.

WangWeiLin-MV

The port features installation tests pass with the following triplets:

x64-windows

BillyONeal · 2024-06-10T22:21:00Z

@dg0yt 's analysis in #38879 (comment) seems correct to me; I think this needs to be controlled with an overlay-port rather than features.

luncliff · 2024-06-13T15:47:50Z

Seems like no more comment.

You can close the PR with the team's decision.

BillyONeal · 2024-06-13T21:44:33Z

Related: #36850 . The discussion of the features-as-alternatives problem appears to have been discussed there.

### Description Changes vcpkg manifest and configuration file (vcpkg.json & vcpkg-configuration.json) * Update vcpkg version to https://github.com/microsoft/vcpkg/releases/tag/2024.12.16 * Use protobuf 3.21.12(= `v21.12`) to sync with [cmake/deps.txt](https://github.com/microsoft/onnxruntime/blob/main/cmake/deps.txt) * Resolve #22750 * Add `onnx` to vcpkg manifest so `find_package(ONNX)` and `find_dependency(Protobuf)` can work as expected. * Currently, It uses 1.16.2 * v1.17.0 will become available after microsoft/vcpkg#42942 However, `onnx` in vcpkg doesn't configure `ONNX_DISABLE_STATIC_REGISTRATION` build option. * microsoft/vcpkg#38879 * Create "cmake/vcpkg-triplets/" folder and triplet files which use `VCPKG_CMAKE_CONFIGURE_OPTIONS` for the option * This requires `VCPKG_OVERLAY_TRIPLETS` environment variable for CI steps, which is a bit inconvenient. I will try to find simple way to get same result ### Motivation and Context * Help #23158 * "ONNX is not consumed from vcpkg" * "Mismatch protobuf version. When vcpkg is enabled , we should not fetch protoc from Github which may cause version mismatches." * microsoft/vcpkg#43126 * #21348

### Description Changes vcpkg manifest and configuration file (vcpkg.json & vcpkg-configuration.json) * Update vcpkg version to https://github.com/microsoft/vcpkg/releases/tag/2024.12.16 * Use protobuf 3.21.12(= `v21.12`) to sync with [cmake/deps.txt](https://github.com/microsoft/onnxruntime/blob/main/cmake/deps.txt) * Resolve microsoft#22750 * Add `onnx` to vcpkg manifest so `find_package(ONNX)` and `find_dependency(Protobuf)` can work as expected. * Currently, It uses 1.16.2 * v1.17.0 will become available after microsoft/vcpkg#42942 However, `onnx` in vcpkg doesn't configure `ONNX_DISABLE_STATIC_REGISTRATION` build option. * microsoft/vcpkg#38879 * Create "cmake/vcpkg-triplets/" folder and triplet files which use `VCPKG_CMAKE_CONFIGURE_OPTIONS` for the option * This requires `VCPKG_OVERLAY_TRIPLETS` environment variable for CI steps, which is a bit inconvenient. I will try to find simple way to get same result ### Motivation and Context * Help microsoft#23158 * "ONNX is not consumed from vcpkg" * "Mismatch protobuf version. When vcpkg is enabled , we should not fetch protoc from Github which may cause version mismatches." * microsoft/vcpkg#43126 * microsoft#21348

### Description Changes vcpkg manifest and configuration file (vcpkg.json & vcpkg-configuration.json) * Update vcpkg version to https://github.com/microsoft/vcpkg/releases/tag/2024.12.16 * Use protobuf 3.21.12(= `v21.12`) to sync with [cmake/deps.txt](https://github.com/microsoft/onnxruntime/blob/main/cmake/deps.txt) * Resolve #22750 * Add `onnx` to vcpkg manifest so `find_package(ONNX)` and `find_dependency(Protobuf)` can work as expected. * Currently, It uses 1.16.2 * v1.17.0 will become available after microsoft/vcpkg#42942 However, `onnx` in vcpkg doesn't configure `ONNX_DISABLE_STATIC_REGISTRATION` build option. * microsoft/vcpkg#38879 * Create "cmake/vcpkg-triplets/" folder and triplet files which use `VCPKG_CMAKE_CONFIGURE_OPTIONS` for the option * This requires `VCPKG_OVERLAY_TRIPLETS` environment variable for CI steps, which is a bit inconvenient. I will try to find simple way to get same result ### Motivation and Context * Help #23158 * "ONNX is not consumed from vcpkg" * "Mismatch protobuf version. When vcpkg is enabled , we should not fetch protoc from Github which may cause version mismatches." * microsoft/vcpkg#43126 * #21348

Neumann-A reviewed May 22, 2024

View reviewed changes

ports/onnx/portfile.cmake Outdated Show resolved Hide resolved

Neumann-A reviewed May 22, 2024

View reviewed changes

MonicaLiu0311 assigned WangWeiLin-MV May 23, 2024

WangWeiLin-MV added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 23, 2024

WangWeiLin-MV requested changes May 24, 2024

View reviewed changes

luncliff marked this pull request as draft May 25, 2024 05:14

luncliff changed the title ~~[onnx] Support disable-static-generation, disable-exception feature~~ [onnx] Add features to use ONNX build options May 25, 2024

luncliff marked this pull request as ready for review May 30, 2024 02:26

luncliff requested review from WangWeiLin-MV and Neumann-A May 30, 2024 02:27

WangWeiLin-MV requested changes May 30, 2024

View reviewed changes

WangWeiLin-MV approved these changes May 31, 2024

View reviewed changes

WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label May 31, 2024

vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 3, 2024

BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Jun 10, 2024

BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 13, 2024

BillyONeal marked this pull request as draft June 13, 2024 21:55

BillyONeal mentioned this pull request Jun 27, 2024

[onnx,onnxruntime] new port for v1.19.2 with onnx 1.16.0 #36850

Draft

13 tasks

luncliff mentioned this pull request Aug 9, 2024

[onnxruntime] Update to v1.18.1, redesign portfile luncliff/vcpkg-registry#227

Merged

luncliff added 2 commits January 6, 2025 23:07

[onnx] remove @only parts in ONNXConfig.cmake.in

0c4eb74

[onnx] update baseline

205f9f7

luncliff force-pushed the port/onnx branch from f339729 to 205f9f7 Compare January 6, 2025 14:10

luncliff closed this Jan 6, 2025

luncliff deleted the port/onnx branch January 6, 2025 14:11

luncliff mentioned this pull request Jan 8, 2025

Correct ONNX and Protobuf version in vcpkg build microsoft/onnxruntime#23285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[onnx] Add features to use ONNX build options #38879

[onnx] Add features to use ONNX build options #38879

luncliff commented May 22, 2024 •

edited

Loading

Neumann-A May 22, 2024

luncliff May 25, 2024

luncliff May 25, 2024

WangWeiLin-MV May 27, 2024

luncliff May 31, 2024

WangWeiLin-MV left a comment

luncliff commented May 25, 2024

luncliff commented May 25, 2024

luncliff commented May 30, 2024

dg0yt commented May 30, 2024

luncliff commented May 30, 2024

dg0yt commented May 30, 2024

luncliff commented May 30, 2024 via email

dg0yt commented May 30, 2024

WangWeiLin-MV left a comment

dg0yt commented May 30, 2024

luncliff commented May 31, 2024

WangWeiLin-MV left a comment

BillyONeal commented Jun 10, 2024

luncliff commented Jun 13, 2024

BillyONeal commented Jun 13, 2024 •

edited

Loading

[onnx] Add features to use ONNX build options #38879

[onnx] Add features to use ONNX build options #38879

Conversation

luncliff commented May 22, 2024 • edited Loading

Changes

References

Checklist

Neumann-A May 22, 2024

Choose a reason for hiding this comment

luncliff May 25, 2024

Choose a reason for hiding this comment

luncliff May 25, 2024

Choose a reason for hiding this comment

WangWeiLin-MV May 27, 2024

Choose a reason for hiding this comment

luncliff May 31, 2024

Choose a reason for hiding this comment

WangWeiLin-MV left a comment

Choose a reason for hiding this comment

luncliff commented May 25, 2024

luncliff commented May 25, 2024

luncliff commented May 30, 2024

dg0yt commented May 30, 2024

luncliff commented May 30, 2024

dg0yt commented May 30, 2024

luncliff commented May 30, 2024 via email

dg0yt commented May 30, 2024

WangWeiLin-MV left a comment

Choose a reason for hiding this comment

dg0yt commented May 30, 2024

luncliff commented May 31, 2024

WangWeiLin-MV left a comment

Choose a reason for hiding this comment

BillyONeal commented Jun 10, 2024

luncliff commented Jun 13, 2024

BillyONeal commented Jun 13, 2024 • edited Loading

luncliff commented May 22, 2024 •

edited

Loading

BillyONeal commented Jun 13, 2024 •

edited

Loading