-
Notifications
You must be signed in to change notification settings - Fork 5
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
Use PrecompileTools to compile basic model on build. #2114
base: main
Are you sure you want to change the base?
Conversation
Wow, this crashes hard on TC: Windows
Linux
I think that's the fullspecialize? Also note that the second run of the testmodel is not instant, if anything it's slower. @visr, could you build locally and see what happens? |
I'll first finish the state Vector, which may help here. And if that doesn't help lets try disabling FullSpecialize. Also discussed with @evetion that if all goes well we may be able to only use PrecompileTools and not PackageCompiler precompilation on top. And if PrecompileTools speeds up other models as well we should always enable it. |
Still failing at PackageCompiler creating a sysimage:
This is possibly because with PrecompileTools it will compile more good leading to a sysimage that is too large? The line that it is failing is also using PrecompileTools. Sounds like we need to put this on hold for a little longer. |
On Linux this seems to work now (even though the build log doesn't look like it), as tested on our cluster:
@visr If you can confirm this for the Windows build, I'd say this is good to merge. |
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.
Left some comments, I still have to verify the timings.
# Commented out due to segfault on building | ||
# set_preferences!( | ||
# UUID("0bca4576-84f4-4d90-8ffe-ffa030f20462"), # SciMLBase | ||
# "SpecializationLevel" => "FullSpecialize"; | ||
# force = true, | ||
# ) |
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.
# Commented out due to segfault on building | |
# set_preferences!( | |
# UUID("0bca4576-84f4-4d90-8ffe-ffa030f20462"), # SciMLBase | |
# "SpecializationLevel" => "FullSpecialize"; | |
# force = true, | |
# ) |
I made #2130.
# Commented out due to segfault on building | ||
# set_preferences!( | ||
# UUID("0bca4576-84f4-4d90-8ffe-ffa030f20462"), # SciMLBase | ||
# "SpecializationLevel" => missing; | ||
# force = true, | ||
# ) |
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.
# Commented out due to segfault on building | |
# set_preferences!( | |
# UUID("0bca4576-84f4-4d90-8ffe-ffa030f20462"), # SciMLBase | |
# "SpecializationLevel" => missing; | |
# force = true, | |
# ) |
toml_path = normpath(@__DIR__, "../../generated_testmodels/basic/ribasim.toml") | ||
isfile(toml_path) || return | ||
@compile_workload begin | ||
Ribasim.main(toml_path) |
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.
Ribasim.main(toml_path) | |
main(toml_path) |
Comparing the CLI from this branch against main I don't see any speedup from this at all. The basic model is 1s for both, and the trivial model is 16s for both. Even though trivial is a subset of basic feature wise, hinting that perhaps #2127 won't help much at this point either. That doesn't mean we shouldn't merge it though, hopefully this will start making a difference with further latency work. |
Should help with latency (#1942) on our executables. The basic testmodel runs in half a second, and models related to basic see improvements too (but much less) with this PR.
This sets up a precompile workload using PrecompileTools, which also compiles the statements of other packages with our types, instead of just Ribasim. This also sets the specialization level of SciML to full, which is recommended for simulations.
However, as both will significantly increase compile time (and the model printing progress during it), I've disabled it by default for developers.
I believe that if we fix the type explosion of our Parameters we will have a fast executable for all models, although we ideally should have a workload that includes all our functionality (arrow, alloc, etc.).