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

Allow broadcast errors to surface later. #491

Merged
merged 3 commits into from
Aug 30, 2023
Merged

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Aug 30, 2023

We are currently checking for isconcretetype, which is too limiting, as e.g. CUDA.jl supports isbits-union arrays. Checking for allocatedinline would be better, but let's just do away with the check entirely and have the array constructor fail in the presence of unsupported element types. This is better anyway, as some back-ends may not support isbits-unions.

We do however still check for Union{}, as that isn't allocated inline so would fail array construction. By using Nothing there instead, we give the GPU kernel (which is expected to throw an error) the chance to execute and report an exeception dynamically. This also makes it possible to trace a broadcast invocation under, e.g., Cthulhu.

We are currently checking for `isconcretetype`, which is too limiting, as
e.g. CUDA.jl supports isbits-union arrays. Checking for `allocatedinline`
would be better, but let's just do away with the check entirely and have
the array constructor fail in the presence of unsupported element types.
This is better anyway, as some back-ends may not support isbits-unions.

We do however still check for Union{}, as that isn't allocated inline
so would fail array construction. By using Nothing there instead, we give
the GPU kernel (which is expected to throw an error) the chance to execute
and report an exeception dynamically. This also makes it possible to
trace a broadcast invocation under, e.g., Cthulhu.
@maleadt maleadt enabled auto-merge (squash) August 30, 2023 14:58
@maleadt maleadt disabled auto-merge August 30, 2023 14:58
@maleadt maleadt merged commit 15969e9 into master Aug 30, 2023
13 checks passed
@maleadt maleadt deleted the tb/broadcast_errors branch August 30, 2023 14:58
@RainerHeintzmann
Copy link

Strange. This was merged into master, but I still get the same error:
ERROR: GPU broadcast resulted in non-concrete element type Union{Missing, Float64}. This probably means that the function you are broadcasting contains an error or type instability.

@maleadt
Copy link
Member Author

maleadt commented Aug 30, 2023

Then you're not using GPUArrays#master. Check st -m in the Pkg REPL.

@RainerHeintzmann
Copy link

... I see. The dependence of CUDA.jl has not yet changed. If I download and install GPUArrays.jl it works.

@maleadt
Copy link
Member Author

maleadt commented Aug 30, 2023

Yeah, that's normal. You don't automatically get commits from master branches until a version has been released, or unless you specifically request those changes. Would you expect otherwise?

@RainerHeintzmann
Copy link

;-) No. But then I did not really notice that this change was not a change to the master of CUDA.jl repo when I posted my first comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants