Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add onnxruntime-cpp run export #78

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kuepe-sl
Copy link

@kuepe-sl kuepe-sl commented Oct 13, 2023

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

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
Copy link
Contributor

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:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@kuepe-sl
Copy link
Author

@conda-forge-admin, please rerender

@kuepe-sl
Copy link
Author

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 kuepe-sl force-pushed the fix-version-pinning branch from 35717bf to 3c57c49 Compare October 16, 2023 10:16
@kuepe-sl
Copy link
Author

kuepe-sl commented Nov 2, 2023

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

@cbourjau
Copy link
Contributor

cbourjau commented Nov 7, 2023

@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
Copy link
Member

xhochy commented Nov 7, 2023

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

@kuepe-sl
Copy link
Author

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
Copy link
Member

xhochy commented Nov 10, 2023

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 kuepe-sl force-pushed the fix-version-pinning branch from 3c57c49 to 0d2167d Compare December 14, 2023 12:33
@conda-forge-webservices
Copy link
Contributor

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
Copy link
Contributor

traversaro commented Dec 14, 2023

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
Copy link
Author

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
Copy link
Member

xhochy commented Dec 14, 2023

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
Copy link
Author

would a major.minor SOVERSION be fine?

@xhochy
Copy link
Member

xhochy commented Dec 14, 2023

Please open an upstream issue and ask for guidance.

@xhochy
Copy link
Member

xhochy commented Dec 14, 2023

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

@kuepe-sl
Copy link
Author

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:

  • 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.


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: microsoft/onnxruntime@e6301ee

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
Copy link
Contributor

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.

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.

Run export / version pinning missing for cpp package
4 participants