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

CI with Julia v1.11 #3836

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

CI with Julia v1.11 #3836

wants to merge 27 commits into from

Conversation

navidcy
Copy link
Collaborator

@navidcy navidcy commented Oct 8, 2024

This PR switches the CI to use Julia v1.11.
It also adds a Manifest with v1.11 ending so that there is still compatibility with previous versions.

Note the the distributed CI still does not have Julia v1.11 (right @Sbozzolo?) so there Julia v1.10 is used. This is possible because there are two Manifests.

@navidcy navidcy added testing 🧪 Tests get priority in case of emergency evacuation package 📦 Quite meta labels Oct 8, 2024
@Sbozzolo
Copy link
Member

Sbozzolo commented Oct 8, 2024

Note the the distributed CI still does not have Julia v1.11 (right @Sbozzolo?) so there Julia v1.10 is used. This is possible because there are two Manifests.

I installed julia 1.11 on the Caltech cluster, but we haven't made a module yet (but it's coming today or so)

@glwagner
Copy link
Member

glwagner commented Oct 9, 2024

This is possible because there are two Manifests.

There's only one manifest?

@ali-ramadhan
Copy link
Member

This is possible because there are two Manifests.

There's only one manifest?

I assume he's referring to this new feature of Julia 1.11: https://julialang.org/blog/2024/10/julia-1.11-highlights/#manifest_versioning

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 9, 2024

There's only one manifest?

The PR adds another Manifest specifically for v1.11 and keeps the older Manifest that works for v1.10. Or we can just have one Manifest (the one for v1.11) and drop the one for v1.10

@glwagner
Copy link
Member

glwagner commented Oct 9, 2024

keeps the older Manifest that works for v1.10

The Manifest was deleted in #3783

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 9, 2024

oh great! I missed that!
I wanted to delete it long now but I got resistance doing that before... Great! So I'll delete the new Manifest then as well!

@glwagner
Copy link
Member

glwagner commented Oct 9, 2024

oh great! I missed that! I wanted to delete it long now but I got resistance doing that before... Great! So I'll delete the new Manifest then as well!

Deleting it seemed to help increase the likelihood that CI passed. Although, it did not fully solve the problem (and note a few other changes were also made on #3783).

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 11, 2024

Noting that internal_tide.jl gives NaN with Julia v1.11 while all is OK with Julia v1.10; something with immersed boundaries....? I'm looking into it.

@simone-silvestri
Copy link
Collaborator

I think it's a plotting issue. We are filling up the immersed boundaries with NaN and, apparently, we cannot plot NaNs anymore? The error says:

ERROR: LoadError: On worker 2:
  | Looking up a non-finite or NaN value in a colormap is undefined.

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 11, 2024

I ran the script and the actual simulation NaN-ed.

@glwagner
Copy link
Member

I ran the script and the actual simulation NaN-ed.

That means Oceanangians isn't compatible with julia 1.11.

Do any other tests catch the issue? We can use this opportunity to add more tests.

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 11, 2024

I’m trying to make an mwe

@glwagner
Copy link
Member

glwagner commented Oct 30, 2024

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

There are regression tests, but not sure if they are with immersed boundary or not.

@simone-silvestri
Copy link
Collaborator

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

There are some tests but quite fragmented, not very exhaustive and that run only for 10 time steps, here for diffusion, here for advection and here for distributed vs serial.

There is scope to add more tests for the immersed boundary grid, especially for a partial cell bottom, which I believe is still not tested.
I can start adding some tests for it.

@glwagner
Copy link
Member

glwagner commented Nov 7, 2024

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

Good to add tests though I don't know why 100+ time steps matters specifically?

@navidcy
Copy link
Collaborator Author

navidcy commented Nov 7, 2024

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

Good to add tests though I don't know why 100+ time steps matters specifically?

For a regression test, 10 should be plenty.

@ali-ramadhan
Copy link
Member

I can confirm that Julia 1.11 is changing how the immersed boundary (and free surface?) works. Here's the difference (including halos) in the internal tide u field after 1 time step (difference is Julia 1.10 - Julia 1.11 with + = red). Color range is ±0.003 m/s so it's a pretty significant for just 1 time step.

Probably time to pull out Debugger.jl and step through a time step to find what causes the difference!

display

@ali-ramadhan
Copy link
Member

Well, Debugger.jl doesn't work in Julia 1.11 either lol.

X-Ref: JuliaDebug/Debugger.jl#361

@glwagner
Copy link
Member

glwagner commented Jan 7, 2025

That is weird and alarming.

@glwagner
Copy link
Member

glwagner commented Jan 7, 2025

@wsmoses @vchuravy do you have any guesses how we could possibly observe a numerical difference between julia 1.10 and 1.11? This does not seem to be a syntax issue but rather a numerical calculation issue. Unless there is a latent syntax issue that has changed functionality which I suppose is also possible...

@glwagner
Copy link
Member

glwagner commented Jan 7, 2025

@ali-ramadhan it might be worth testing the nonhydrostatic model and also problems without immersed boundaries to pinpoint the problem.

@wsmoses
Copy link
Collaborator

wsmoses commented Jan 7, 2025

The random number generator doesn’t guarantee the same results even with a seed across versions

@navidcy
Copy link
Collaborator Author

navidcy commented Jan 9, 2025

The random number generator doesn’t guarantee the same results even with a seed across versions

Good point! 👍🏼 Good to keep that in mind!

But the differences we see in the internal_tide.jl example shouldn't be due to random number generator.

@glwagner
Copy link
Member

glwagner commented Jan 9, 2025

One possibility is that syntax changed for something in a subtle way, so the code still runs but some function is being called incorrectly. Not sure what that could be though

@glwagner
Copy link
Member

glwagner commented Jan 9, 2025

Here a list of changes:

https://docs.julialang.org/en/v1/NEWS/#:~:text=New%20language%20features,-New%20Memory%20type&text=Most%20of%20the%20Array%20type,public%20are%20considered%20public%20API.

the change to zero stands out the most to me since that is called extensively; then again I don't think the described feature applies to us.

We should check if there are changes only on CPU, or on both CPU and GPU. @ali-ramadhan I'm assuming your test was on CPU.

@navidcy
Copy link
Collaborator Author

navidcy commented Jan 9, 2025

Good find. Is there a way to redefine/import zero(::AbstractArray) to be how it was in Julia v1.10 to check if problem goes away? That would give us a very good hint!

@glwagner
Copy link
Member

glwagner commented Jan 9, 2025

Good find. Is there a way to redefine/import zero(::AbstractArray) to be how it was in Julia v1.10 to check if problem goes away? That would give us a very good hint!

I don't think we use zero(::AbstractArray). I was more wondering if changes related to that update might have affected us (which are not written in the notes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package 📦 Quite meta testing 🧪 Tests get priority in case of emergency evacuation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants