-
Notifications
You must be signed in to change notification settings - Fork 10
Add @might_produce macro
#198
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
Conversation
|
Libtask.jl documentation for PR #198 is available at: |
In v0.40 (or earlier?), TracedModel was moved from essential to src/mcmc/particle_mcmc.jl.
CI failuresThe 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. |
8177399 to
73e7b68
Compare
| function $(Libtask).might_produce(::Type{<:Tuple{typeof($(esc(f))),Vararg}}) | ||
| return true |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, thanks :)
There was a problem hiding this comment.
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 😅
mhauru
left a comment
There was a problem hiding this 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.
| function $(Libtask).might_produce(::Type{<:Tuple{typeof($(esc(f))),Vararg}}) | ||
| return true |
There was a problem hiding this comment.
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.
This PR introduces a macro to generate a superset of the
Libtask.might_producemethods needed for a functionfthat might contain aproducecall. For example, for a functioninstead of writing
you can just write
This works for functions with multiple methods and keyword arguments.
See #197 for the original motivation.