-
Notifications
You must be signed in to change notification settings - Fork 112
Faster engine compile time #316
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
Conversation
rgsl888prabhu
left a comment
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.
Thank you @Kh4ster for the PR, I have requested few changes and clarification.
build.sh
Outdated
| SKIP_C_PYTHON_ADAPTERS=0 | ||
| SKIP_TESTS_BUILD=0 | ||
| SKIP_ROUTING_BUILD=0 | ||
| WRITE_FATBIN=0 |
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.
WRITE FATBIN should be by default enabled, and disabled with option
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.
Changed!
| option(BUILD_TESTS "Configure CMake to build tests" ON) | ||
| option(DISABLE_OPENMP "Disable OpenMP" OFF) | ||
| option(CUDA_STATIC_RUNTIME "Statically link the CUDA toolkit runtime and libraries" OFF) | ||
| option(BUILD_LP_ONLY "Build only linear programming components, exclude routing and MIP-specific files" OFF) |
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.
Missing BUILD_TESTS
WRITE_FATBIN and BUILD_FATBIN is being used.
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.
Build test is above. We had it already but it was not used and not working in practice
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.
Good catch for the BUILD_FATBIN!
| BUILD_CI_ONLY=0 | ||
| BUILD_LP_ONLY=0 | ||
| SKIP_C_PYTHON_ADAPTERS=0 | ||
| SKIP_TESTS_BUILD=0 |
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.
Dow we need to add option to skip BUILD_MIP_BENCHMARKS
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 think we can keep this for now and see if someone needs it
| PRIVATE "$<$<COMPILE_LANGUAGE:CXX>:${CUOPT_CXX_FLAGS}>" | ||
| "$<$<COMPILE_LANGUAGE:CUDA>:${CUOPT_CUDA_FLAGS}>" | ||
| ) | ||
| if(NOT BUILD_LP_ONLY) |
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.
Should this be
| if(NOT BUILD_LP_ONLY) | |
| if(BUILD_LP_ONLY) |
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.
No I confirm we don't want to have cuopt_cli if we are in lp_only mode since there is a call to the MIP solver
| add_library(cuopttestutils STATIC | ||
| routing/utilities/check_constraints.cu | ||
| ) | ||
| if(BUILD_TESTS) |
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.
Is there an option where we can skip directly from cpp/CMakefiles.txt
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 know any. That's the only way I found to truely diable the compilation of tests
| } | ||
| ]=]) | ||
| target_link_options(mps_parser PRIVATE "${MPS_PARSER_BINARY_DIR}/fatbin.ld") | ||
| if(WRITE_FATBIN) |
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.
Remove this always?
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.
What do you mean? Remove the fatbin writing completly?
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 think he is asking, by default currently this was disabled, so do we need to disable it by default might be the question. Think I also had asked this question.
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.
And you have changed it if I am not wrong.
|
/ok to test 0e3191a |
|
/ok to test 3a9bac7 |
|
/ok to test 7bc2ce4 |
|
/ok to test 2ad2c73 |
@Kh4ster, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 33e9344 |
|
/ok to test 3a3e7c9 |
| instance_name=$(basename $instance) | ||
| echo "Parsing ${instance_name}.mps then solving" | ||
| ${CUOPT_HOME}/cpp/build/solve_MPS_file --path ${CUOPT_HOME}/benchmarks/linear_programming/datasets/${instance_name}/${instance_name}.mps --time-limit 3600 # --solution-path $CUOPT_HOME/benchmarks/linear_programming/datasets/$instance.sol | ||
| ${CUOPT_HOME}/cpp/build/solve_LP --path ${CUOPT_HOME}/benchmarks/linear_programming/datasets/${instance_name}/${instance_name}.mps --time-limit 3600 # --solution-path $CUOPT_HOME/benchmarks/linear_programming/datasets/$instance.sol |
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.
Let's start using cuopt_cli here? That's what Hans would be using as well.
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.
same comment as below
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.
Here, you are not using any of those additional settings/options from run_pdlp right? We can still use run_pdlp for when benchmarking with those settings.
| target_compile_options(solve_MPS_file | ||
| option(BUILD_LP_BENCHMARKS "Build LP benchmarks" OFF) | ||
| if(BUILD_LP_BENCHMARKS) | ||
| add_executable(solve_LP ../benchmarks/linear_programming/cuopt/run_pdlp.cu) |
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.
For benchmarking, I think we should directly use the cuopt_cli, because that's what everyone will use
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.
The issue I have is run_pdlp has more options that cuopt_cli
|
/ok to test 3a3e7c9 |
|
/ok to test 50e56b8 |
|
/ok to test c241602 |
aliceb-nv
left a comment
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.
Approving :) Awesome work Nicolas, this is going to be super useful
Is it ready to merge?
|
/merge |
1 similar comment
|
/merge |
This PR aims at making the compile time faster by adding several options: - build lp only (extracted MIP files necessary in LP), also meant we needed a skip c and python adapter option - put back no fetch rapids - put back not building test option - added skip routing - added skip write of fatbin - adding ccache to libcuopt target - added two benchmarking targets, one for LP and for MIP Authors: - Nicolas Blin (https://github.com/Kh4ster) Approvers: - Alice Boucher (https://github.com/aliceb-nv) - Rajesh Gandham (https://github.com/rg20) - Ramakrishnap (https://github.com/rgsl888prabhu) URL: #316
This PR aims at making the compile time faster by adding several options: