Skip to content

Conversation

@penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Aug 19, 2025

This PR introduces a macro to generate a superset of the Libtask.might_produce methods needed for a function f that might contain a produce call. For example, for a function

function f(x::Int, y::Int)
    produce(x + y)
end

instead of writing

Libtask.might_produce(::Type{<:Tuple{typeof(f),Int,Int}}) = true

you can just write

Libtask.@might_produce(f)

This works for functions with multiple methods and keyword arguments.

See #197 for the original motivation.

@penelopeysm penelopeysm requested a review from mhauru August 19, 2025 11:48
@github-actions
Copy link

Libtask.jl documentation for PR #198 is available at:
https://TuringLang.github.io/Libtask.jl/previews/PR198/

@penelopeysm penelopeysm changed the title Add @might_produce_kwargs macro Add @might_produce macro Aug 19, 2025
In v0.40 (or earlier?), TracedModel was moved from essential to
src/mcmc/particle_mcmc.jl.
@penelopeysm
Copy link
Member Author

penelopeysm commented Aug 19, 2025

CI failures

The AdvancedPS "integration test" is failing, because AdvancedPS CI itself is failing, because TuringLang/SSMProblems.jl#108

The benchmark test is failing because this is the first time it's ever been run with the new version of Turing that supports it. A lot of the code in there is not even updated for the current version of Libtask. It needs to be updated, but I'd rather not do it in this PR.

The rest are Julia 1.12.

Comment on lines +394 to +395
function $(Libtask).might_produce(::Type{<:Tuple{typeof($(esc(f))),Vararg}})
return true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is a little bit of a sledgehammer: we're basically saying, 'any invocation of f with any positional arguments might produce'. This is not necessarily true because some methods of f might produce and some might not.

But since there isn't any real downside to marking all methods are produceable, I don't think this is a huge issue. And if someone wants to be surgical, they can still use the non-macro version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since there isn't any real downside to marking all methods are produceable, I don't think this is a huge issue.

I would guess that there's a performance downside. Maybe for both compile-time and for run-time.

I was just thinking of proposing a comment in the docstring noting this aspect, and directing the user to might_produce if the function has many methods and they want to be performance-optimal.

Also, in the future we could make a version where you could restrict the types, like @might_produce(f(::Int,::Any)). Not in this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess that there's a performance downside. Maybe for both compile-time and for run-time.

Not super important now, but would be curious to hear more about this, since I never looked closely at how it actually works.

a comment in the docstring

Will do

Also, in the future we could make a version where you could restrict the types

Interestingly enough parsing a function signature in a macro is exactly what I did here so this actually feels a bit familiar 😄 agree it can be a separate thing though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super important now, but would be curious to hear more about this, since I never looked closely at how it actually works.

I think if might_produce returns false for some method/function f, the Libtask-transformed function just calls f. Whereas if might_produce returns true, I think we recurse into transforming f, which means doing all the slow reading/writing of all intermediate variables within f as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, thanks :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make the warning a bit more strident then because that actually sounds non-trivial 😅

Copy link
Member

@mhauru mhauru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! One documentation request, otherwise happy.

Comment on lines +394 to +395
function $(Libtask).might_produce(::Type{<:Tuple{typeof($(esc(f))),Vararg}})
return true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since there isn't any real downside to marking all methods are produceable, I don't think this is a huge issue.

I would guess that there's a performance downside. Maybe for both compile-time and for run-time.

I was just thinking of proposing a comment in the docstring noting this aspect, and directing the user to might_produce if the function has many methods and they want to be performance-optimal.

Also, in the future we could make a version where you could restrict the types, like @might_produce(f(::Int,::Any)). Not in this PR though.

@penelopeysm penelopeysm requested a review from mhauru October 5, 2025 13:09
@penelopeysm penelopeysm merged commit 0aa14c1 into main Oct 7, 2025
13 of 19 checks passed
@penelopeysm penelopeysm deleted the py/kwargs branch October 7, 2025 13:53
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.

3 participants