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

Remove global allowscalar from testing suite attempt 2 #4039

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

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Jan 12, 2025

Because it does not allow us to catch problems where kernels erroneously run on the CPU (for example #4036), see CliMA/ClimaOcean.jl#318 (comment)

Probably will require a bit of fiddling to add allowscalar granularly in tests

closes #3039

@navidcy
Copy link
Collaborator

navidcy commented Jan 12, 2025

I'm so much in favor!

I did try to do it one other time long ago and gave up... :(

I'll help out!

@navidcy navidcy added testing 🧪 Tests get priority in case of emergency evacuation GPU 👾 Where Oceananigans gets its powers from labels Jan 12, 2025
@navidcy navidcy self-requested a review January 14, 2025 20:26
@navidcy
Copy link
Collaborator

navidcy commented Jan 15, 2025

So the output writer was failing with allow scalar operations and I added a CUDA.@allowscalar at the Base.getindex(f::Field, inds...) method; see 1fe5fb6.

Is this bad idea?

cc @glwagner, @simone-silvestri

@simone-silvestri
Copy link
Collaborator Author

I think so, this would defeat the purpose of this PR because it would not allow to catch scalar indexing issues with fields

@navidcy
Copy link
Collaborator

navidcy commented Jan 15, 2025

I think so, this would defeat the purpose of this PR because it would not allow to catch scalar indexing issues with fields

OK. But we need to do something because fetch_output was hitting a wall

@navidcy
Copy link
Collaborator

navidcy commented Jan 19, 2025

@simone-silvestri
Copy link
Collaborator Author

looks like a bug in convert_output. We should be passing parent(field) to it, not a field, but from the error we are trying to

copyto!(dst::Array{}, scr::Field{})

@simone-silvestri
Copy link
Collaborator Author

What do we do with this: https://buildkite.com/clima/oceananigans/builds/20198#019476ab-1f43-478a-a915-a760bb94157e/31-1301 ?

The problem seems to arise when trying to write down a FunctionField. this is because we have

function fetch_output(field::AbstractField, model)
    compute_at!(field, time(model))
    return parent(field)
end

that returns the same FunctionField which then fails in

function convert_output(output::AbstractArray, writer)
    if architecture(output) isa GPU
        output_array = writer.array_type(undef, size(output)...)
        copyto!(output_array, output)
    else
        output_array = convert(writer.array_type, output)
    end

    return output_array
end

I have added a method that converts the function field to CPU before writing it down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GPU 👾 Where Oceananigans gets its powers from testing 🧪 Tests get priority in case of emergency evacuation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do we allow scalar indexing in all our tests?
2 participants