-
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
Fix inference failure in callbacks #318
Conversation
How does this affect compilation times? |
Better inferred code correlates with shorter compilation time, but heuristics are involved, so we’d need to test it out to know for sure. My guess is that this shouldn’t have a notable impact. |
Maybe I should have said inference + compilation. Better inferred code can lead to longer inference + compilation time (it's much easier to propagate
I also think so because we don't have many callbacks, but it would be great if you could double-check this before merging. |
One reason to continue with this is that, sometimes, JET will show inference failures up to a point, and bail on showing more. This inference failure seems to be the last one seen in a handful of jobs in CliMA/ClimaAtmos.jl#3290. It very well may be that this is the last one, however, it’s possible that it’s the last failure before a bunch more, which is what I’d like to know. Ultimately, fixing inference failures is a sort of due diligence in that latency is often attributed to poorly inferred code. I will say though, CliMA/ClimaAtmos.jl#3290 is increasingly making me think that that JuliaLang/julia#55807 is the real issue. |
sure, I’ll run clima atmos with this branch tomorrow |
(cc @Sbozzolo) |
Thank you! |
This PR fixes the broken inference test for the callback loop. Closes #316 and #315.