Skip to content

Add DI warning to AutoEnzyme #109

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add DI warning to AutoEnzyme #109

wants to merge 1 commit into from

Conversation

gdalle
Copy link
Collaborator

@gdalle gdalle commented May 31, 2025

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Calling Enzyme through DI can introduce errors or slowdowns compared to Enzyme's native API. However, end users are unlikely to check out either the DI docs or the Enzyme docs when investigating what happened, especially when derivatives are deep in the call stack.
On the other hand, end users usually provide the AutoEnzyme object to whatever package they are using. Thus, adding a warning there seems like a good way to raise awareness.

@wsmoses what do you think? You could also write a page for the Enzyme docs with examples of what can go wrong when going through DI, and we could link it from here and from the DI docs.

Related:

@@ -45,6 +45,11 @@ Struct used to select the [Enzyme.jl](https://github.com/EnzymeAD/Enzyme.jl) bac

Defined by [ADTypes.jl](https://github.com/SciML/ADTypes.jl).

!!! warning
`AutoEnzyme` can be used by [DifferentiationInterface.jl](https://github.com/JuliaDiff/DifferentiationInterface.jl) to access a restricted subset of Enzyme.jl functionality.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like the wrong place to have it, since sciml uses ADTypes (created the package), and calls Enzyme directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning is that high-level users often don't make the call to DI themselves, but they do provide an AutoEnzyme (or AutoForwardDiff, etc.) object to whatever package they are calling. That's why putting a warning in ADTypes makes sense to me, because it is the entry point that users actually control about derivative computation.

I also plan on making the corresponding Enzyme warning in the DI docs more prominent. And I will let you have the final word in any Enzyme-related discussion on Discourse, Slack and other platforms, instead of piling up about what DI can or can't do. As long as users are aware they can try both options, I'm good with that. Consider this an olive branch :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but this still feels like the wrong place for this to happen, since ADTypes often directly calls enzyme and doesn’t go through DI.

As mentioned in the other thread I’m working on trying to make the errors throw only if called within DI

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this would be a good addition for the many packages who do call Enzyme through DI, without hurting the others, but I'll leave it up to you. Feel free to revive this PR if you change your mind, I'm setting it as a draft for the time being.

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.

2 participants