-
Notifications
You must be signed in to change notification settings - Fork 373
Disabled building tests default (like LLVM), enabled for our CIs #3021
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
base: main
Are you sure you want to change the base?
Disabled building tests default (like LLVM), enabled for our CIs #3021
Conversation
Signed-off-by: Alexandre Eichenberger <[email protected]>
|
From the log from linux/amd So it appears to build & test the tests in the CI for Linux/AMD I will double check that it worked too for s390 once the CI completes. |
|
@AlexandreEichenberger I am curious to know if without building onnx, can you compile a .onnx model or not? Numerical tests do not use .onnx files as input, so it does not tell whether we can compile a .onnx model. Just to double check. |
|
|
||
| if (NOT ONNX_MLIR_BUILD_TESTS) | ||
| set(EXCLUDE_FROM_ALL ON) | ||
| endif() |
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.
I don't think this does anything. EXCLUDE_FROM_ALL is a target property. Setting a variable called EXCLUDE_FROM_ALL won't do anything. The proper use of EXCLUDE_FROM_ALL should be like the code in MLIR.cmake:
if (EXCLUDE_FROM_ALL)
add_executable(${name} EXCLUDE_FROM_ALL ${ARG_UNPARSED_ARGUMENTS})
else()
add_executable(${name} ${ARG_UNPARSED_ARGUMENTS})
endif()
The variable EXCLUDE_FROM_ALL is used to decide whether to add the property EXCLUDE_FROM_ALL to a target.
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.
@gongsu832 I reused this pattern from the current onnx-mlir. I know it does not make it right, but on my Mac it just seems to work. When I set the variable as ON, it builds all the tests as before. When I set it to off, the resulting build does not recognize the make check-onyx-backend as a valid target.
Let me run the experiment on LoZ too.
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.
I assume you meant check-onnx-backend since there is no check-onyx-backend.
Regardless of how the variable is set, check-onnx-backend should always be a valid target. The difference is whether it's included in the default all target.
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.
Also note that add_executable, add_library, etc. behave differently from add_custom_target. By default, add_executable, add_library, etc. will add the target to all unless you specify EXCLUDE_FROM_ALL on the target. add_custom_target, on the other hand, by default will not add the target to all unless you specify ALL on the target.
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.
@gongsu832 Are there specific steps you would like me to take? Or if it takes you less time, do you want to take over this PR to do it the right way (if the current approach is not the preferred approach).
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.
I can do it but probably won't be able to get to it until some time next week.
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.
Thanks, there is no rush, glad it will be done using the proper approach
@tungld Here is the experiment I ran. I executed a That was on a Mac, let me retry all this on a LoZ |
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Signed-off-by: Alexandre Eichenberger <[email protected]>
Thank you for the confirmation! That sounds good. Let @gongsu832 give the final decision since the most changes are about CMakeFiles. |
As mentioned in #1059, our default builds require onnx to be installed.
This PR implements the changes requested in that issue:
ONNX_MLIR_BUILD_TESTSbeingONONNX_MLIR_BUILD_TESTSvalue switched to off.ONNX_MLIR_BUILD_TESTS=ONdefine.That way, folks just building a default
onnx-mlirwill not build the tests, and thus we are not dependent ononnxthird party to be installed.For folks using their own scripts for testing, you must now add
to your
cmakecommand.To respond to issue #1059, I also
pip uninstal onnxand tested that we can build a working compiler without onnx on my local Mac machine, and it worked.