Use weak dependencies if supported by devmotion · Pull Request #68 · JuliaDiff/AbstractDifferentiation.jl

devmotion · 2022-12-27T15:26:05Z

This PR replaces the use of Requires with weak dependencies on Julia >= 1.9 (https://pkgdocs.julialang.org/dev/creating-packages/#Weak-dependencies).

Currently the PR is breaking since it removes the AD alias and export. I couldn't get the Requires/weak dependencies submodules working with the AD alias on a first try (I tried different combinations of using AbstractDifferentiation: AD, using AbstractDifferentiation: AbstractDifferentiation, using AD: AD etc.). I'm sure there's some way to fix it but I did not spend more time on it since

my impression was it is not really needed anyways due to the "renaming with as" feature in Julia >= 1.6,
the problems seemed to indicate its not an intended/common use case (actually, the only where I've seen this pattern is AbstractDifferentiation - e.g., FiniteDifferences only mentions and uses the alias FDM in the docs but does not define it in the package itself)

~~Edit: Shouldn't be merged until the Registrator deployment of JuliaRegistries/Registrator.jl#397 is done.~~

Edit: A few additional changes (see below):

As suggested by @ChrisRackauckas in Use weak dependencies if supported #68 (comment), I also used the opportunity to simplify how/when Zygote is loaded and bumped the Julia compat to 1.6.
As mentioned in Use weak dependencies if supported #68 (comment), the macros made too strong assumptions about what is available in the current scope but are fixed now
When updating the julia compat to 1.6, I also replaced a few @assert length(x) == 1; x[1] with only(x)

codecov-commenter · 2022-12-27T15:37:46Z

Codecov Report

Patch coverage: 91.96% and project coverage change: +0.54 🎉

Comparison is base (b929457) 83.82% compared to head (9db3b75) 84.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   83.82%   84.36%   +0.54%     
==========================================
  Files           6        8       +2     
  Lines         476      467       -9     
==========================================
- Hits          399      394       -5     
+ Misses         77       73       -4

Impacted Files	Coverage Δ
src/AbstractDifferentiation.jl	`79.66% <64.00%> (+0.58%)`	⬆️
ext/AbstractDifferentiationChainRulesCoreExt.jl	`100.00% <100.00%> (ø)`
ext/AbstractDifferentiationFiniteDifferencesExt.jl	`100.00% <100.00%> (ø)`
ext/AbstractDifferentiationForwardDiffExt.jl	`100.00% <100.00%> (ø)`
ext/AbstractDifferentiationReverseDiffExt.jl	`100.00% <100.00%> (ø)`
ext/AbstractDifferentiationTrackerExt.jl	`100.00% <100.00%> (ø)`
ext/AbstractDifferentiationZygoteExt.jl	`100.00% <100.00%> (ø)`
src/backends.jl	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mohdibntarek · 2022-12-27T18:58:31Z

This is a great PR, thanks! I need to study this feature before I can review the PR. But if someone else approves the PR, I am happy to merge.

devmotion · 2022-12-27T20:30:04Z

Maybe in addition to the Pkg docs linked above, it could be interesting to check Kristoffer's PR to PGFPlotsX: KristofferC/PGFPlotsX.jl#306

ChrisRackauckas · 2023-02-16T13:53:30Z

Can you quasi-namespace the extensions? I.e. AbstractDifferentiationForwardDiffExt? Otherwise you'll get collisions.

ChrisRackauckas · 2023-02-16T13:55:06Z

src/AbstractDifferentiation.jl

-    @require FiniteDifferences = "26cc04aa-876d-5657-8c51-4c34ba976000" include("finitedifferences.jl")
-    @require Tracker = "9f7883ad-71c0-57eb-9f7f-b5c9e6d3789c" include("tracker.jl")
-    @require Zygote = "e88e6eb3-aa80-5325-afca-941959d7151f" begin
+    @static if !EXTENSIONS_SUPPORTED


can move this out of the init definition

I tried many different versions and this was the (only?) one that seemed to work. But I'll give it another shot.

https://github.com/SciML/RecursiveArrayTools.jl/blob/master/src/RecursiveArrayTools.jl#L45-L50 it works here?

Ah, I think the problem was that I had put using Requires in the same check (outside of ìnit) which does not work. Having to if statements seems to work though.

Actually, I just managed to reproduce the problem: tpapp/LogDensityProblemsAD.jl@e4194f8 failed with https://github.com/tpapp/LogDensityProblemsAD.jl/actions/runs/4226767173/jobs/7340561574

ChrisRackauckas · 2023-02-16T13:55:17Z

src/AbstractDifferentiation.jl

+        @require Tracker = "9f7883ad-71c0-57eb-9f7f-b5c9e6d3789c" include("../ext/TrackerExt.jl")
        @static if VERSION >= v"1.6"
-            ZygoteBackend() = ReverseRuleConfigBackend(Zygote.ZygoteRuleConfig())
+            @require Zygote = "e88e6eb3-aa80-5325-afca-941959d7151f" include("../ext/ZygoteExt.jl")


why is this Zygote one versioned off?

For simplicity, just bump to LTS?

devmotion · 2023-02-16T15:09:31Z

Can you quasi-namespace the extensions? I.e. AbstractDifferentiationForwardDiffExt? Otherwise you'll get collisions.

Sure, I've seen this discussion in other places and Kristoffer said that probably it would be reasonable even if the collision issues are fixed upstream. It's a bit hacky though IMO since just adding a prefix does not guarantee that there won't be any collisions - e.g., an extension such as ForwardDiffChainRulesStaticArraysExt could be both an extension of ForwardDiff with weak dependencies ChainRules and StaticArrays or an extension of ForwardDiffChainRules with weak dependency StaticArrays. In any case, I'll update the PR 🙂

…erentiation.jl into dw/weakdeps

devmotion · 2023-02-28T14:04:02Z

Well, well. Updating to import AbstractDifferentiation as AD in the extensions revealed that the macros for defining pullbacks etc. have to be fixed: They implicitly assume that AbstractDifferentiation is available in the current scope (under this name) and they can't deal with arguments without names etc. So it seems this PR will get quite big, and possibly one even wants to fix a few things on the master branch.

devmotion · 2023-03-01T22:09:14Z

To summarize the last commits:

As suggested by @ChrisRackauckas in Use weak dependencies if supported #68 (comment), I also used the opportunity to simplify how/when Zygote is loaded and bumped the Julia compat to 1.6.
As mentioned in Use weak dependencies if supported #68 (comment), the macros made too strong assumptions about what is available in the current scope but are fixed now
When updating the julia compat to 1.6, I also replaced a few @assert length(x) == 1; x[1] with only(x)

The PR is ready for a thorough review and updated with the current practices for defining extensions.

ChrisRackauckas · 2023-03-02T08:10:11Z

ext/AbstractDifferentiationChainRulesCoreExt.jl

+module AbstractDifferentiationChainRulesCoreExt
+
+import AbstractDifferentiation as AD
+using ChainRulesCore: ChainRulesCore


This is not being imported using the ..ChainRulesCore that's needed for Requires

It's not loaded with Requires.

ChrisRackauckas · 2023-03-02T08:11:50Z

src/AbstractDifferentiation.jl

+   using Requires: @require
+   include("../ext/AbstractDifferentiationChainRulesCoreExt.jl")


I see, because there's no backwards compat to do? It's a bit odd but fine I guess.

I followed the same pattern as in other packages: pre-Julia 1.9 ChainRulesCore is a proper dependency (no Requires involved) whereas in >= 1.9 it's an extension.

ChrisRackauckas

This looks correct.

devmotion added 3 commits December 27, 2022 16:07

Reorganize files

154769c

Use weak dependencies and remove AD alias

b659ca2

Bump version

87694f3

devmotion added 2 commits December 27, 2022 17:37

Load only on Julia < 1.4

db4af2d

Use Compat.eachcol

ec29a2d

devmotion marked this pull request as draft December 30, 2022 14:14

devmotion marked this pull request as ready for review January 29, 2023 01:33

devmotion mentioned this pull request Feb 16, 2023

Replace Requires.jl with conditional dependencies for Julia 1.9 #72

Closed

ChrisRackauckas reviewed Feb 16, 2023

View reviewed changes

devmotion added 3 commits February 16, 2023 16:55

Rename extensions

5403dd2

Move check out of __init__

a83e71c

Merge branch 'dw/weakdeps' of ssh://github.com/JuliaDiff/AbstractDiff…

6e6e307

…erentiation.jl into dw/weakdeps

ChrisRackauckas mentioned this pull request Feb 28, 2023

LoadError: UndefVarError: StaticArrays not defined #73

Closed

devmotion and others added 5 commits February 28, 2023 13:46

Move ChainRulesCore support in extension and drop Julia < 1.6

6fe27db

Simplify code using features of Julia >= 1.6

6dccd17

Merge branch 'master' into dw/weakdeps

7fb49be

Update CI

9e77312

Fix include statements

d42dbe6

devmotion added 2 commits February 28, 2023 15:20

More fixes

90fcbae

Additional fixes

8378e26

ToucheSir mentioned this pull request Mar 1, 2023

reset Zygote context cache after each call #70

Closed

Simplify Zygote extension

9db3b75

ChrisRackauckas reviewed Mar 2, 2023

View reviewed changes

ChrisRackauckas approved these changes Mar 2, 2023

View reviewed changes

ChrisRackauckas merged commit 19ce815 into master Mar 2, 2023

ChrisRackauckas deleted the dw/weakdeps branch March 2, 2023 08:12

		using Requires: @require
		include("../ext/AbstractDifferentiationChainRulesCoreExt.jl")

Conversation

devmotion commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Dec 27, 2022 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mohdibntarek commented Dec 27, 2022

Uh oh!

devmotion commented Dec 27, 2022

Uh oh!

ChrisRackauckas commented Feb 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmotion Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmotion commented Feb 16, 2023

Uh oh!

devmotion commented Feb 28, 2023

Uh oh!

devmotion commented Mar 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisRackauckas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

devmotion commented Dec 27, 2022 •

edited

Loading

codecov-commenter commented Dec 27, 2022 •

edited by codecov bot

Loading

devmotion Feb 20, 2023 •

edited

Loading