conda-forge / onnxruntime-feedstock

A conda-smithy repository for onnxruntime.
BSD 3-Clause "New" or "Revised" License
1 stars 19 forks source link

add onnxruntime-cpp run export #78

Open kuepe-sl opened 11 months ago

kuepe-sl commented 11 months ago

Checklist

fixes #76

I also made the SO use only the major version for linking (.so.1 instead of .so.1.16.0) for consistency with the Windows build.

conda-forge-webservices[bot] commented 11 months ago

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

kuepe-sl commented 11 months ago

@conda-forge-admin, please rerender

kuepe-sl commented 11 months ago

The linter has issues with the suffix variable, which is defined in conda_build_config.yaml - which the linter doesn't read.

I'm not sure what to do about this.

kuepe-sl commented 10 months ago

@cbourjau / @xhochy - would you mind having a quick look?

cbourjau commented 10 months ago

@cbourjau / @xhochy - would you mind having a quick look?

I had a look at it, but I'm honestly also unsure how to go about it! Sorry! :/

xhochy commented 10 months ago

Do you have any information on whether they will keep the ABI stable?

kuepe-sl commented 10 months ago

To be honest - I don't know.

The C API looks stable. They sometimes add new functions with new minor versions.
The C++ API is just a wrapper around the existing C calls and its implementation seems to be inside the header, so ABI compatibility should match the C API.

xhochy commented 10 months ago

The C++ API is just a wrapper around the existing C calls and its implementation seems to be inside the header, so ABI compatibility should match the C API

It still depends on whether they make ABI braking changes in the C++ wrapper. Can you link to the header so I can have a closer look?

kuepe-sl commented 10 months ago

sure: https://github.com/microsoft/onnxruntime/blob/main/include/onnxruntime/core/session/onnxruntime_cxx_api.h

conda-forge-webservices[bot] commented 9 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

traversaro commented 9 months ago

If upstream does not clearly documents an ABI stability policy, perhaps we can just ask upstream? Unless we receive a clear answer, probably it is better to be conservative and just claim patch version compatibility, and avoid to patch the SOVERSION?

Note that at the moment there is no conda-forge recipe that depends on onnxruntime-cpp, so at the moment we would not have any migration problem.

kuepe-sl commented 9 months ago

The current SOVERSION patch is mainly for consistency with the Windows build, which has no versioning in the DLL name at all.`

They have a constant called ORT_API_VERSION in one of the header files: https://github.com/microsoft/onnxruntime/blob/main/include/onnxruntime/core/session/onnxruntime_c_api.h#L40

It looks like this increases with every minor version, so maybe the best solution would be to set the SOVERSION to major.minor.
By default, it is major.minor.patch, which is too much, IMO.

xhochy commented 9 months ago

The current SOVERSION patch is mainly for consistency with the Windows build, which has no versioning in the DLL name at all.

Normally, there is no SOVERSION in any package on Windows. Thus, I wouldn't count this as an argument.

kuepe-sl commented 9 months ago

would a major.minor SOVERSION be fine?

xhochy commented 9 months ago

Please open an upstream issue and ask for guidance.

xhochy commented 9 months ago

SOVERSION indicates the ABI stability. If they have a better stability, they should indicate that upstream.

kuepe-sl commented 9 months ago

https://github.com/microsoft/onnxruntime/blob/main/docs/Versioning.md

ONNX Runtime follows Semantic Versioning 2.0 for its public API. Each release has the form MAJOR.MINOR.PATCH, adhering to the definitions from the linked semantic versioning doc.

That is for the API.


How this affects the ABI is told sort of implicitly in the rules about API changes here: https://github.com/microsoft/onnxruntime/blob/7dade5d05b67f4da8cc9ab949d576159682aff20/onnxruntime/core/session/onnxruntime_c_api.cc#L2361

The goal is for newer shared libraries of the Onnx Runtime to work with binaries targeting the previous versions. In order to do that we need to ensure older binaries get the older interfaces they are expecting.

The whole paragraph can be summarized as:

Due to the strict requirements, they effectively require a new "API version" for any sort of ABI changes as well. This is in line with their goal I quoted above.


Their "API version" equals the minor version of the library. They don't explicitly say that, but they always keep the "minor version" and the "API version" synchronized as seen here: https://github.com/microsoft/onnxruntime/commit/e6301eee6aa3f22f31bdcf431d0ed0ca828d20be

It should be also mentioned, that their "C++ API" just consists of header files that act as a wrapper for the existing C API. The library does not export actual C++ functions for their API.

traversaro commented 9 months ago

How this affects the ABI is told sort of implicitly in the rules about API changes here: https://github.com/microsoft/onnxruntime/blob/7dade5d05b67f4da8cc9ab949d576159682aff20/onnxruntime/core/session/onnxruntime_c_api.cc#L2361

The goal is for newer shared libraries of the Onnx Runtime to work with binaries targeting the previous versions. In order to do that we need to ensure older binaries get the older interfaces they are expecting.

The whole paragraph can be summarized as:

* no changes to the existing API or its underlying structures

* any sort of changes result in a new "API version"

Due to the strict requirements, they effectively require a new "API version" for any sort of ABI changes as well. This is in line with their goal I quoted above.

That is interestingly, probably it make sense to raise an issue upstream to ask why the SOVERSION setting is not aligned with this guidelines. It is possible that that documentation is not actually followed or enforced. However, I would not change the SOVERSION setting until upstream does so.