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

Throw a more informative error for unsupported config dictionary entries #2844

Merged
merged 5 commits into from
Mar 28, 2024

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Mar 27, 2024

This PR helps users debug configuration dictionaries by throwing an error that indicates which entry is problematic and why.

After this PR,

using ClimaAtmos
configuration_dict = Dict("hyperdiff" => nothing)
configuration = ClimaAtmos.AtmosConfig(configuration_dict)

throw the error

ERROR: LoadError: ArgumentError: Configuration entry "hyperdiff" = nothing has type Nothing,
but must have type String.
Stacktrace:
 [1] override_default_config(config_dict::Dict{String, Nothing})
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/yaml_helper.jl:84
 [2] ClimaAtmos.AtmosConfig(config::Dict{String, Nothing}; comms_ctx::Nothing)
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/types.jl:487
 [3] ClimaAtmos.AtmosConfig(config::Dict{String, Nothing})
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/types.jl:486
 [4] top-level scope
   @ ~/Projects/ClimaEarth.jl/examples/atmos_single_column_model/test.jl:31
 [5] include(fname::String)
   @ Base.MainInclude ./client.jl:489
 [6] top-level scope
   @ REPL[1]:1
in expression starting at /Users/gregorywagner/Projects/ClimaEarth.jl/examples/atmos_single_column_model/test.jl:31

caused by: MethodError: no method matching String(::Nothing)

Closest candidates are:
  String(::Base.CodeUnits{UInt8, String})
   @ Base strings/string.jl:107
  String(::Vector{UInt8})
   @ Base strings/string.jl:67
  String(::LazyString)
   @ Base strings/lazy.jl:80
  ...

Stacktrace:
 [1] override_default_config(config_dict::Dict{String, Nothing})
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/yaml_helper.jl:79
 [2] ClimaAtmos.AtmosConfig(config::Dict{String, Nothing}; comms_ctx::Nothing)
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/types.jl:487
 [3] ClimaAtmos.AtmosConfig(config::Dict{String, Nothing})
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/types.jl:486
 [4] top-level scope
   @ ~/Projects/ClimaEarth.jl/examples/atmos_single_column_model/test.jl:31
 [5] include(fname::String)
   @ Base.MainInclude ./client.jl:489
 [6] top-level scope
   @ REPL[1]:1

whereas previously users were greeted with the more mysterious

MethodError: no method matching String(::Nothing)

(I'm also wondering how to turn hyperdiffusion off).

It also improves the error users get when their "config" is not valid (note that the term "config" is being used to mean multiple things --- either AtmosConfig, or the "config type", which can be sphere, column, etc. This should probably be improved because it is another source of bugs...)

For example if we try to use config = "crazy configuration" we get the error:

ERROR: LoadError: ArgumentError: config = crazy configuration is not one of the valid configurations ("sphere", "column", "box", "plane")
Stacktrace:
 [1] get_model_config(parsed_args::Dict{Any, Any})
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/model_getters.jl:21
 [2] get_atmos(config::ClimaAtmos.AtmosConfig{…}, params::ClimaAtmos.Parameters.ClimaAtmosParameters{…})
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/type_getters.jl:50
 [3] get_simulation(config::ClimaAtmos.AtmosConfig{Float32, ClimaParams.ParamDict{Float32}, Dict{Any, Any}, ClimaComms.SingletonCommsContext{ClimaComms.CPUSingleThreaded}})
   @ ClimaAtmos ~/Projects/ClimaAtmos.jl/src/solver/type_getters.jl:576

Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I added @nefrathenrici to take a look, too.

hyperdiff does have an entry in our automated docs: https://clima.github.io/ClimaAtmos.jl/dev/config/#Default-Configuration. It looks like none turns off hyperdiffusion

@glwagner
Copy link
Member Author

Ok great, I didn't know about that docs page.

The docs also suggest making a yml file, but it doesn't seem like this is actually required. Nor should it be I think, it's easier to work with just one file/script rather than multiple ones.

Are config dictionaries fully supported, or are there options that can only be provided in a yml file? I'm happy to work on making the changes that allow us to avoid the yml file step (this will be especially useful for developing a coupled model).

@charleskawczynski
Copy link
Member

The docs also suggest making a yml file, but it doesn't seem like this is actually required. Nor should it be I think, it's easier to work with just one file/script rather than multiple ones.

I agree, but I do think that there is a split between the parameters being tuned, and those not.

Are config dictionaries fully supported, or are there options that can only be provided in a yml file? I'm happy to work on making the changes that allow us to avoid the yml file step (this will be especially useful for developing a coupled model).

So, we can technically do whatever we want in the REPL. The driver has

if !(@isdefined config)
    config = CA.AtmosConfig()
end
simulation = CA.get_simulation(config)
(; integrator) = simulation
sol_res = CA.solve_atmos!(simulation)

And you can define your own config, which will then get used.

For example, you could do:

import ClimaAtmos as CA
config = CA.AtmosConfig()
config.parsed_args["rad"] = "clearsky"; # use clearsky RRTMGP
# run with this config:
include(joinpath(pkgdir(CA), "examples", "hybrid", "driver.jl"))

Also, there is TargetJobConfig in the perf folder, which I often use like this:

import ClimaAtmos as CA
include(joinpath(pkgdir(CA), "perf", "common.jl"))
config = TargetJobConfig("gpu_aquaplanet_dyamond") # get GPU aquaplanet dyamond job config, from our buildkite CI
# run with this config:
include(joinpath(pkgdir(CA), "examples", "hybrid", "driver.jl"))

@glwagner
Copy link
Member Author

I agree, but I do think that there is a split between the parameters being tuned, and those not.

The point is just that for developing the Earth system model, we'd like to be able to build AtmosSimulation without the yml file.

Changing parsed_args after building the config sounds like an ok solution as long as we don't have to change too many things to get the setup we want.

@glwagner
Copy link
Member Author

Note that we don't want to run the driver... we need to build AtmosSimulation and then incorporate it into an Earth system model.

Copy link
Member

@nefrathenrici nefrathenrici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this, the error before was very unclear.

Config dictionaries are fully supported, and are the easiest way to set the configuration interactively:

import ClimaAtmos as CA
config_dict = Dict("hyperdiff" => "false", "t_end" => "10days")
config = CA.AtmosConfig(config_dict)
simulation = CA.get_simulation(config)
sol_res = CA.solve_atmos!(simulation)

@glwagner
Copy link
Member Author

Thank you for adding this, the error before was very unclear.

Config dictionaries are fully supported, and are the easiest way to set the configuration interactively:

import ClimaAtmos as CA
config_dict = Dict("hyperdiff" => "false", "t_end" => "10days")
config = CA.AtmosConfig(config_dict)
simulation = CA.get_simulation(config)
sol_res = CA.solve_atmos!(simulation)

Awesome thank you! Just as a side comment, we don't want to use solve_atmos! since we have to couple with other components between time-steps. But the example is helpful, thank you!

PS I think get_simulation should just be called AtmosSimulation since it is a constructor. It will help us understand the code!

@glwagner
Copy link
Member Author

glwagner commented Mar 28, 2024

I made one more change to error messages. I mixed up the config (as in AtmosConfig) with the dictionary key "config", leading to an AssertionError that I could not easily debug. So I change the AssertionError to an ArgumentError with an informative error message. Let me know if this is ok. I guess I formatted it wrong too unfortunately.

@glwagner glwagner added the Launch Buildkite Add to launch buildkite run label Mar 28, 2024
@glwagner glwagner enabled auto-merge March 28, 2024 20:18
@glwagner glwagner added this pull request to the merge queue Mar 28, 2024
@glwagner
Copy link
Member Author

glwagner commented Mar 28, 2024

For example, you could do:

import ClimaAtmos as CA
config = CA.AtmosConfig()
config.parsed_args["rad"] = "clearsky"; # use clearsky RRTMGP
# run with this config:
include(joinpath(pkgdir(CA), "examples", "hybrid", "driver.jl"))

Just a note --- this approach recommended by @charleskawczynski is not fully supported right now, because FLOAT_TYPE cannot be correctly set this way (apparently, the float type cannot be changed after AtmosConfig is created). So either we'd have to fix that, or we have to recommend first constructing the args::Dict and then passing them to AtmosConfig(args).

Merged via the queue into main with commit d6f2aa6 Mar 28, 2024
11 checks passed
@glwagner glwagner deleted the glw/yaml-error branch March 28, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Launch Buildkite Add to launch buildkite run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants