Skip to content

Conversation

@jrevels
Copy link
Member

@jrevels jrevels commented Sep 23, 2015

I get this when running using ForwardDiff:

julia> using ForwardDiff
INFO: Recompiling stale cache file /Users/jarrettrevels/.julia/lib/v0.5/ForwardDiff.ji for module ForwardDiff.
WARNING: eval from module Calculus to ForwardDiff:
Expr(:call, :/, 1, 2)::Any
  ** incremental compilation may be broken for this module **

WARNING: eval from module Calculus to ForwardDiff:
Expr(:call, :/, 1, 3)::Any
  ** incremental compilation may be broken for this module **

WARNING: eval from module Calculus to ForwardDiff:
Expr(:call, :log, 10)::Any
  ** incremental compilation may be broken for this module **

WARNING: eval from module Calculus to ForwardDiff:
Expr(:call, :log, 2)::Any
  ** incremental compilation may be broken for this module **

WARNING: eval from module Calculus to ForwardDiff:
Expr(:call, :log, 2)::Any
  ** incremental compilation may be broken for this module **
...

With a lot more of the same post-ellipses. Is this a side effect ForwardDiff's heavy reliance on code generation? FWIW, I already pulled JuliaMath/Calculus.jl#76 before running this, so it shouldn't have anything to do with Calculus precompilation AFAIK (that PR is what reminded me I should probably do this).

@KristofferC
Copy link
Collaborator

This is what the docs say

"Several additional restrictions are placed on the operations that can be done while precompiling code to help the user avoid other wrong-behavior situations:

  1. Calling eval to cause a side-effect in another module. This will also cause a warning to be emitted when the incremental precompile flag is set."

@mlubin
Copy link
Contributor

mlubin commented Sep 23, 2015

What is ForwardDiff doing to cause a side-effect in another module?

@jrevels
Copy link
Member Author

jrevels commented Sep 23, 2015

Calling eval to cause a side-effect in another module.

What is ForwardDiff doing to cause a side-effect in another module?

AFAIK, it's not...the only thing ForwardDiff uses Calculus for is

  1. calling Calculus.differentiate
  2. calling Calculus.symbolic_derivatives_1arg

Maybe calling Calculus.differentiate calls @eval in Calculus's scope, and that's triggering the warning?

@jrevels
Copy link
Member Author

jrevels commented Sep 23, 2015

At the very least, it does seem like a macro is used inside of Calculus.differentiate; I bet that's why this is happening.

If that's true, the real question is then: should we ignore these warnings? They only appear for the initial compilation, and tests are passing.

@mlubin
Copy link
Contributor

mlubin commented Sep 23, 2015

The @sexpr macro doesn't call eval anywhere. I don't think that explains it

@jrevels
Copy link
Member Author

jrevels commented Sep 23, 2015

Ah, I misread where that expression interpolation in Calculus.differentiate was actually occurring; I thought it was in function scope but it looks like it's in the scope of the encompassing @eval block.

@jrevels
Copy link
Member Author

jrevels commented Sep 23, 2015

The warning is

WARNING: eval from module Calculus to ForwardDiff:

Maybe it's not ForwardDiff causing a side-effect in Calculus, but the reverse, i.e. I'm using Calculus methods to generate the expressions that define ForwardDiff methods? That doesn't really make sense to me, but just throwing the idea out there...the thing that confuses me about this is that none of the Calculus methods called in ForwardDiff are called in an @eval block...

@mlubin
Copy link
Contributor

mlubin commented Sep 23, 2015

Are we extending any methods exported from Calculus?

@jrevels
Copy link
Member Author

jrevels commented Sep 23, 2015

Not that I know of. We don't do it explicitly, and we use import instead of using.

crosses fingers that this isn't related to exported Calculus.gradient

@mlubin
Copy link
Contributor

mlubin commented Sep 24, 2015

Maybe @vtjnash has some insight?

@vtjnash
Copy link

vtjnash commented Sep 24, 2015

The warning is (correctly) telling you that the eval at https://github.com/johnmyleswhite/Calculus.jl/blob/ae9ab47c48af0b872f16f8c3a2b7f2824401346f/src/symbolic.jl#L96 is probably invalid (although it happens that it is still safe for precompilation). The problem being that the eval occurs in the scope of Calculus, instead of whatever (unspecified) module it was supposed to be evaluated relative-to.

@timholy
Copy link
Contributor

timholy commented Sep 26, 2015

Maybe change that line to eval(current_module(), ex)?

@vtjnash
Copy link

vtjnash commented Sep 27, 2015

Yes, that would hide the warning, although it may not be what the author intended for this method to do. Another way to avoid the warning is to write this as:

getfield(TheModule, ex.args[1])(ex.args[2:end]...)

@mlubin
Copy link
Contributor

mlubin commented Sep 27, 2015

The expression inside eval here doesn't refer to any symbols or scope, it's just a algebraic expression like 1+1.

@jrevels
Copy link
Member Author

jrevels commented Sep 29, 2015

I'm no longer get warnings here as of JuliaMath/Calculus.jl#77.

Thanks for your help, everyone!

jrevels added a commit that referenced this pull request Sep 29, 2015
Turn on precompilation for ForwardDiff
@jrevels jrevels merged commit 5efb62e into master Sep 29, 2015
@jrevels jrevels deleted the precomp branch September 29, 2015 18:37
@vtjnash
Copy link

vtjnash commented Sep 29, 2015

@mlubin + is a symbol and has scope, or you wouldn't have needed eval. usually the language semantics manage it for you (for example, functions contain a reference to the module that created it). however, current_module() is probably the wrong scope, since it will cause confusing and unexpected behavior when using it in combination with other packages (and managing to trick the current heuristic does not make it correct, just that the heuristic was not designed to catch this particular situation).

@mlubin
Copy link
Contributor

mlubin commented Sep 29, 2015

@vtjnash is the answer to eval into Base, or is there a cleaner way?

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.

6 participants