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

ONNXruntime interface #420

Merged
merged 36 commits into from
Jan 15, 2024
Merged

ONNXruntime interface #420

merged 36 commits into from
Jan 15, 2024

Conversation

20DM
Copy link
Collaborator

@20DM 20DM commented Oct 27, 2023

  • Add a draft interface for ONNXruntime
  • Add unit tests to check model loading, metadata loading and forward folding of the classifier
  • Add an example ONNXrt file constructed from one of Tobías's PyTorch models

@20DM 20DM requested a review from mmcleod89 October 27, 2023 18:46
@mmcleod89
Copy link
Contributor

mmcleod89 commented Dec 12, 2023

When trying to get this to compile with the pre-compiled onnxruntime binary I added the following to the top level CMakeLists:

set(onnx_prefix /home/mmcleod/onnxruntime-linux-x64-1.16.2)
set(onnxruntime_INCLUDE_DIR "${onnx_prefix}/include/")
find_library(onnxruntime_LIBRARY onnxruntime PATHS "${onnx_prefix}/lib")
add_library(onnxruntime SHARED IMPORTED)
set_property(TARGET onnxruntime PROPERTY IMPORTED_LOCATION "${onnxruntime_LIBRARY}")
set(onnx_LIBRARIES onnxruntime)

The hard-coded path can naturally be replaced by a variable

I also had to change references to onnxruntime::onnxruntime in the CMakeLists files to onnxruntime and the folder structure was a bit different, so I needed to use #include "onnxruntime_cxx_api.h" instead of onnxruntime/onnxruntime_cxx_api.h.

@mmcleod89
Copy link
Contributor

Okay so when I install using conan2 I got "Trying to register schema ... already registered ... " and it seems like this is coming from the fact that onnx and onnxruntime both try to register these schemata during their compilation process, and the onnxruntime recipe installs onnx first. onnx has an option DISABLE_STATIC_REGISTRATION which can be used to stop onnx doing this so only onnxruntime will. It seems the simplest way to get this option set is to stick it in your profile; I can't figure out how to set it in the conanfile itself yet:

[options]
onnx/*:shared=False
onnx/*:disable_static_registration=True

I also make sure that onnx is not built as a shared library which should be the default behaviour but isn't necessarily (the recipe could change online!), and it seems from what I read that onnxruntime dynamic linking to onnx can cause issues too.

The other thing is it might be worth mentioning in the instructions in the readme that with onnxrt turned on the minimum standard is C++17, as we can set this using -s compiler.cppstd=17?

@mmcleod89
Copy link
Contributor

Interestingly, even with the double registration, the tests still passed (now that they've been fixed). But it is handy to not have hundreds of error messages first!

@20DM
Copy link
Collaborator Author

20DM commented Dec 14, 2023

The other thing is it might be worth mentioning in the instructions in the readme that with onnxrt turned on the minimum standard is C++17, as we can set this using -s compiler.cppstd=17?

This should already be our default anyway, no?

@mmcleod89
Copy link
Contributor

Conan will use your compiler's default C++ when building dependencies, so for me it was defaulting to C++14. I had to set it to use c++17 explicitly to make things work.

@20DM
Copy link
Collaborator Author

20DM commented Dec 14, 2023

Conan will use your compiler's default C++ when building dependencies, so for me it was defaulting to C++14. I had to set it to use c++17 explicitly to make things work.

Sorry, I misremembered: In Purify we're specifying the C++ standard but not in Sopt. I'll add it to Sopt as well.

@20DM
Copy link
Collaborator Author

20DM commented Dec 14, 2023

Ah no, actually we're doing this already:

 13 set(CMAKE_CXX_STANDARD 17)
 14 set(CMAKE_CXX_STANDARD_REQUIRED ON)

@20DM
Copy link
Collaborator Author

20DM commented Jan 10, 2024

image
Great ...

@mmcleod89 mmcleod89 merged commit a293d1e into development Jan 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants