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

Fix type escaping in @import_frule, @import_rrule #1446

Merged
merged 6 commits into from
May 16, 2024

Conversation

mofeing
Copy link
Contributor

@mofeing mofeing commented May 14, 2024

While trying to import some ChainRules from a package, I got the following error:

julia> using Enzyme, OMEinsum

julia> Enzyme.@import_rrule(typeof(einsum), OMEinsum.EinCode, Any, Any)
UndefVarError: `OMEinsum` not defined

Stacktrace:
 [1] macro expansion
   @ ~/.julia/packages/Enzyme/srACB/ext/EnzymeChainRulesCoreExt.jl:181 [inlined]
 [2] top-level scope
   @ ~/Developer/k-local-gradient-descent/notebooks/enzyme-omeinsum-from-chainrules.ipynb:1

By expanding the macro, I saw the the type annotations where not correctly escaped. For example, for augmented_primal we get

... augmented_primal(...) where {... var"#378#AN_1" <: Enzyme.Annotation{<:(Enzyme.OMEinsum).EinCode}, var"#379#AN_2" <: Enzyme.Annotation{<:Enzyme.Any}, var"#380#AN_3" <: Enzyme.Annotation{<:Enzyme.Any}}

Check out that Enzyme.OMEinsum and Enzyme.Any are wrong.

I haven't checked it out but this PR should fix it.

@wsmoses
Copy link
Member

wsmoses commented May 14, 2024 via email

@mofeing
Copy link
Contributor Author

mofeing commented May 14, 2024

Added! It tests against a MockType in a MockModule.

@wsmoses
Copy link
Member

wsmoses commented May 14, 2024

@mofeing this fails CI

@mofeing
Copy link
Contributor Author

mofeing commented May 14, 2024

Forgot to add methods for fdiff and rdiff. Should be fixed now.

@mofeing
Copy link
Contributor Author

mofeing commented May 15, 2024

I fixed all errors in the tests except one: The rrule test of MockType.

The pullback in rrule defined for mock_function should return a number

function ChainRulesCore.rrule(::typeof(MockModule.mock_function), x)
    y = MockModule.mock_function(x)
    return y, ȳ -> 2 * ȳ
end

But it seems like Enzyme is returning a MockType. Is this because we are annotating the return activity to be Active?

rdiff(f, x::MockModule.MockType) = autodiff(Reverse, f, Active, Active(x))[1][1]

Enzyme.@import_rrule typeof(MockModule.mock_function) MockModule.MockType
@test rdiff(MockModule.mock_function, MockModule.MockType(1f0)) === 2f0

...

import_rrule: Test Failed at /Users/mofeing/Developer/Enzyme.jl/test/ext/chainrulescore.jl:117
  Expression: rdiff(MockModule.mock_function, MockModule.MockType(1.0f0)) === 2.0f0
   Evaluated: Main.MockModule.MockType(2.0f0) === 2.0f0

@wsmoses
Copy link
Member

wsmoses commented May 16, 2024

@mofeing CI still fails "import_rrule: Test Failed at /home/runner/work/Enzyme.jl/Enzyme.jl/test/ext/chainrulescore.jl:117
Expression: rdiff(MockModule.mock_function, MockModule.MockType(1.0f0)) === 2.0f0
Evaluated: Main.MockModule.MockType(2.0f0) === 2.0f0
"

@wsmoses
Copy link
Member

wsmoses commented May 16, 2024

and no, for active vals, Enzyme returns a value of the same type (whatever it is)

@mofeing
Copy link
Contributor Author

mofeing commented May 16, 2024

Okay, fixed now.

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.43%. Comparing base (cc8ceb6) to head (8130415).
Report is 2 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1446      +/-   ##
==========================================
+ Coverage   68.04%   72.43%   +4.39%     
==========================================
  Files          30       30              
  Lines       10772    10838      +66     
==========================================
+ Hits         7330     7851     +521     
+ Misses       3442     2987     -455     

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

@wsmoses wsmoses merged commit d582308 into EnzymeAD:main May 16, 2024
39 of 46 checks passed
@mofeing mofeing deleted the patch-1 branch May 16, 2024 18:19
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.

None yet

3 participants