-
Notifications
You must be signed in to change notification settings - Fork 4
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
Use ClimaAtmos new config file interface #395
Conversation
85f9435
to
7588afb
Compare
ed70bd0
to
bf44bc6
Compare
bors try |
tryBuild failed: |
c1601d6
to
90075f9
Compare
17e1ef6
to
95b5959
Compare
ec99c75
to
5504640
Compare
CI passes (I just rebased onto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about some of the perf changes, but the atmos interface changes all look good to me! Thanks for dealing with the new config flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job on this, @valeriabarra ! I have a few clarification comments and suggestions to re-enable interactive runs.
@@ -9,6 +9,9 @@ env: | |||
BUILDKITE_BRANCH: "${BUILDKITE_BRANCH}" | |||
JULIA_MAX_NUM_PRECOMPILE_FILES: 100 | |||
GKSwstype: 100 | |||
CONFIG_PATH: "config/model_configs" | |||
PERF_CONFIG_PATH: "config/perf_configs" | |||
MPI_CONFIG_PATH: "config/mpi_configs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be clearer to call this mpi_unit_test_configs
since some of the integration test configs also use MPI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had followed the same name convention they have in Atmos. Do we care? Or we don't mind having them different?
For other cases (e.g., ClimaLSM, I had seen we tend to follow the same name conventions)
Interesting, thanks! I had tested it interactively (in fact, it was one of the checkboxes in the PR description). Let me take a closer look. Perhaps that was some time before I made some latest changes. Thanks for noticing |
bors try |
tryBuild failed: |
Hi all, thanks for the review. I believed I replied/addressed all comments and CI passes. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @valeriabarra , thanks for the revisions! Look good, but I'm noticing that the flame graphs are not being generated on Buildkite anymore. I'm also getting some errors with TemperstRemap when running interactively. I should have some time to debug/merge this next week. Does that work for you?
Oh, so sorry for not noticing that. They were fixed before the revision. I must have missed something. Of course, if you can, please. Otherwise let me know if I can help. Thanks a lot! |
b943ea3
to
d072248
Compare
CI: Renamed AMIP - modular, Float32 test -> AMIP - modular, Float64 test Apply reviewer's comments Use different merge order in interactive/non-interactive mode flame test modification trigger perf configs using model configs Update to ClimaAtmos v0.16.0: Use config file interface CI: Renamed AMIP - modular, Float32 test -> AMIP - modular, Float64 test flame test modification trigger perf configs using model configs rm perf LSM dep Update to ClimaAtmos v0.16.0: Use config file interface CI: Renamed AMIP - modular, Float32 test -> AMIP - modular, Float64 test Apply reviewer's comments Use different merge order in interactive/non-interactive mode flame test modification trigger perf configs using model configs rm ClimaLSM deps of sub folders fix rebase Buildkite name fix spec run_name hdf5 mpi fix try fix perf runs revert MPI load try MPI specs revert revert mpi write debug - barrier try try CC env vars try CC env vars srun CA16 BK setup flame fix j1.9 Manifest v1.9 local pass add JuliaProject.toml revert Projects runs serially, try JuliaProject, J1.8.5 srun for mpi jobs add srun for amip rm depot path rm CLIMACOMMS_CONTEXT test JULIA_DEPOT_PATH race in mpi tests race in mpi tests race in mpi tests race in mpi tests race in mpi tests show pid race in mpi tests show pid device race in mpi tests race in AMIP cleanup cleanup add notes to local mpi tests add notes to local mpi tests GKSwstype: 100
d072248
to
45f86f6
Compare
bors r+ |
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. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Purpose
This PR updates ClimaAtmos to v0.16.0 and its new interface for configuration files (rather than CLI options), and keeps the existing functionality for the Coupler to still use CLI options.
This new interface reduces the maintenance burden and code duplication that we used to have (we used to manually copy/paste ClimaAtmos' default CLI options). Now, if any default in ClimaAtmos will change in the future, the Coupler won't need to overwrite them. They will be reflected directly in the Coupler.
Closes #388
To-do
Content
--config_file
)"AMIP - modular, Float32 test"
->"AMIP - modular, Float64 test"
, since we were using Float64 in that testflame.jl
andflame_diff.jl
scripts (they were referring to an oldjob_id
target_amip_n32_shortrun
that we don't use anymore)Remap and MPI trouble shooting (issues that came up after cluster upgrade)
JULIA_LOAD_PATH
(soon to be resolved with the TempestRemap new release, but for now we need to load packages in a specific order)const comms_ctx = ClimaComms.context(ClimaComms.CPUSingleThreaded())
run_name
Review checklist
I have:
In the Content, I have included
-->