Skip to content

Some fixes for onnxruntime in TRD, but disabling it since still broken and untested#12964

Merged
davidrohr merged 2 commits intoAliceO2Group:devfrom
davidrohr:dev_pull_request3
Apr 2, 2024
Merged

Some fixes for onnxruntime in TRD, but disabling it since still broken and untested#12964
davidrohr merged 2 commits intoAliceO2Group:devfrom
davidrohr:dev_pull_request3

Conversation

@davidrohr
Copy link
Collaborator

TRD tries to use onnxruntime it seems, but this code is buggy since it checks for ONNXRuntime_FOUND which is never said, so this code was never used. Also the CMake code was wrong lacking the PRIVATE keyword, which was not seen, since it was never used.
This fixes it (requires #12963) and enables the TRD ML.cxx compilation if onnruntime is present.

@martenole : Could you check if this code is actually sane, since it was never used before? The alternative would be to just drop it, but I would not compile something conditionally based on a broken condition.

@github-actions
Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass
async-2023-pp-apass1
async-2022-pp-apass6
async-2022-pp-apass4
async-mc
async-data

@davidrohr davidrohr changed the title Fix TRD usage of onnxruntime Fix TRD usage of onnxruntime - don't merge yet Mar 30, 2024
@davidrohr davidrohr force-pushed the dev_pull_request3 branch from 6d0f75c to f3b6288 Compare April 1, 2024 09:03
@alibuild
Copy link
Collaborator

alibuild commented Apr 1, 2024

Error while checking build/O2/fullCI for f3b6288 at 2024-04-01 11:38:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
/sw/SOURCES/O2/12964-slc8_x86-64/0/Detectors/TRD/pid/src/ML.cxx:129:14: error: 'trdTRD' was not declared in this scope; did you mean 'trkTRD'?
/sw/SOURCES/O2/12964-slc8_x86-64/0/Detectors/TRD/pid/src/ML.cxx:140:27: error: 'input' was not declared in this scope; did you mean 'int'?
/sw/SOURCES/O2/12964-slc8_x86-64/0/Detectors/TRD/pid/src/ML.cxx:143:115: error: 'trkIn' was not declared in this scope
ninja: build stopped: subcommand failed.

Full log here.

@martenole
Copy link
Contributor

@f3sch could you comment on @davidrohr 's question?

@f3sch
Copy link
Collaborator

f3sch commented Apr 2, 2024

Hi @davidrohr and @martenole, the changes to the cmake were made in #10672. Since this code is really not used and but can be revived anytime (VC tracked), I would be fine do drop it completely.

@davidrohr
Copy link
Collaborator Author

Thx, I have disabled it now with if (0), but I am leaving the code in so people know how to reenable it in the correct way (just because it was wrong before...)

@martenole
Copy link
Contributor

Ah and now the CI is unfortunately reset. There was also at least one typo in ML.cxx due to which the compilation failed with ONNX enabled. But OK, fine to leave it out for now and once someone would want to work on that part again it cannot directly be enabled, but also some other fixes will need to be done.

@davidrohr davidrohr changed the title Fix TRD usage of onnxruntime - don't merge yet Some fixes for onnxruntime in TRD, but disabling it since still broken and untested Apr 2, 2024
@davidrohr davidrohr merged commit 4be913d into AliceO2Group:dev Apr 2, 2024
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
…n and untested (AliceO2Group#12964)

* Fix TRD usage of onnxruntime

* Disable ONNX in TRD for now
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
…n and untested (AliceO2Group#12964)

* Fix TRD usage of onnxruntime

* Disable ONNX in TRD for now
mwinn2 pushed a commit to mwinn2/AliceO2 that referenced this pull request Apr 25, 2024
…n and untested (AliceO2Group#12964)

* Fix TRD usage of onnxruntime

* Disable ONNX in TRD for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants