Fix bug where integral fails when Enzyme ext not loaded #160

mikeingold · 2024-12-28T03:49:30Z

I discovered a bug where a clean install of Meshes without Enzyme will still default to using AutoEnzyme but won’t have the appropriate method defined.

I’m currently trying to fix this by signaling whether the extension is loaded via multiple dispatch. I created a supports_autoenzyme(::Any) = false method and moved the rest into the extension itself. That way, without the extension loaded, all calls will return false. When the extension is loaded, the methods with more specific argument types will take over.

src/utils.jl

test/combinations.jl

github-actions · 2024-12-28T04:06:28Z

Benchmark Results

	main	`e22f40d`...	main/e22f40d4e13c34...
Differentials/Differential	0.203 ± 0.001 μs	0.202 ± 0.0011 μs	1
Differentials/Jacobian	0.166 ± 0.001 μs	0.167 ± 0.001 μs	0.995
Integrals/Segment/Scalar GaussKronrod	0.822 ± 0.0079 μs	0.84 ± 0.0076 μs	0.978
Integrals/Segment/Scalar GaussLegendre	1.61 ± 0.006 μs	1.64 ± 0.005 μs	0.985
Integrals/Segment/Scalar HAdaptiveCubature	1.15 ± 0.064 μs	1.16 ± 0.15 μs	0.984
Integrals/Segment/Vector GaussKronrod	3.01 ± 0.059 μs	2.99 ± 0.054 μs	1.01
Integrals/Segment/Vector GaussLegendre	17.2 ± 0.5 μs	17.3 ± 0.46 μs	0.995
Integrals/Segment/Vector HAdaptiveCubature	3.82 ± 0.084 μs	3.88 ± 0.08 μs	0.983
Integrals/Sphere/Scalar GaussKronrod	0.0726 ± 0.00029 ms	0.0721 ± 0.0005 ms	1.01
Integrals/Sphere/Scalar GaussLegendre	1.81 ± 0.0081 ms	1.81 ± 0.0085 ms	1
Integrals/Sphere/Scalar HAdaptiveCubature	0.047 ± 0.00011 ms	0.0471 ± 8e-05 ms	0.996
Integrals/Sphere/Vector GaussKronrod	0.108 ± 0.0012 ms	0.109 ± 0.0016 ms	0.991
Integrals/Sphere/Vector GaussLegendre	3.25 ± 0.054 ms	3.19 ± 0.057 ms	1.02
Integrals/Sphere/Vector HAdaptiveCubature	0.0983 ± 0.00088 ms	0.0972 ± 0.0011 ms	1.01
Rules/GaussLegendre	21.9 ± 0.6 μs	21.9 ± 0.69 μs	0.999
Specializations/Scalar GaussLegendre/BezierCurve	0.25 ± 0.0017 ms	0.252 ± 0.00031 ms	0.993
Specializations/Scalar GaussLegendre/Line	7 ± 0.058 μs	7 ± 0.046 μs	1
Specializations/Scalar GaussLegendre/Plane	0.739 ± 0.0022 ms	0.739 ± 0.0021 ms	1
Specializations/Scalar GaussLegendre/Ray	5.96 ± 0.042 μs	5.93 ± 0.045 μs	1
Specializations/Scalar GaussLegendre/Rope	0.0514 ± 0.00016 ms	0.0512 ± 0.0002 ms	1
Specializations/Scalar GaussLegendre/Tetrahedron	0.154 ± 0.0012 s	0.152 ± 0.0013 s	1.01
Specializations/Scalar GaussLegendre/Triangle	0.596 ± 0.0041 ms	0.655 ± 0.0095 ms	0.909
time_to_load	1.48 ± 0.0013 s	1.49 ± 0.012 s	0.995

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…n loaded

…to enzyme

mikeingold · 2024-12-28T17:56:35Z

@JoshuaLampert I'm a little puzzled that the current code here is failing these tests, but I'm not an expert in package extensions. In my mind, the catch-all line

MeshIntegrals.jl/src/utils.jl

Line 25 in 017ecfe

supports_autoenzyme(::Type{<:Any}) = false

should always be used when the Enzyme extension isn't loaded, then when loaded the new methods

MeshIntegrals.jl/ext/MeshIntegralsEnzymeExt.jl

Lines 21 to 25 in 017ecfe

    
           MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.Geometry}) = true 
        
           MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.BezierCurve}) = false 
        
           MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.CylinderSurface}) = false 
        
           MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.Cylinder}) = false 
        
           MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.ParametrizedCurve}) = false

should be loaded.

Am I missing something?

test/utils.jl

test/combinations.jl

JoshuaLampert · 2024-12-28T19:22:24Z

@JoshuaLampert I'm a little puzzled that the current code here is failing these tests, but I'm not an expert in package extensions. In my mind, the catch-all line

MeshIntegrals.jl/src/utils.jl

Line 25 in 017ecfe

supports_autoenzyme(::Type{<:Any}) = false

should always be used when the Enzyme extension isn't loaded, then when loaded the new methods

MeshIntegrals.jl/ext/MeshIntegralsEnzymeExt.jl

Lines 21 to 25 in 017ecfe

MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.Geometry}) = true

MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.BezierCurve}) = false

MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.CylinderSurface}) = false

MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.Cylinder}) = false

MeshIntegrals.supports_autoenzyme(::Type{<:Meshes.ParametrizedCurve}) = false

should be loaded.

Am I missing something?

In principle it works. I just tried:

julia> using Meshes, MeshIntegrals

julia> MeshIntegrals._default_diff_method(Meshes.Sphere, Float64)
FiniteDifference{Float64}(1.0e-6)

julia> import Enzyme

julia> MeshIntegrals._default_diff_method(Meshes.Sphere, Float64)
AutoEnzyme()

but the test doesn't seem to recognize that the extension is not loaded. Maybe if we load the extension in another @testitem, it is also loaded in another even without using Enzyme in the other?

mikeingold · 2024-12-28T19:49:58Z

Now that I'm seeing the latest CI results...

for the Enzyme-loaded case it does actually look like the code is behaving as intended but the test is checking for the wrong support state.
for the Enzyme-not-loaded case, I wonder if the TestItems.jl backend has Enzyme accessible due to the other @testsnippets even if it isn't explicitly "import"ed by the particular @testitem.

JoshuaLampert · 2024-12-28T19:52:07Z

Now that I'm seeing the latest CI results...

* for the Enzyme-loaded case it does actually look like the code is behaving as intended but the test is checking for the wrong support state.

* for the Enzyme-not-loaded case, I wonder if the TestItems.jl backend has Enzyme accessible due to the other `@testsnippet`s even if it isn't explicitly "import"ed by the particular `@testitem`.

I agree. The first issue should be resolved by the comment I made above. The second is trickier, I think.

…me support flag in testing

test/combinations.jl

JoshuaLampert · 2024-12-28T20:28:03Z

Maybe revert 47193a2? I liked the automatically generated SupportStatus.

mikeingold · 2024-12-28T20:48:48Z

Maybe revert 47193a2? I liked the automatically generated SupportStatus.

My concern with the prior arrangement is mostly that we're using the codebase itself as the source of truth against which we're attempting to verify behavior. So, if somehow supports_autoenzyme(::SomeGeometry) is producing an unintended result, then the behavior of the code changes, but the checks we're running on the code could also change and result in a false negative/pass.

JoshuaLampert · 2024-12-28T21:01:59Z

We could explicitly test supports_autoenzyme separately in utils.jl for the different geometries (I actually thought we are doing that already, but it looks like we only test for _default_diff_method, which relies on supports_autoenzyme). The reason I like the automatically generated SupportStatus is that it minimizes the number of places in the tests, where we check for the support status. Ideally, we would only need to change one false to true in the tests once we implement support for a geometry, which is currently not supported for AutoEnzyme. Also, I would argue that all the tests in combinations.jl are more or less to test integral and that the correct integral implementation is used. I think, all the infrastructure around that and that the differentiation methods are picked up correctly, should be tested in utils.jl.
Having said that this is mostly bikeshedding, and I'm fine either way 😄

mikeingold · 2024-12-28T21:56:03Z

I restored a supports_autoenzyme check because I do think there's value in ensuring that it's tested for all possible geometries.

I'm thinking more about the SupportStatus constructor, but my concern more concretely is that the previous form was equivalent to

supports.autoenzyme = MeshIntegrals.supports_autoenzyme(geometry)
@test MeshIntegrals.supports_autoenzyme(geometry) == supports.autoenzyme

Maybe we could, inside the constructor, use something like

autoenzyme = typeof(geometry) \notin Union{BezierCurve, ...}

codecov · 2024-12-29T01:28:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (67fc779) to head (e22f40d).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #160   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines          184       181    -3     
=========================================
- Hits           184       181    -3

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

mikeingold · 2024-12-29T02:11:36Z

I removed the problematic @testitem (Enzyme not loaded) and switched to a constructor like I mentioned.

JoshuaLampert

LGTM

mikeingold added 2 commits December 27, 2024 22:04

Add explicit finitedifference test, shorten line lengths

d4a7bde

supports_autoenzyme always false unless ext is loaded

ec03b1d

github-actions bot reviewed Dec 28, 2024

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated Show resolved Hide resolved

test/combinations.jl Outdated Show resolved Hide resolved

mikeingold and others added 2 commits December 27, 2024 23:32

Use explicit package name

11485d5

Apply suggestions from code review

607d1bc

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

mikeingold added the bug Something isn't working label Dec 28, 2024

mikeingold added 4 commits December 28, 2024 12:22

Tweak supports_autoenzme any method, comments, style

68d6744

Add tests without Enzyme ext loaded, and more comprehensive tests whe…

f86680d

…n loaded

Merge branch 'enzyme' of github.com:JuliaGeometry/MeshIntegrals.jl in…

774e0cc

…to enzyme

Code style

017ecfe

Update utils.jl

254c90d

JoshuaLampert reviewed Dec 28, 2024

View reviewed changes

test/utils.jl Outdated Show resolved Hide resolved

JoshuaLampert reviewed Dec 28, 2024

View reviewed changes

test/combinations.jl Outdated Show resolved Hide resolved

Update utils.jl

ef96c81

Update testitem name for consistency

858086e

Avoid circular definition/verification by requiring explicit autoenzy…

47193a2

…me support flag in testing

JoshuaLampert reviewed Dec 28, 2024

View reviewed changes

test/combinations.jl Outdated Show resolved Hide resolved

Drop newly-redundant enzyme support checks

eace4d8

Add a supports_autoenzyme check

6a1b6b5

mikeingold added 2 commits December 28, 2024 20:08

Use blacklist in SupportStatus to determine autoenzyme

f9bf1c6

Remove problematic testitem

ceca964

Remove leftover autoenzyme kwarg, code style tweaks

d7c4c06

Add a test

d8ac4c0

mikeingold marked this pull request as ready for review December 29, 2024 02:09

Update changelog

e22f40d

JoshuaLampert approved these changes Dec 29, 2024

View reviewed changes

mikeingold merged commit 2d8938d into main Dec 29, 2024
12 checks passed

mikeingold deleted the enzyme branch December 29, 2024 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug where integral fails when Enzyme ext not loaded #160

Fix bug where integral fails when Enzyme ext not loaded #160

mikeingold commented Dec 28, 2024 •

edited

Loading

github-actions bot commented Dec 28, 2024 •

edited

Loading

mikeingold commented Dec 28, 2024

JoshuaLampert commented Dec 28, 2024

mikeingold commented Dec 28, 2024

JoshuaLampert commented Dec 28, 2024

JoshuaLampert commented Dec 28, 2024

mikeingold commented Dec 28, 2024

JoshuaLampert commented Dec 28, 2024

mikeingold commented Dec 28, 2024

codecov bot commented Dec 29, 2024 •

edited

Loading

mikeingold commented Dec 29, 2024

JoshuaLampert left a comment

Fix bug where integral fails when Enzyme ext not loaded #160

Fix bug where integral fails when Enzyme ext not loaded #160

Conversation

mikeingold commented Dec 28, 2024 • edited Loading

github-actions bot commented Dec 28, 2024 • edited Loading

Benchmark Results

Benchmark Plots

mikeingold commented Dec 28, 2024

JoshuaLampert commented Dec 28, 2024

mikeingold commented Dec 28, 2024

JoshuaLampert commented Dec 28, 2024

JoshuaLampert commented Dec 28, 2024

mikeingold commented Dec 28, 2024

JoshuaLampert commented Dec 28, 2024

mikeingold commented Dec 28, 2024

codecov bot commented Dec 29, 2024 • edited Loading

Codecov Report

mikeingold commented Dec 29, 2024

JoshuaLampert left a comment

Choose a reason for hiding this comment

mikeingold commented Dec 28, 2024 •

edited

Loading

github-actions bot commented Dec 28, 2024 •

edited

Loading

codecov bot commented Dec 29, 2024 •

edited

Loading