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

Isolate config and argument parsing #1086

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Isolate config and argument parsing #1086

merged 1 commit into from
Dec 2, 2024

Conversation

juliasloan25
Copy link
Member

@juliasloan25 juliasloan25 commented Nov 16, 2024

Purpose

closes #678

This PR isolates the argument and configuration file parsing to functions. Since we have one configuration file each for the coupled simulation and for the atmosphere, we end up with the following functions, as well as smaller helper functions. These are located in the new experiments/ClimaEarth/arg_parsing.jl file.

  • get_coupler_config, get_coupler_args, get_atmos_args

These functions are called from the driver run_amip.jl, where the returned arguments are unpacked. After that point, the config files are not accessed further down in the driver, so we have isolated the config file access to the initial step.

Many files are changed in this PR because argument changes are propagated to many config files. The main files to look at for review are: run_amip.jl, arg_parsing.jl, cli_options.jl, and climaatmos.jl.

Note that this PR introduces some very minor behavioral changes in buildkite runs because the atmos config is no longer added to the coupler config. This will allow us to have better separation of and control over the two in the future.

To-do

  • Add function to read in config file and job id
  • Add function to parse input arguments (from config file)
    • Separate functions for coupler and atmos?
  • Update atmos config function to not modify coupler (make these two independent)
  • Unify argparse and get_args functions - want same parameters available from command line and from config files
  • Allow t_start as an argument
  • Unify dt_cpl to use same format as all other times
  • Change restart_dir default to nothing
  • Rename "diagnostics" -> "extra_atmos_diagnostics" for clarity
  • Check CI outputs

Notes

  • Separate PR: Remove atmos-related args from coupler configs, and move them into atmos configs
    • extra_atmos_diagnostics, EDMF-related arguments
    • This PR will become very large if we do this here, and I'd prefer to change that logic in an isolated PR

@juliasloan25 juliasloan25 force-pushed the js/parsedargs branch 5 times, most recently from 53ef44c to 728b0e2 Compare November 19, 2024 22:37
@@ -1,7 +1,5 @@
FLOAT_TYPE: "Float32"
apply_limiter: false
dt: "400secs"
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed because 400secs is the default for dt and dt_cpl

@juliasloan25 juliasloan25 requested review from imreddyTeja and Sbozzolo and removed request for Sbozzolo and imreddyTeja November 19, 2024 22:57
@juliasloan25 juliasloan25 force-pushed the js/parsedargs branch 2 times, most recently from 31dbea9 to c5ba1c7 Compare November 19, 2024 23:33
@juliasloan25 juliasloan25 force-pushed the js/parsedargs branch 2 times, most recently from 6c02f03 to c6817f3 Compare November 20, 2024 19:32
experiments/ClimaEarth/cli_options.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/cli_options.jl Show resolved Hide resolved
experiments/ClimaEarth/cli_options.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/components/atmosphere/climaatmos.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Outdated Show resolved Hide resolved
@juliasloan25 juliasloan25 force-pushed the js/parsedargs branch 2 times, most recently from d01a9e8 to 54e6b6b Compare November 25, 2024 20:07
@juliasloan25 juliasloan25 force-pushed the js/parsedargs branch 2 times, most recently from 90d1c2a to 1e40048 Compare November 26, 2024 00:11
Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

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

This is a good step in the direction of isolating config/argument parsing. The next step would be to make sure that we don't use the parsed_args anywhere after the initial parsing. This would entail making everything types/symbols and ensuring that we are not matching strings after the initial parsing

experiments/ClimaEarth/cli_options.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Outdated Show resolved Hide resolved
@juliasloan25
Copy link
Member Author

This is a good step in the direction of isolating config/argument parsing. The next step would be to make sure that we don't use the parsed_args anywhere after the initial parsing. This would entail making everything types/symbols and ensuring that we are not matching strings after the initial parsing

If we did this, wouldn't we still use the args we've read in after the initial parsing? We would just use them for dispatch instead of matching strings

@Sbozzolo
Copy link
Member

This is a good step in the direction of isolating config/argument parsing. The next step would be to make sure that we don't use the parsed_args anywhere after the initial parsing. This would entail making everything types/symbols and ensuring that we are not matching strings after the initial parsing

If we did this, wouldn't we still use the args we've read in after the initial parsing? We would just use them for dispatch instead of matching strings

Yes, but this would allow users to run without any argument parsing at all, just setting up the correct types (and only the relevant types, as we might want to do in a CI context). It would also allow users to extend the behavior of functions with their custom types, whereas it is not possible to extend string-matching without changing the source code.

@juliasloan25
Copy link
Member Author

This is a good step in the direction of isolating config/argument parsing. The next step would be to make sure that we don't use the parsed_args anywhere after the initial parsing. This would entail making everything types/symbols and ensuring that we are not matching strings after the initial parsing

If we did this, wouldn't we still use the args we've read in after the initial parsing? We would just use them for dispatch instead of matching strings

Yes, but this would allow users to run without any argument parsing at all, just setting up the correct types (and only the relevant types, as we might want to do in a CI context). It would also allow users to extend the behavior of functions with their custom types, whereas it is not possible to extend string-matching without changing the source code.

I see! That would be nice for things like the turbulent flux partition, albedo to use, etc, but I think we'll still need to parse user inputs for things like dt, t_end, output directory, etc

@juliasloan25 juliasloan25 force-pushed the js/parsedargs branch 2 times, most recently from 86d5031 to aa188b2 Compare November 28, 2024 00:56
experiments/ClimaEarth/cli_options.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Outdated Show resolved Hide resolved
experiments/ClimaEarth/user_io/arg_parsing.jl Show resolved Hide resolved
@juliasloan25 juliasloan25 force-pushed the js/parsedargs branch 2 times, most recently from 73cccba to 703bfab Compare December 2, 2024 18:07
This commit isolates the argument and configuration file parsing to functions. Since we have one configuration file each for the coupled simulation and for the atmosphere, we end up with the following functions, as well as smaller helper functions. These are located in the new `experiments/ClimaEarth/arg_parsing.jl file`.
- `get_coupler_config`, `get_coupler_args`, `get_atmos_args`

These functions are called from the driver `run_amip.jl`, where the returned arguments are unpacked. After that point, the config files are not accessed further down in the driver, so we have isolated the config file access to the initial step.
@juliasloan25 juliasloan25 merged commit 4f0169a into main Dec 2, 2024
11 checks passed
@juliasloan25 juliasloan25 deleted the js/parsedargs branch December 2, 2024 21:40
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 this pull request may close these issues.

Isolate argument parsing and configuration setup
3 participants