cpp/purify/config.in.h: Add #cmakedefine PURIFY_ONNXRT #360
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To enable onnxrt (onnxruntime) supporting the compiled C++ code, PURIFY_ONNXRT must be defined at the preprocessor level. On the cmake level, it is set here: https://github.com/astro-informatics/purify/blob/development/cmake_files/dependencies.cmake#L37 But it needs to be conditinally defined in config.h(.in) to reach the C++ level (actual compilation): //! Whether PURIFY is running with onnxrt (onnxruntime) #cmakedefine PURIFY_ONNXRT Without this, even with `cmake -Donnxrt=ON`, strings bin/purify | grep onnxrt will find a string that is compiled only when PURIFY_ONNXRT is not set by config.h (and it is not if this cmakedefine is not set).
tkoskela
approved these changes
Oct 14, 2024
There was a problem hiding this comment.
Thank you for the sugestion, it looks good to me!
It looks like the double to float conversion error you ran into has already been fixed in the development branch astro-informatics/sopt@e0aab12. I'll update the purify onnx variant once we've got a new release.
|
Going to close this as the change is also already in #343 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@tkoskela @20DM:
To enable onnxrt (onnxruntime) supporting the compiled C++ code, PURIFY_ONNXRT must be defined at the preprocessor level:
https://github.com/astro-informatics/purify/blob/development/cpp/purify/algorithm_factory.h#L27
https://github.com/astro-informatics/purify/blob/development/cpp/purify/algorithm_factory.h#L204
(these are the only places that I found)
This string is found in
bin/purifyif the ONNXRT code is not compiled:https://github.com/astro-informatics/purify/blob/development/cpp/purify/algorithm_factory.h#L210
On the cmake level, it is set here:
https://github.com/astro-informatics/purify/blob/development/cmake_files/dependencies.cmake#L37
But it needs to be conditinally defined in config.h(.in) to reach the C++ level (actual compilation):
Without this, even with
cmake -Donnxrt=ON,strings bin/purify | grep onnxrtwill find a string that is compiled only when PURIFY_ONNXRT is not set by config.h (and it is not if this cmakedefine is not set).PS: I noticed this in an extended review of spack/spack#46839
PPS: With this, PURIFY_ONNXRT is set, onnxrt code gets included in the build, but compilation with gcc-13.1.0 fails with these errors:
This used py-onnxruntime@1.18.2 and sopt@4.2.0