Skip to content
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

WIP: build_function rewrite #764

Merged
merged 29 commits into from
Feb 12, 2021
Merged

WIP: build_function rewrite #764

merged 29 commits into from
Feb 12, 2021

Conversation

shashi
Copy link
Collaborator

@shashi shashi commented Feb 1, 2021

Based on JuliaSymbolics/SymbolicUtils.jl#183 (docs: https://juliasymbolics.github.io/SymbolicUtils.jl/codegen/)

The build_function.jl in test/ passes for me locally, but I suppose I may have deleted one too many keyword argument (and hence little feature) that something in the ecosystem breaks.

cc @ChrisRackauckas @YingboMa comments and tests are welcome!

The first argument to build_function can be any of the Code types in the Symutils PR, hence allowing arbitrary julia code! The only special case we handle here is arrays -- it is handled rather weirdly for legacy (and probably performance) reasons -- it returns 2 exprs rather than one: out of place and in-place.... Maybe we should make the user invoke build_function twice to do that?

@shashi
Copy link
Collaborator Author

shashi commented Feb 1, 2021

The way I want to go about completing this is take test cases or code using build_function in the wild and get it to work by adding back features they need.

@shashi
Copy link
Collaborator Author

shashi commented Feb 1, 2021

@isaacsas let's revisit BCR with this!

@dpad
Copy link
Contributor

dpad commented Feb 2, 2021

@shashi Thanks a lot for your work on this, which is relevant to some issues I had previously.

Could I please ask for you to also consider the tests/fix I had in #681 (which I originally wrote to fix #678)?
I'm not sure whether your changes will avoid that problem, or if I'll have to fix it again after this gets merged, so I'd really appreciate if you had a look :)
The short version of the issue was that I couldn't call build_function from my own package at precompile time (e.g. const X = something_that_calls_build_function()) because it would get put into ModelingToolkit's RuntimeGeneratedFunction cache, which disappears after the precompilation.

@shashi
Copy link
Collaborator Author

shashi commented Feb 3, 2021

@dpad thanks, I will try to do the changes in your PR over here. For a while now we have been interning the function itself into the expressions we evaluate -- this eliminates the need to try and qualify functions since they are not "name"s anymore. But we do need to take care of which compile cache RGF uses. Thanks!

@shashi
Copy link
Collaborator Author

shashi commented Feb 4, 2021

@dpad I moved the code over from there, and also tried defining $mod.$(gensym()) instead of an anonymous function when doing the RGF, please have a look, but it still does not help your tests pass...... Could you have a look at this if you get a chance?

@YingboMa YingboMa force-pushed the s/new-build_function branch from 08614d0 to 41130c8 Compare February 4, 2021 23:32
@dpad
Copy link
Contributor

dpad commented Feb 5, 2021

Hi @shashi , thanks for merging them in! I've had a brief look, I believe the issue is due to a change in RuntimeGeneratedFunctions, where it's now using @__MODULE__ for the cache tag, instead of the provided module (which it's using for a "context tag"). I'm not sure why it was changed to this, but I'll investigate and raise it with them if it is indeed the issue.

@dpad
Copy link
Contributor

dpad commented Feb 5, 2021

@shashi I've worked around the issue here and opened a PR to merge into this. Please see #769.

Work around to specify cache module when building RuntimeGeneratedFunctions
@dpad
Copy link
Contributor

dpad commented Feb 9, 2021

For reference, once SciML/RuntimeGeneratedFunctions.jl#26 is merged in, we should be able to clean up the workaround from #769 to just:

function _build_and_inject_function(mod::Module, ex)
    ...
    # Ensure the function is cached and looked-up in the specified module.
    RuntimeGeneratedFunction(mod, mod, ex)
end

[edit: this will now work with [email protected]]

@shashi
Copy link
Collaborator Author

shashi commented Feb 9, 2021

Oh great! Thank you!!

@shashi
Copy link
Collaborator Author

shashi commented Feb 9, 2021

Can't really explain the CI fail. The branch passed for me locally.

@ChrisRackauckas
Copy link
Member

The RGF change was just tagged.

@shashi shashi merged commit b12c854 into master Feb 12, 2021
@shashi shashi deleted the s/new-build_function branch February 12, 2021 05:37
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.

4 participants