TuringLang / DynamicPPL.jl

Implementation of domain-specific language (DSL) for dynamic probabilistic programming
https://turinglang.org/DynamicPPL.jl/
MIT License
157 stars 26 forks source link

Replace usage of `typeof` (inferred) by `Core.Typeof` (runtime) #627

Closed torfjelde closed 3 weeks ago

torfjelde commented 2 months ago

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, https://github.com/TuringLang/DynamicPPL.jl/issues/643

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 9749798418

Details


Files with Coverage Reduction New Missed Lines %
src/model.jl 1 88.35%
src/abstract_varinfo.jl 9 82.68%
src/threadsafe.jl 14 48.25%
<!-- Total: 24 -->
Totals Coverage Status
Change from base Build 9678167735: -0.9%
Covered Lines: 2642
Relevant Lines: 3448

💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 9749858036

Details


Totals Coverage Status
Change from base Build 9678167735: 0.0%
Covered Lines: 2657
Relevant Lines: 3427

💛 - Coveralls
torfjelde commented 2 months ago

Unfortately doesn't seem to address the issue in https://github.com/TuringLang/DynamicPPL.jl/issues/643 nor the failing one herehttps://github.com/torfjelde/EnzymeCon2024-Turing.jl-benchmarks/blob/main/src/models/fails/satellite-ssm-matrix.jl 😕

torfjelde commented 2 months ago

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 typeof and we were hypothesizing if that might have been a cause of an Enzyme issue; seems like it's not though 😕

torfjelde commented 2 months ago

Converted to DRAFT though because it wasn't my intention to have this be merged into master atm; just for Enzyme testing

wsmoses commented 2 months ago

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 https://github.com/TuringLang/DynamicPPL.jl/pull/627#issuecomment-2202471673, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJTUXFLXWBADHMK6IUWJ5TZKJVXLAVCNFSM6AAAAABKGEMTZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBSGQ3TCNRXGM . You are receiving this because you were mentioned.Message ID: @.***>

torfjelde commented 2 months ago

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

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:)

mhauru commented 2 months ago

I can take a look, may take may a few days to get to this.

yebai commented 3 weeks ago

Ref: https://github.com/TuringLang/DynamicPPL.jl/issues/643