-
-
Notifications
You must be signed in to change notification settings - Fork 15
Introduce AutoDI wrapper type for DifferentiationInterface dispatch #140
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: wsmoses <[email protected]>
Co-authored-by: wsmoses <[email protected]>
src/ADTypes.jl
Outdated
| include("mode.jl") | ||
| include("dense.jl") | ||
| include("sparse.jl") | ||
| include("di.jl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot move this inside of dense, similarly to AutoReactant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved AutoDI into dense.jl (commit 5180120). The separate di.jl file has been removed and all AutoDI definitions are now at the end of dense.jl, similar to how AutoReactant is structured.
src/ADTypes.jl
Outdated
| include("mode.jl") | ||
| include("dense.jl") | ||
| include("sparse.jl") | ||
| include("di.jl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot move this inside of dense, similarly to AutoReactant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already addressed in commit 5180120. The AutoDI code has been moved into dense.jl and the separate di.jl file has been removed.
Co-authored-by: wsmoses <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
cd("/home/REDACTED/work/ADTypes.jl/ADTypes.jl/test")
|
|
@gdalle @ChrisRackauckas any thoughts here? |
|
I'm not sure what the point is. Right now, the semantics of ADTypes objects for many packages out there is "call the backend package through DI". If you introduce |
|
Essentially I'd like to take a step back and ask what is best for the everyday users who already have code going through DI for
|
|
I mean the point is that AutoDI(AutoEnzyme()) and AutoEnzyme() do mean different things. See the Flux thread which I linked to here which cannot use DI from within Reactant, and thus cannot use DI as a fallback. However, if AutoEnzyme is overloaded to call into Enzyme directly it cannot call the DI implementation of Enzyme. So there needs to be something to distinguish the two. There doesn't need to be a breaking release. Downstream packages that leverage ADTypes, however, can retain bindings for both DI and AD Tools directly. The downstream packages can (as they do already) choose the semantics. |
So in the meantime, I would just add a fallback to DI unwrapping |
Yeah exactly |
|
I guess I could be okay with that, but I still would like to ask users on the Slack |
|
okay @gdalle from the slack thread it doesn't seem like there's any oppositition to the current design here (which seems to be the only one which adheres to the current adtypes semantics). Let me know if you have any final thoughts on this PR, and I'll go ahead and approve it |
test/runtests.jl
Outdated
| ReverseMode, | ||
| SymbolicMode | ||
| using ADTypes: dense_ad, | ||
| inner_ad, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix the merge conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 540c6d5. The merge conflict was due to formatting differences - the main branch switched from JuliaFormatter to Runic.jl formatting. Updated all files to match the new Runic style (4-space indentation, removed explicit returns, adjusted spacing).
…branch Co-authored-by: wsmoses <[email protected]>
| """ | ||
| AutoDI{I<:AbstractADType} | ||
|
|
||
| Wraps an AD type to signify that the DifferentiationInterface wrapper should be used instead of calling the backend directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this formulation, because it seems to suggest that anything not wrapped in AutoDI will not use DI. However we agreed that the current behavior of packages who call, say, AutoEnzyme through DI should be preserved. Maybe add a word of caution specifying that "using an ADType directly instead of wrapping it inside AutoDI does not forbid the use of DI"?
| end | ||
|
|
||
| """ | ||
| AutoDI{I<:AbstractADType} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call it AutoDifferentiationInterface? I know it is longer but it is more explicit, and coherent with other types in here. People can always define a shortcut if needed
| @test mode(di_ad) isa ForwardOrReverseMode | ||
| elseif mode(ad) isa ReverseMode | ||
| @test mode(di_ad) isa ReverseMode | ||
| elseif mode(ad) isa SymbolicMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is never hit
| end | ||
|
|
||
| @testset "Nested wrapping" begin | ||
| # Test that we can wrap AutoDI with AutoSparse and vice versa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to support arbitrary wrappings like that. Shouldn't there be a proper way to wrap and an improper one?
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
cd("/home/REDACTED/work/ADTypes.jl/ADTypes.jl/test")
|
AutoDIwrapper type following theAutoSparsepatternAutoDIstruct in src/dense.jl (moved from separate file)inner_adaccessor function (similar todense_ad)modefunction works withAutoDIAutoDIOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.