Skip to content

Conversation

@penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Aug 5, 2025

What?

This PR removes PrefixContext, and replaces it with an equivalent field on the Model struct.

Really?

I actually quite dislike this PR, and I don't think this is the correct direction to go in.

I think PrefixContext was easier to reason about because it allowed for a cleaner isolation of prefixing-related code. The absence of PrefixContext means that prefixes now get handled at different levels of the codebase, and is particularly messy when it comes to submodels.

But, well, the work's done, so I may as well put it up.

More details?

There are two main 'types' of changes in this PR:

(1) Compiler

Inside the compiler, instead of just vn (which is the lhs of tilde) we now have to handle both left_vn (which is the lhs of tilde) as well as vn (which is the result of applying any currently active prefixes).

In effect, because the prefixing no longer happens in the tilde-pipeline, prefixes have to be handled at a higher level. For example, previously you could do something like:

f(ctx::PrefixContext, vn::VarName) = f(childcontext(ctx), prefix(ctx.prefix, vn))
f(ctx::OtherContext, vn::VarName) = ...

Since PrefixContext no longer exists, you have to prefix the VarName in the compiler itself before passing that to the general method f(ctx, vn).

This strategy works for functions like getconditioned, hasconditioned, etc.

(2) tilde-pipeline

For tilde_assume!!, the above strategy is not enough. We also have to thread model.prefix through to tilde_assume!!.

The reason for this is quite subtle and has to do with submodels. Consider the following:

@model function f()
    x ~ Normal() # (2)
end
@model function g()
    return a ~ to_submodel(f()) # (1)
end

pg = prefix(g(), @varname(hello))

In this case, we want the sampled variable to be called hello.a.x. If we use automatic prefixing as is shown here, this is easy to do: inside tilde_assume!! of the top-level model (1), a will already have been prefixed with hello, so the left argument to tilde_assume!! will be hello.a. We can thus simply set this as the appropriate 'prefix' for the inner model.

The problem arises if we don't automatically prefix:

@model function f()
    x ~ Normal() # (2)
end
@model function g()
    return a ~ to_submodel(f(), false) # (1)
end

pg = prefix(g(), @varname(hello))

If we run this, everything is the same as before: when we get to (1), tilde_assume!! knows that the prefixed left-hand side is hello.a. However, because we don't want to prefix the submodel, the inner variable should only be called hello.x, and not hello.a.x.

This means that we have to store the prefix hello separately so that we can apply it to the submodel without the a as well.

Note that in the absence of submodels, this would be entirely unnecessary.

@penelopeysm penelopeysm changed the base branch from main to py/remove-samplingcontext August 5, 2025 16:28
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2025

Benchmark Report for Commit caa0c0f

Computer Information

Julia Version 1.11.7
Commit f2b3dbda30a (2025-09-08 12:10 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 4 × AMD EPYC 7763 64-Core Processor
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 4 virtual cores)

Benchmark Results


@codecov
Copy link

codecov bot commented Aug 5, 2025

Codecov Report

❌ Patch coverage is 89.79592% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (breaking@79150ba). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/prefix.jl 85.18% 4 Missing ⚠️
src/compiler.jl 88.88% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             breaking    #1011   +/-   ##
===========================================
  Coverage            ?   80.88%           
===========================================
  Files               ?       40           
  Lines               ?     3746           
  Branches            ?        0           
===========================================
  Hits                ?     3030           
  Misses              ?      716           
  Partials            ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2025

DynamicPPL.jl documentation for PR #1011 is available at:
https://TuringLang.github.io/DynamicPPL.jl/previews/PR1011/

@penelopeysm penelopeysm force-pushed the py/no-prefix-context branch 2 times, most recently from 180816d to 93ceec4 Compare August 6, 2025 00:08
@penelopeysm penelopeysm mentioned this pull request Aug 8, 2025
6 tasks
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch from 5278370 to 331279c Compare August 8, 2025 10:23
@penelopeysm penelopeysm force-pushed the py/no-prefix-context branch from 93ceec4 to c670ef0 Compare August 8, 2025 10:25
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch 3 times, most recently from 0bd4e02 to 8e3dcac Compare August 10, 2025 23:26
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch 2 times, most recently from 0b87d0d to 992569f Compare September 18, 2025 13:03
Base automatically changed from py/remove-samplingcontext to breaking September 24, 2025 15:48
@penelopeysm
Copy link
Member Author

holy merge conflicts

Base automatically changed from breaking to main October 21, 2025 17:06
@penelopeysm penelopeysm mentioned this pull request Oct 21, 2025
@penelopeysm
Copy link
Member Author

Agh, this is frustrating.

@penelopeysm penelopeysm force-pushed the py/no-prefix-context branch 2 times, most recently from 9723158 to bd7951c Compare November 4, 2025 17:32
@penelopeysm penelopeysm changed the base branch from main to breaking November 4, 2025 17:33
@penelopeysm penelopeysm marked this pull request as ready for review November 4, 2025 17:33
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