Skip to content
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

Configuration priorities in AtmosCoveragePerfConfig are surprising #2093

Closed
Tracked by #1980
Sbozzolo opened this issue Sep 15, 2023 · 6 comments · Fixed by #2112
Closed
Tracked by #1980

Configuration priorities in AtmosCoveragePerfConfig are surprising #2093

Sbozzolo opened this issue Sep 15, 2023 · 6 comments · Fixed by #2112

Comments

@Sbozzolo
Copy link
Member

Sbozzolo commented Sep 15, 2023

AtmosCoveragePerfConfig advertises itself as creating a model configuration for many performance tests. The docstring says that the priorities for the configuration s are:

Creates a config from the following in order of top priority to last:
1. Configuration from the given config file/dict
2. Default perf configuration
3. Target job configuration
4. Default configuration

This is very unexpected.

The flag target_job says: An (optional) job to target for analyzing performance. So, one would expect that something like

config = CA.AtmosCoveragePerfConfig(config_dict = Dict("target_job" => "sphere_baroclinic_wave_rhoe_equilmoist"))

would run the sphere_baroclinic_wave_rhoe example. Instead, the default perf configuration has higher priority, so, the following configurations are changed (from default_perf.yml):

moist: "equil"
apply_limiter: true
z_elem: 25
dt: "1secs"
surface_setup: "DefaultExchangeCoefficients"
t_end: "10secs"
vert_diff: "true"
h_elem: 12
forcing: "held_suarez"
precip_model: "0M"
dt_save_to_sol: "Inf"
rad: "allskywithclear"

At this point, not much is left of sphere_baroclinic_wave_rhoe and a completely different configuration is being profiled.

Should default_perf.yml be ignored when a target_job is passed? Maybe default_perf should be thought not as a default, but as a all_perf, ie, something that actives several parts of the code so that the benchmark is closer to a realistic complete run.

@Sbozzolo
Copy link
Member Author

Sbozzolo commented Sep 15, 2023

In addition to the above, ClimaAtmos.AtmosCoveragePerfConfig() fails because YAML.load_file(parsed_args["config_file"])) is called with Nothing as argument.

This is not the same behavior as AtmosConfig, which uses the default configuration when called with no options.

@charleskawczynski
Copy link
Member

Yeah, we should fix the doc string, and or move AtmosCoveragePerfConfig outside of src and into a common file for the perf scripts. The main point of using target_job is so that that is the only flag needed to target a particular flag. The main point of AtmosCoveragePerfConfig is to add a lot of physics to our default (dry baro wave driver) so that we can analyze the performance of a many physics kernels. I think the docs were re-written during some refactoring.

@charleskawczynski
Copy link
Member

I'm not sure what you mean by "ClimaAtmos.AtmosCoveragePerfConfig() fails", this is run regularly in our CI.

@charleskawczynski
Copy link
Member

Ah, I see now. This did not used to be the case. @nefrathenrici, can you please fix this? It looks like the config changes broke this functionality

@charleskawczynski
Copy link
Member

I think we can recover the default behavior with ClimaAtmos.AtmosCoveragePerfConfig(;config_dict=Dict()), which isn't too bad. Ultimately, I'd like for these perf scripts to work out of the box. The default being the ClimaAtmos default (the dry baro wave).

@Sbozzolo
Copy link
Member Author

The main point of using target_job is so that that is the only flag needed to target a particular flag.

What do you mean with this? Did you mean "particular job"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants