Change same space different instance error to warning.

[jb-mackay]

Addresses #639

[simonbyrne]

Looks like we need to tweak the JET tests to make exclude this: look at function_filter in https://aviatesk.github.io/JET.jl/stable/optanalysis/#JET.OptAnalyzer

[simonbyrne]

Or you might just be able to stick a @nospecialized tag on the function, as they are excluded by default

[jb-mackay]

This doesn't seem to be a great solution, as each @test_opt that fails this needs to have this function filter added. Plus, other tests still fail.

I'm a little surprised that we are seeing this runtime dispatch detected: Base.CoreLogging.handle_message since handle_message uses @nospecialize. At least given my current understanding of runtime dispatch and @nospecialize...

@simonbyrne any tips?

[simonbyrne]

@aviatesk any idea what is causing the problem here?

[aviatesk]

I will take a look on this today or tomorrow!

[aviatesk]

That @nospecialize doesn't eliminate dynamic dispatch when logger is abstract as handle_message has multiple methods for different loggers.

I'd say that usage of function_filter is a solution until we make the logging module type stable or make some special casing on JET side, which I'm not sure is worthwhile as I guess logging is designed to be very dynamic for some good reasons.

[jb-mackay]

The issue seems to be with logging macros like @warn, @info, and @error. Here is a minimum working example:

using JET

g() = @warn "warning!"
@test_opt g() # fails

JET complains about runtime dispatch for handle_message.

Is there any other way to throw a warning? JET does fine with error, though not @error.

[aviatesk]

Is there any other way to throw a warning?

Use function_filter. Maybe you want something like:

macro test_climaopt(ex0...)
    return var"@test_opt"(__source__, __module__, :(function_filter=$clima_function_filter), ex0...)
end

JET does fine with error, though not @error.

On code path leading to error (or, throw internally), Julia compiler intentionally disables most optimizations for latency reasons (as error path in general is not really supposed to be performance sensitive), so JET doesn't anything on it neither. If you put @error instead, it is just a type unstable code and JET currently doesn't special case it because, e.g. it's certainly very bad for performance if you put it in a heavy loop.

[jb-mackay]

On code path leading to error (or, throw internally), Julia compiler intentionally disables most optimizations for latency reasons (as error path in general is not really supposed to be performance sensitive), so JET doesn't anything on it neither. If you put @error instead, it is just a type unstable code and JET currently doesn't special case it because, e.g. it's certainly very bad for performance if you put it in a heavy loop.

That makes a lot of sense!

Use function_filter. Maybe you want something like...

Thank you for taking a look at this, this macro seems like a good solution.

[jb-mackay]

@simonbyrne would we want to use @test_climaopt to replace all instances of test_opt?

[simonbyrne]

@simonbyrne would we want to use @test_climaopt to replace all instances of test_opt?

that might be the easiest option?

[jb-mackay]

@charleskawczynski this change to a warning creates additional allocations that are caught by your tests - what would you advise as the best way to proceed?

[charleskawczynski]

@charleskawczynski this change to a warning creates additional allocations that are caught by your tests - what would you advise as the best way to proceed?

We should find a way to not incur allocations. I'm not sure of the best way to do this. One option is to just disable it all-together (that would be type-stable). That said, it would be nice to continue having some sort of sanity check. Maybe we can define a debug_spaces() = false method, and only apply the warning when debug_spaces() = true? The only issue I see with this is that it'd be good for us to have tests that typical drivers we're using somehow apply this test.

A long term solution might be: generate a hash from the grid points and add this to the type parameter space, then this check can be applied at compile time.

[jb-mackay]

Thank you @charleskawczynski for working on this with me earlier. We are almost ready to go, I would like to add a test that checks that the error is thrown when we want it to be.

@simonbyrne let us know if you have any suggestions, but I think this is a good way to move forward.

[jb-mackay]

@LenkaNovak when this is merged, we will simply need to turn on the warning (and thereby turn off the error) via:

import ClimaCore.Fields
Fields.allow_mismatched_diagonalized_spaces() = true

This changes the error to a warning when the spaces are the same but different instances. I think we can simply put this in the ClimaCoupler.jl module/file so that this is automatically enabled when the Coupler is imported. We will also probably want to turn off warning logs so that the warning is not printed a million times during a coupled simulation:

import Logging
Logging.disable_logging(Logging.Warn)

Not sure if we can target this specific warning to be filtered out.

[LenkaNovak]

Thanks so much for adding this, Ben! That's really helpful! And yeah, def want to avoid printing out lots of warning messages. If it's just that one, would it be possible to just do an if statement for now?

[LenkaNovak]

And are the stack allocations still a problem, with the same instance warning turned off?

[jb-mackay]

@LenkaNovak

Thanks so much for adding this, Ben! That's really helpful! And yeah, def want to avoid printing out lots of warning messages. If it's just that one, would it be possible to just do an if statement for now?

I don't think an if would work here. I really don't know if there is a good way to target specific warnings.

And are the stack allocations still a problem, with the same instance warning turned off?

With the warning turned off (and error turned on), the allocations are not an issue, due to the way Charlie rewrote this.

[simonbyrne]

should we still check if they are == equal?

[charleskawczynski]

This is inside a space1 !== space2 branch?

[simonbyrne]

!== is not ===. We should check if they are ==.

[charleskawczynski]

Ah, right, agreed.

[jb-mackay]

Can you explain this further @simonbyrne or @charleskawczynski, given that the if space1 !== space2 has not been changed by this PR? I thought we wanted this error to check that memory was not being used wastefully, so === for identical memory address makes sense.

[charleskawczynski]

Hmm, good question, what do we want == to check exactly?

[simonbyrne]

So there are 3 possible cases we should consider:

• spaces are ===: we're good
• spaces are not ===, but are == (i.e. they are both constructed from the same way). In this case, we should warn, but let it go
• spaces are not ==: should be an error.

The extra question is how to define ==: in this case, the easiest is probably to just check if the local_geometry fields are ==?

[jb-mackay]

• spaces are ===: we're good

• spaces are not ===, but are == (i.e. they are both constructed from the same way). In this case, we should warn, but let it go

• spaces are not ==: should be an error.

The extra question is how to define ==: in this case, the easiest is probably to just check if the local_geometry fields are ==?

Right, currently our "==" is is_diagonalized_spaces:

is_diagonalized_spaces(::Type{S}, ::Type{S}) where {S <: AbstractSpace} = true

is_diagonalized_spaces(::Type, ::Type) = false

Is it insufficient to check at this type level? Or overkill?

[jb-mackay]

@simonbyrne I think we are ready, feel free to merge if you are happy or suggest any further changes.

[jb-mackay]

bors r+

[bors]

Build succeeded:

• buildkite/climacore-ci
• docbuild
• format
• lib-climacore-makie
• lib-climacore-plots
• lib-climacore-tempestremap
• lib-climacore-vtk
• test-os (macos-latest)
• test-os (ubuntu-latest)
• test-os (windows-latest)