Clean more caches #2287

Sbozzolo · 2023-10-25T00:35:42Z

Clean up cache for:

precipitation
edmfx_nh_pressure
subsidence
EDMFCoriolis
forcing_type
non_orographic_gravity_wave
orographic_gravity_wave
hyperdiffiusion
radiation
limiters

Add surface_setup to AtmosModel and removes dt from simulation

A quick test seem to indicate that this PR reduces compile time by 5-10 % (or more in some cases)

Sbozzolo · 2023-10-25T16:05:56Z

To be merged after #2281

src/parameterized_tendencies/gravity_wave_drag/orographic_gravity_wave.jl

Sbozzolo · 2023-10-26T20:42:26Z

I take back the compile-time benefits. They are not uniform, and there's a regression for some EDMF cases (not all).

I am investigating this.

examples/hybrid/driver.jl

szy21 · 2023-10-26T21:45:22Z

@dennisYatunin may want to take a look at the changes to radiation cache.

src/diagnostics/radiation_diagnostics.jl

Sbozzolo · 2023-10-26T22:09:36Z

@dennisYatunin may want to take a look at the changes to radiation cache.

We agreed to remove one level of nesting to the RRTMPGI cache and rename radiation_mode to radiation_model in a future PR.

szy21

Thanks!

src/parameterized_tendencies/radiation/radiation.jl

Sbozzolo · 2023-10-26T23:08:27Z

I thought I identified and reverted the issue for the slowdown, but that's not the case.

While on my computer, the PR leads to consistent performance/latency improvements, this is not the case on the cluster. Quite the opposite, this PR introduces some noticeable regressions.

I would like to still merge this (and the other, #2296) PR. They clean up and improve the cache, and they are a lot of work to maintain because pretty much every change touches the cache, leading to conflicts that are time consuming to solve.

Debugging this issue is particularly annoying because seemingly there's no correlation between my changes and the outcome (+ I need to run stuff on the cluster to clearly see the effects). I suspect that some of the newly introduce (and non identified) problems will be automatically solved by the mutable spaces.

What do you think?
@szy21 , @charleskawczynski , @dennisYatunin

szy21 · 2023-10-26T23:13:08Z

How much regression did you get on the cluster? If it's not too bad (e.g. ci can still finish in a reasonable time), then I think it's ok to merge it in.

Sbozzolo · 2023-10-26T23:21:01Z

You can compare

https://buildkite.com/clima/climaatmos-ci/builds/14418#_ (which would be the "final" PR)

With master:

https://buildkite.com/clima/climaatmos-ci/builds/14401#_

Some jobs take 10 more minutes:

https://buildkite.com/clima/climaatmos-ci/builds/14418#018b6e11-f634-4293-b119-d2b0d8b59bac

vs

https://buildkite.com/clima/climaatmos-ci/builds/14401#018b6d70-d498-49e9-a8cf-8a3eab19e45c

I think that this is all in compiling solve_atmos!. If you look at everything else (including SYPD), the new PR is better.

szy21 · 2023-10-26T23:38:40Z

hmm, indeed a lot of jobs are slower. @charleskawczynski What do you think?

szy21 · 2023-10-28T21:22:24Z

src/diagnostics/radiation_diagnostics.jl

@@ -96,12 +96,12 @@ function compute_rld!(
    FT = eltype(state)
    if isnothing(out)
        return RRTMGPI.array2field(
-            FT.(cache.radiation_model.face_lw_flux_dn),
+            FT.(cache.atmos.radiation.radiation_model.face_lw_flux_dn),


Is cache.atmos.radiation the same as cache.radiation? (I was wondering why this line is different from e.g. line23)

On a side note, we didn't test all the diagnostics in ci, so it would be hard to catch things like this.

szy21 · 2023-10-28T21:24:49Z

It seems the compile time is back to normal on the cluster now. Feel free to merge it once others' comments are addressed.

Sbozzolo · 2023-10-30T02:05:22Z

In #2244 I add one additional commit to this PR to complete the refactor of the cache by moving all the precomputed quantities in p.precomputed. Overall compile time is nearly identical to the master branch (https://buildkite.com/clima/climaatmos-ci/builds/14519). I would like to merge that and close the chapter of "cache refactoring".

@szy21 do you want to have a look at that commit too? If it the change is okay, I will move it over to this PR and merge it.

szy21 · 2023-10-30T04:00:41Z

The commit looks good to me, thanks! I need to think a bit more about what should be in core and what should be in precomputed, but we don't need to wait for that to merge this PR.

Sbozzolo · 2023-10-30T14:21:53Z

bors r+

2287: Clean more caches r=Sbozzolo a=Sbozzolo Clean up cache for: - precipitation - edmfx_nh_pressure - subsidence - EDMFCoriolis - forcing_type - non_orographic_gravity_wave - orographic_gravity_wave - hyperdiffiusion - radiation - limiters Add surface_setup to AtmosModel and removes dt from simulation A quick test seem to indicate that this PR reduces compile time by 5-10 % (or more in some cases) Co-authored-by: Gabriele Bozzola <[email protected]>

bors · 2023-10-30T14:24:07Z

Build failed:

format

Sbozzolo · 2023-10-30T14:26:04Z

bors r+

bors · 2023-10-30T15:37:10Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

Sbozzolo force-pushed the gb/cache5 branch from 50b8473 to 0dce953 Compare October 25, 2023 16:04

Sbozzolo force-pushed the gb/cache5 branch 9 times, most recently from c9d886a to 0c97bf3 Compare October 26, 2023 18:56

Sbozzolo mentioned this pull request Oct 26, 2023

Turn cache into struct #2296

Merged

Sbozzolo requested a review from szy21 October 26, 2023 19:18

Sbozzolo marked this pull request as ready for review October 26, 2023 19:19

Sbozzolo requested a review from charleskawczynski October 26, 2023 19:34

charleskawczynski reviewed Oct 26, 2023

View reviewed changes

src/parameterized_tendencies/gravity_wave_drag/orographic_gravity_wave.jl Show resolved Hide resolved

szy21 reviewed Oct 26, 2023

View reviewed changes

examples/hybrid/driver.jl Outdated Show resolved Hide resolved

Sbozzolo force-pushed the gb/cache5 branch 3 times, most recently from e5e89c6 to 2dc050c Compare October 26, 2023 21:39

szy21 reviewed Oct 26, 2023

View reviewed changes

src/diagnostics/radiation_diagnostics.jl Outdated Show resolved Hide resolved

szy21 approved these changes Oct 26, 2023

View reviewed changes

src/parameterized_tendencies/radiation/radiation.jl Show resolved Hide resolved

src/parameterized_tendencies/radiation/radiation.jl Show resolved Hide resolved

Sbozzolo force-pushed the gb/cache5 branch from 2dc050c to 41bc2b7 Compare October 26, 2023 22:13

Sbozzolo force-pushed the gb/cache5 branch from 41bc2b7 to e82cf76 Compare October 27, 2023 20:20

szy21 reviewed Oct 28, 2023

View reviewed changes

Sbozzolo added 14 commits October 30, 2023 07:20

Cache: precipitation_cache

46acaae

Remove edmfx_nh_pressure_cache

b206339

Cache: large_scale_advection_cache

0bf5440

Cache: subsidence

4308b81

Cache: EDMFCoriolis

3512fc8

Cache: forcing_type

39983bb

Add held_suarez_ΔT_y_T_equator

c17e649

Cache: non_orographic_gravity_wave_cache

32a8e30

Cache: orographic gravity wave

ef0f027

Cache: radiation

59fede1

Cache: clean up imports

3514e5c

Cache: Remove additional_cache

f525534

Cache: limiters

c12cb57

Cache: hyperdiffiusion

fd73ed3

Sbozzolo force-pushed the gb/cache5 branch from e82cf76 to e351286 Compare October 30, 2023 14:20

Cache: precomputed

51e03ca

Sbozzolo force-pushed the gb/cache5 branch from e351286 to 51e03ca Compare October 30, 2023 14:25

bors bot merged commit 098e92c into main Oct 30, 2023
6 checks passed

bors bot deleted the gb/cache5 branch October 30, 2023 15:37

Sbozzolo mentioned this pull request Oct 30, 2023

Clean up cache #2244

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean more caches #2287

Clean more caches #2287

Sbozzolo commented Oct 25, 2023 •

edited

Loading

Sbozzolo commented Oct 25, 2023

Sbozzolo commented Oct 26, 2023

szy21 commented Oct 26, 2023

Sbozzolo commented Oct 26, 2023

szy21 left a comment

Sbozzolo commented Oct 26, 2023 •

edited

Loading

szy21 commented Oct 26, 2023

Sbozzolo commented Oct 26, 2023

szy21 commented Oct 26, 2023

szy21 Oct 28, 2023

szy21 commented Oct 28, 2023

Sbozzolo commented Oct 30, 2023 •

edited

Loading

szy21 commented Oct 30, 2023

Sbozzolo commented Oct 30, 2023

bors bot commented Oct 30, 2023

Sbozzolo commented Oct 30, 2023

bors bot commented Oct 30, 2023

Clean more caches #2287

Clean more caches #2287

Conversation

Sbozzolo commented Oct 25, 2023 • edited Loading

Sbozzolo commented Oct 25, 2023

Sbozzolo commented Oct 26, 2023

szy21 commented Oct 26, 2023

Sbozzolo commented Oct 26, 2023

szy21 left a comment

Choose a reason for hiding this comment

Sbozzolo commented Oct 26, 2023 • edited Loading

szy21 commented Oct 26, 2023

Sbozzolo commented Oct 26, 2023

szy21 commented Oct 26, 2023

szy21 Oct 28, 2023

Choose a reason for hiding this comment

szy21 commented Oct 28, 2023

Sbozzolo commented Oct 30, 2023 • edited Loading

szy21 commented Oct 30, 2023

Sbozzolo commented Oct 30, 2023

bors bot commented Oct 30, 2023

Sbozzolo commented Oct 30, 2023

bors bot commented Oct 30, 2023

Sbozzolo commented Oct 25, 2023 •

edited

Loading

Sbozzolo commented Oct 26, 2023 •

edited

Loading

Sbozzolo commented Oct 30, 2023 •

edited

Loading