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.

Sign up for GitHub

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

Jump to bottom

Update Insolation to latest release #195

Merged
merged 3 commits into from
May 16, 2023
Merged

Update Insolation to latest release #195

merged 3 commits into from
May 16, 2023

Conversation

valeriabarra
Copy link
Member

@valeriabarra valeriabarra commented May 11, 2023 •
edited
Loading

Purpose

The purpose of this PR is to update the compat entry for Insolation, given its latest release.

This should fix compatibility issues downstream in the Coupler

Todo / Related PRs

Content

  • Updated Insolation compat entry in the top-level Project.toml file
  • Applied necessary API changes

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 marked this pull request as draft May 11, 2023 01:45
@valeriabarra
Copy link
Member Author

I cannot reproduce the current failure

SystemError: opening file "/home/runner/.julia/packages/Insolation/IKhEM/src/Artifacts.toml": Permission denied

that happens in the Vegetation/canopy_model.jl:18 test, which calls the get_data_folder function on this Insolation.jl line. The test passes locally for me.

charleskawczynski
charleskawczynski reviewed May 13, 2023
View reviewed changes
Copy link
Member

@charleskawczynski charleskawczynski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I left a comment. It's a bit inconvenient that we I can't seem to leave comments without adding a non-empty review comment. The single comment ends up not visible and sits pending until a review comment is made. 🙁

@valeriabarra
Copy link
Member Author

I left a comment

Did you see my question about the failure that I cannot reproduce locally?

@valeriabarra
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request May 15, 2023
@bors
Try #195:
0880329
@bors
Copy link
Contributor

bors bot commented May 15, 2023

try

Build failed:

@charleskawczynski charleskawczynski mentioned this pull request May 15, 2023
Closed
@valeriabarra valeriabarra mentioned this pull request May 15, 2023
Closed
8 tasks
@valeriabarra
Copy link
Member Author

The issue was identified and it will be fixed in CliMA/Insolation.jl#47

@charleskawczynski
Copy link
Member

@valeriabarra, once CliMA/Insolation.jl#47 lands and a new release is available, let's apply

on-top of this PR.

@valeriabarra valeriabarra force-pushed the valeria/update-insolation branch 2 times, most recently from 333f564 to 7c841b0 Compare May 16, 2023 17:54
@valeriabarra valeriabarra marked this pull request as ready for review May 16, 2023 18:59
@valeriabarra
Copy link
Member Author

@juliasloan25 , @charleskawczynski CI passes now in this PR.
If/when approved, I will reduce the number of commits. Thanks a lot!

charleskawczynski
charleskawczynski reviewed May 16, 2023
View reviewed changes
Project.toml Outdated Show resolved Hide resolved
charleskawczynski
charleskawczynski approved these changes May 16, 2023
View reviewed changes
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.

Code changes look good to me. I think we only need to fix some compat bounds.

juliasloan25
juliasloan25 approved these changes May 16, 2023
View reviewed changes
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!

@valeriabarra valeriabarra changed the title Update Insolation compat entry after latest release Update Insolation to latest release May 16, 2023
@valeriabarra
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented May 16, 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.

@bors bors bot merged commit 5e986e3 into main May 16, 2023
@bors bors bot deleted the valeria/update-insolation branch May 16, 2023 21:54
mitraA90 pushed a commit that referenced this pull request Dec 22, 2023
@bors @valeriabarra @charleskawczynski
Merge #195
3f426a1 
195: Update Insolation to latest release r=valeriabarra a=valeriabarra

## Purpose 
The purpose of this PR is to update the `compat` entry for Insolation, given its latest release. 

This should fix compatibility issues downstream in the Coupler

## Todo / Related PRs
- [x] This is blocking this Coupler PR CliMA/ClimaCoupler.jl#306

## Content
- Updated Insolation compat entry in the top-level Project.toml file
- Applied necessary API changes


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 <[email protected]>
Co-authored-by: Charles Kawczynski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charleskawczynski charleskawczynski charleskawczynski approved these changes

@juliasloan25 juliasloan25 juliasloan25 approved these changes

@simonbyrne simonbyrne Awaiting requested review from simonbyrne

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@valeriabarra @charleskawczynski @juliasloan25