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

[TTFX] Use Packages Extensions to drop Zygote and others when not used. #62

Closed
lrnv opened this issue Oct 28, 2023 · 4 comments
Closed

Comments

@lrnv
Copy link
Contributor

lrnv commented Oct 28, 2023

TaylorDiff is depending on a lot of stuff it should not, which tanks my TTFX.

As already noted in #52 (comment)_ , It would be very beneficial to leverage package preferences to avoid, say, having Zygote and ReverseDiff as dependencies. Formally, the composability with the AD systems is not a core functionality of the package and could be loaded ONLY when those AD systems are loaded.

@lrnv
Copy link
Contributor Author

lrnv commented Nov 8, 2023

Taking a second look at this, It appear to me that it is only the last two files derivatives.jl and chainrules.jl that are really problematic.

  1. In derivatives.jl, there is a dependency on the (old) SlicedMap.jl, which brings a lot of stuff with it. Since it is only used for three calls to their map functions, maybe you could reconsider and ditch it ? If you are worried about your perfs on Zygote stuff, then read point 2)

  2. In chainrules.jl, the code is there to support a usecase with Zygote and/or with other AD systems. This code should definitely go into one (or two?) package extensions files so that loading Zygote and Chainrules is not necessary if I do not use them. The special overloades from SlicedMap.jl that are there for Zygote perfs could also be part of this package extension.

Tell me what you think

@lrnv lrnv changed the title Leverage package preferences [TTFX] Use Packages Extensions to drop Zygote and others when not used. Nov 8, 2023
@zhujch1
Copy link
Collaborator

zhujch1 commented Nov 8, 2023

  1. The dependency SlicedMap.jl has been removed. We have introduced a more efficient implementation.

  2. We are loading Zygote.jl for solving some issues in it, we will consider integrating ZygoteRules.jl as a weak dependency in the future. We can't remove Chainrules.jl, because we need to use 'frule's.

@lrnv
Copy link
Contributor Author

lrnv commented Nov 8, 2023

For SliceMap: Thanks for removing it !

For Zygote : by weak-dep you mean package extension, right ?

@tansongchen
Copy link
Member

Zygote has been added as an extension in c11dd72

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

No branches or pull requests

3 participants