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

Update to ClimaAtmos v0.11.0 and ClimaLSM v0.2.3 #293

Merged
merged 8 commits into from
May 10, 2023
Merged

Conversation

valeriabarra
Copy link
Member

@valeriabarra valeriabarra commented Apr 27, 2023

Purpose

The Purpose of this PR is to update the experiments/AMIP/modular environment to use the latest ClimaAtmos release, v0.10.0.

In order for CI integration tests to work, I also had to update the experiments/AMIP/moist_mpi_earth environment.

Closes #292 , #297 and #298

Content

  • Modified compat entries in the top-level env, the experiments/AMIP/modular env, and the experiments/AMIP/moist_mpi_earth environments to be able to update using the Package manager
  • Updated to the latest ClimaLSM v0.2.3 release
  • Updated to the latest ClimaAtmos v0.11.0 release
  • Updated the ClimaAtmos-related CLI options
  • Updated to the most recent ClimaCore, ClimaLSM, and ClimaAtmos API

Review checklist

I have:

In the Content, I have included

  • relevant unit tests, and integration tests,

  • appropriate docstrings on all functions, structs, and modules, and included relevant documentation.

  • I have read and checked the items on the review checklist.

@valeriabarra valeriabarra self-assigned this Apr 27, 2023
@valeriabarra valeriabarra changed the title Valeria/atmos v0.10 Update to ClimaAtmos v0.10.0 and ClimaLSM v0.2.3 Apr 27, 2023
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.

We'll need to add something like

      - echo "--- Instantiate amip modular env"
      - "julia --project=experiments/AMIP/modular/ -e 'using Pkg; Pkg.instantiate(;verbose=true)'"
      - "julia --project=experiments/AMIP/modular/ -e 'using Pkg; Pkg.precompile()'"
      - "julia --project=experiments/AMIP/modular/ -e 'using Pkg; Pkg.status()'"

to the buildkite yaml for CI to work. I'm actually not sure how that environment is activated at the moment

Copy link
Member

@juliasloan25 juliasloan25 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 overall. I have one concern though - are you planning to merge this into main as is? It might be better to resolve the failing tests/CI before merging.

experiments/AMIP/modular/Project.toml Show resolved Hide resolved
@valeriabarra
Copy link
Member Author

Looks good overall. I have one concern though - are you planning to merge this into main as is? It might be better to resolve the failing tests/CI before merging.

Hi Julia, thank you. No, absolutely. The idea is not to merge this as is. Of course I want/need to fix CI. I am just also working on other things so I haven't back to this yet. Hopefully this afternoon.

@valeriabarra
Copy link
Member Author

We'll need to add something like

      - echo "--- Instantiate amip modular env"
      - "julia --project=experiments/AMIP/modular/ -e 'using Pkg; Pkg.instantiate(;verbose=true)'"
      - "julia --project=experiments/AMIP/modular/ -e 'using Pkg; Pkg.precompile()'"
      - "julia --project=experiments/AMIP/modular/ -e 'using Pkg; Pkg.status()'"

to the buildkite yaml for CI to work. I'm actually not sure how that environment is activated at the moment

It looks like in CI we're instantiating the other environment experiments/AMIP/moist_mpi_earth. Should I try to use this updated experiments/AMIP/modular in there instead?

@valeriabarra
Copy link
Member Author

@charleskawczynski do you know what is going on with the Ubuntu-latest action?

@charleskawczynski
Copy link
Member

@charleskawczynski do you know what is going on with the Ubuntu-latest action?

It looks like a ClimaLSM error:

home/runner/work/ClimaCoupler.jl/ClimaCoupler.jl/test/conservation_checker_tests.jl:120
  Got exception outside of a @test
  MethodError: no method matching (ClimaLSM.Parameters.LSMParameters{Float64, Float64, Float64})(::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64, ::Float64)
  Stacktrace:
    [1] macro expansion
      @ ~/work/ClimaCoupler.jl/ClimaCoupler.jl/test/conservation_checker_tests.jl:130 [inlined]
...

@charleskawczynski
Copy link
Member

Probably the simplest thing to do, in this particular situation, would be to use their utility:

  • At the top of this test directory, include(joinpath(pkgdir(ClimaLSM), "parameters", "create_parameters.jl"))
  • Then use earth_param_set = create_lsm_parameters(FT) (this function is here) where it's breaking. There's probably been breaking changes to their internal parameter set.

@valeriabarra valeriabarra changed the title Update to ClimaAtmos v0.10.0 and ClimaLSM v0.2.3 WIP Update to ClimaAtmos v0.10.0 and ClimaLSM v0.2.3 Apr 28, 2023
@valeriabarra valeriabarra marked this pull request as draft April 28, 2023 22:06
@valeriabarra
Copy link
Member Author

Probably the simplest thing to do, in this particular situation, would be to use their utility:

  • At the top of this test directory, include(joinpath(pkgdir(ClimaLSM), "parameters", "create_parameters.jl"))
  • Then use earth_param_set = create_lsm_parameters(FT) (this function is here) where it's breaking. There's probably been breaking changes to their internal parameter set.

I fixed it. It was a matter of updating the LSMParameters call, because its API had changed.

@valeriabarra valeriabarra changed the title WIP Update to ClimaAtmos v0.10.0 and ClimaLSM v0.2.3 Update to ClimaAtmos v0.10.0 and ClimaLSM v0.2.3 Apr 28, 2023
@valeriabarra valeriabarra marked this pull request as ready for review April 28, 2023 23:52
@valeriabarra
Copy link
Member Author

This PR is ready for review!
Tests seem to be passing now.
Once approved, I'll reduce commits. Thanks all

@LenkaNovak
Copy link
Collaborator

Hi Valeria, thank you for this! To help with the crashing Buildkites, we’ll need to update the cli_options file by copying the Atmos CLI options here to under the ClimaAtmos flags

@valeriabarra
Copy link
Member Author

Hi Valeria, thank you for this! To help with the crashing Buildkites, we’ll need to update the cli_options file by copying the Atmos CLI options here to under the ClimaAtmos flags

Yes, thank you. I realized that after I wrote my comment. I'll get to it on Monday. I think we're close!

This was linked to issues May 1, 2023
@valeriabarra
Copy link
Member Author

The issue was identified to be in the logic ClimaAtmos tries to prioritize parsed args and populate the toml file (see this line ). Once this is fixed in ClimaAtmos, I'll propagate the change here in the Coupler.

@valeriabarra valeriabarra force-pushed the valeria/atmos-v0.10 branch 2 times, most recently from 8a505be to e735835 Compare May 2, 2023 19:50
@valeriabarra valeriabarra changed the title Update to ClimaAtmos v0.10.0 and ClimaLSM v0.2.3 Update to ClimaAtmos v0.11.0 and ClimaLSM v0.2.3 May 3, 2023
@valeriabarra valeriabarra force-pushed the valeria/atmos-v0.10 branch 5 times, most recently from eb1c268 to 878c8a4 Compare May 5, 2023 00:22
@valeriabarra
Copy link
Member Author

YAY! CI finally passed.

@juliasloan25 , @charleskawczynski if you would like to give a final review, then if approved, I'll go ahead with squashing/reducing commits. Thanks everybody!

Copy link
Member

@juliasloan25 juliasloan25 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! Just a few small comments

experiments/AMIP/modular/Project.toml Outdated Show resolved Hide resolved
experiments/AMIP/modular/components/flux_calculator.jl Outdated Show resolved Hide resolved
perf/Project.toml Outdated Show resolved Hide resolved
Copy link
Member

@juliasloan25 juliasloan25 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! Just a few small comments

@valeriabarra
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request May 9, 2023
293: Update to ClimaAtmos v0.11.0 and ClimaLSM v0.2.3 r=valeriabarra a=valeriabarra

## Purpose 
The Purpose of this PR is to update the `experiments/AMIP/modular` environment to use the latest ClimaAtmos release, `v0.10.0`.

In order for CI integration tests to work, I also had to update the `experiments/AMIP/moist_mpi_earth` environment.

Closes #292 , #297 and #298 

## Content
- Modified `compat` entries in the top-level env, the `experiments/AMIP/modular` env, and the `experiments/AMIP/moist_mpi_earth` environments  to be able to update using the Package manager
- Updated to the latest `ClimaLSM` v0.2.3 release 
- Updated to the latest `ClimaAtmos` v0.11.0 release 
- Updated the `ClimaAtmos`-related CLI options
- Updated to the most recent `ClimaCore`, `ClimaLSM`, and `ClimaAtmos` API

Review checklist

I have:
- followed the codebase contribution guide: https://clima.github.io/ClimateMachine.jl/latest/Contributing/
- followed the style guide: https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/
- followed the documentation policy: https://github.com/CliMA/policies/wiki/Documentation-Policy
- checked that this PR does not duplicate an open PR.

In the Content, I have included 
- relevant unit tests, and integration tests, 
- appropriate docstrings on all functions, structs, and modules, and included relevant documentation.


- [x] I have read and checked the items on the review checklist.


Co-authored-by: Valeria Barra <valeriabarra21@gmail.com>
Co-authored-by: LenkaNovak <lenka@caltech.edu>
@bors
Copy link
Contributor

bors bot commented May 9, 2023

Build failed:

@valeriabarra
Copy link
Member Author

Bors r+

@bors
Copy link
Contributor

bors bot commented May 10, 2023

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.

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