-
Notifications
You must be signed in to change notification settings - Fork 434
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
Library for ORT execution #13522
base: dev
Are you sure you want to change the base?
Library for ORT execution #13522
Conversation
…at16_t. ort_interface.h now usuable as library (not header!) :party:
Ping @davidrohr |
REQUEST FOR PRODUCTION RELEASES:
This will add The following labels are available |
Common/ML/CMakeLists.txt
Outdated
o2_add_library(ML | ||
SOURCES src/ort_interface.cxx | ||
TARGETVARNAME targetName | ||
PUBLIC_LINK_LIBRARIES O2::Framework ONNXRuntime::ONNXRuntime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brauchst du hier PUBLIC, oder geht PRIVATE auch?
Common/ML/include/ML/GPUORTFloat16.h
Outdated
// or submit itself to any jurisdiction. | ||
|
||
/// \file GPUORTFloat16.h | ||
/// \author Christian Sonnabend <[email protected]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hast du das selbst geschrieben, oder von github kopiert?
In letzten Fall musst du den korrekten Autor angeben, und schauen ob der code unter GPL3 lizensiert ist. Jedenfalls haben wir kein copyright dafür.
Ansonten leg ein 3rdparty Verzeichnis wie Framework/Foundation/3rdparty
oder GPU/GPUTracking/display/3rdparty
an. Das wird dann auch im CodeChecker ausgenommen: https://github.com/alisw/alidist/blob/1916f6d88d42959097998d9481b517dc1c1ea84d/o2checkcode.sh#L71
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ja, das ist komplett 3rd party von ONNX. Die source geb ich auch im header an, da kopier ich dann vielleicht einfach direkt die file vom ONNX repo und pack die in das Verzeichnis das du oben angegeben hast
Common/ML/include/ML/ort_interface.h
Outdated
#include <thread> | ||
|
||
// O2 includes | ||
#include "GPUORTFloat16.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wenn du den Datentyp hier nicht benutzt, solltest du den header auch nicht includen.
@@ -81,6 +81,11 @@ if(OpenMP_CXX_FOUND) | |||
target_link_libraries(${mergertargetName} PRIVATE OpenMP::OpenMP_CXX) | |||
endif() | |||
|
|||
o2_add_executable(onnx-interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wenn es ein test ist, vielleicht o2_add_test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Den workflow kann ich für einen PR, der gemerged wird, sowie so weglassen. Das ist nur momentan zum testen und würd ich dann rausnehmen
Ideally, we should not have merge commits. Can you rebase without merge commits. Also, you should squash cleanups. |
I won't need the history afterwards. For now I'm just pushing the recent changes into this branch. I will also soon add the code in the actual clusterization code (although I might open a separate PR for that) |
Adding a library for the execution of ONNX models with GPU execution provider and float16 datatype (OrtDataType::Float16_t -> a wrapper around a uint16_t)