Replace usage of typeof (inferred) by Core.Typeof (runtime)
#627
Conversation
Pull Request Test Coverage Report for Build 9749798418Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9749858036Details
💛 - Coveralls |
|
Unfortately doesn't seem to address the issue in #643 nor the failing one herehttps://github.com/torfjelde/EnzymeCon2024-Turing.jl-benchmarks/blob/main/src/models/fails/satellite-ssm-matrix.jl 😕 |
| @@ -363,7 +363,8 @@ Determine the default `eltype` of the values returned by `vi[spl]`. | |||
| This method is considered legacy, and is likely to be deprecated in the future. | |||
| """ | |||
| function Base.eltype(vi::AbstractVarInfo, spl::Union{AbstractSampler,SampleFromPrior}) | |||
| T = Base.promote_op(getindex, typeof(vi), typeof(spl)) | |||
| # TODO(torfjelde): Is there _any_ scenario where this isn't just `typeof(getlogp(vi))`? | |||
There was a problem hiding this comment.
promote_op was introduced when fixing TuringLang/Turing.jl#2151. If getindex errors it will return Union{}, which - probably - is not equal to typeof(getlogp(vi)).
| @@ -363,7 +363,8 @@ Determine the default `eltype` of the values returned by `vi[spl]`. | |||
| This method is considered legacy, and is likely to be deprecated in the future. | |||
| """ | |||
| function Base.eltype(vi::AbstractVarInfo, spl::Union{AbstractSampler,SampleFromPrior}) | |||
| T = Base.promote_op(getindex, typeof(vi), typeof(spl)) | |||
| # TODO(torfjelde): Is there _any_ scenario where this isn't just `typeof(getlogp(vi))`? | |||
| T = Base.promote_op(getindex, Core.Typeof(vi), Core.Typeof(spl)) | |||
There was a problem hiding this comment.
It's not clear to me why this would be helpful or needed here - typeof is commonly used in this way to avoid where clauses (which is even recommended in the Julia docs). There also shouldn't be any problem to infer the result of typeof here?
| @@ -737,7 +737,7 @@ For a `context` that is _not_ a `SamplingContext`, we fall back to | |||
| `matchingvalue(SampleFromPrior(), vi, value)`. | |||
| """ | |||
| function matchingvalue(sampler, vi, value) | |||
| T = typeof(value) | |||
| T = Core.Typeof(value) | |||
There was a problem hiding this comment.
Same here, I don't see why this would be beneficial in this place? Maybe the problem is rather that value is not inferred in the first place?
There was a problem hiding this comment.
Indeed it might be the case that value is not inferred properly.
If I've understood @wsmoses correctly, Enzyme should be using the runtime value rather than the inferred value, and so this is the discrepancy between how we do it in DPPL: we use inferred instead of runtime. However, this didn't seem to help 😕
| @@ -217,7 +217,7 @@ function SimpleVarInfo(θ::Union{<:NamedTuple,<:AbstractDict}) | |||
| SimpleVarInfo{SIMPLEVARINFO_DEFAULT_ELTYPE}(θ) | |||
| else | |||
| # Infer from `values`. | |||
| SimpleVarInfo{float_type_with_fallback(infer_nested_eltype(typeof(θ)))}(θ) | |||
| SimpleVarInfo{float_type_with_fallback(infer_nested_eltype(Core.Typeof(θ)))}(θ) | |||
|
I think I'll have to defer to @wsmoses for your comments @devmotion . From convo with him, I sort of got the impression that we wanted to avoid |
|
Converted to DRAFT though because it wasn't my intention to have this be merged into master atm; just for Enzyme testing |
|
Having not looked at the code changes here yet. @devmotion the context here
is a program tor had which failed when compiled with GPUComojler with an
error along the lines of cannot construct matrix{any}. This was not from
any differentiation, but just GPUComojler calling the Julia compiler itself
(though perhaps with different flags for say inlining).
Our hypothesis was that somewhere this matrix (which wasn't supported by a
later function resulting in the guaranteed error), was being constructed by
the use of the inferred type of rather than runtime type.
The fact that it fails in this way implies the use of some undefined
behavior (eg the Julia compiler choosing to inline a function differently
causing its body to have an underspecializdd type vs more specialized).
I recommended Tor see if propagating the use of the actual runtime type
would resolve it here, but if not Tor you're going to have to see what
other use of undefined behavior results in the matrix any in your example
…On Tue, Jul 2, 2024, 10:16 AM Tor Erlend Fjelde ***@***.***> wrote:
Converted to DRAFT though because it wasn't my intention to have this be
merged into master atm; just for Enzyme testing
—
Reply to this email directly, view it on GitHub
<#627 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJTUXFLXWBADHMK6IUWJ5TZKJVXLAVCNFSM6AAAAABKGEMTZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBSGQ3TCNRXGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Noted! But I'm unfortunately short on time these days, so uncertain if I'll be the best one to have a go at this. Might be better for @mhauru to have a look at it? In particular given that you two are co-hacking atm:) |
|
I can take a look, may take may a few days to get to this. |
|
Ref: #643 |
This is per @wsmoses suggestion after a chat we (and @mhauru ) had earlier today.
It's an attempt to fix some issues we've had with Enzyme.jl-integration; in particular, #643