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 ClimaCore, switch to using tuples instead of Refs #1333

Merged
merged 2 commits into from
Mar 5, 2023
Merged

Update ClimaCore, switch to using tuples instead of Refs #1333

merged 2 commits into from
Mar 5, 2023

Conversation

simonbyrne
Copy link
Member

@simonbyrne simonbyrne commented Feb 8, 2023 •
edited
Loading

Purpose

Test out latest ClimaCore, including memory fixes (CliMA/ClimaCore.jl#1113)

To-do

Content

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

@simonbyrne
Copy link
Member Author

This is blocked by CliMA/ClimaCore.jl#1124

@simonbyrne
Copy link
Member Author

Need to track down the source of allocations: https://buildkite.com/clima/climaatmos-ci/builds/7041#01863c95-215c-4179-826e-6fa4667e8dd8

@simonbyrne simonbyrne mentioned this pull request Feb 17, 2023
Closed
@simonbyrne
Copy link
Member Author

Looks like EDMF is causing problems, will need to look into it

@simonbyrne simonbyrne self-assigned this Feb 21, 2023
@simonbyrne simonbyrne force-pushed the sb/climacore-tuple branch 2 times, most recently from 185a8d6 to aa47977 Compare February 23, 2023 21:16
@simonbyrne
Copy link
Member Author

Okay, I think this is ready once we tag a new ClimaCore.

@simonbyrne simonbyrne marked this pull request as ready for review March 3, 2023 17:20
@simonbyrne simonbyrne requested a review from szy21 March 3, 2023 17:20
szy21
szy21 approved these changes Mar 3, 2023
View reviewed changes
Copy link
Member

@szy21 szy21 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, thanks! Why does the update (memory fix) increase the allocation for flame_perf_target_edmf though?

@simonbyrne
Copy link
Member Author

I'm not sure: I think it might be the same issue @akshaysridhar was seeing, I just used a higher threshold.

@szy21
Copy link
Member

szy21 commented Mar 3, 2023

The difference Akshay saw is much smaller I think, less than 1e-5？

@simonbyrne
Copy link
Member Author

@szy21 not sure what is going on with the mse tables?

@akshaysridhar
Copy link
Member

The difference Akshay saw is much smaller I think, less than 1e-5？

The magnitude of this is one aspect (I guess this informs our allocation safety-buffer size), the issue was additionally that the allocation change result was not deterministic.

@simonbyrne
Copy link
Member Author

what's the fix for the mse tables stuff?

@szy21
Copy link
Member

szy21 commented Mar 3, 2023

I haven’t looked at the ci. @simonbyrne do you expect behavior change?

@szy21
Copy link
Member

szy21 commented Mar 3, 2023

The results look fine to me. @simonbyrne if you expect behavior change for this PR, and think the allocation change is ok, I can fix the mse and merge it.

@simonbyrne
Copy link
Member Author

no, i don't expect any behavior change

@szy21
Copy link
Member

szy21 commented Mar 3, 2023

no, i don't expect any behavior change

Oh, but the mse table suggests that there are behavior changes?

@simonbyrne
Copy link
Member Author

yeah, i'm not sure what is going on there. Has something changed, because the last test I ran passed: https://buildkite.com/clima/climaatmos-ci/builds/7670#01869ef2-cb56-4b5e-ac81-6f22081d07ae

@szy21
Copy link
Member

szy21 commented Mar 3, 2023

The only PRs that changes mse between Wednesday and now are #1398 and #1327 I think

@akshaysridhar
Copy link
Member

akshaysridhar commented Mar 3, 2023 •
edited
Loading

yeah, i'm not sure what is going on there. Has something changed, because the last test I ran passed: https://buildkite.com/clima/climaatmos-ci/builds/7670#01869ef2-cb56-4b5e-ac81-6f22081d07ae

The ref_counter was bumped from 73->74 in the last build (was this consistent with the main branch at the time of this commit linked here? - 8277a53#diff-2588c3f891993a780927846fac46e12e18cb9043171a3dc6afa39465ad6b7018) - this means the system treats it as a self-reference and that would pass the CI build right ?

@simonbyrne
Update to ClimaCore v0.10.26
7ec124e 
- Updates to latest ClimaCore release
- use 1-tuples instead of Refs for broadcasting
- should fix memory issues
@simonbyrne
Copy link
Member Author

okay, i started a longruns build: https://buildkite.com/clima/climaatmos-longruns/builds/838

@szy21
Copy link
Member

szy21 commented Mar 4, 2023

The results look fine to me.

longrun_hs_rhoe_highres_hightop
image
image

longrun_hs_rhoe_equilmoist_highres_hightop_rayleigh30e3
image
image

@szy21
Copy link
Member

szy21 commented Mar 4, 2023

bors r+

bors bot added a commit that referenced this pull request Mar 4, 2023
@bors @simonbyrne @szy21
Merge #1333
488efe6 
1333: Update ClimaCore, switch to using tuples instead of Refs r=szy21 a=simonbyrne



Co-authored-by: Simon Byrne <simonbyrne@gmail.com>
Co-authored-by: Zhaoyi Shen <11598433+szy21@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Mar 4, 2023

Build failed:

@szy21
Copy link
Member

szy21 commented Mar 4, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 5, 2023

Build succeeded:

@bors bors bot merged commit 9187112 into main Mar 5, 2023
@bors bors bot deleted the sb/climacore-tuple branch March 5, 2023 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szy21 szy21 szy21 approved these changes

Assignees

@simonbyrne simonbyrne

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simonbyrne @szy21 @akshaysridhar