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

Remove LoopVectorization.jl #842

Merged
merged 2 commits into from
Dec 8, 2020
Merged

Remove LoopVectorization.jl #842

merged 2 commits into from
Dec 8, 2020

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Dec 1, 2020

Zygote depends on LoopVectorization only in order to add one gradient definition, for vmap. That doesn't seem like a good idea. Surely the rule can live elsewhere, via ZygoteRules or ChainRulesCore which exist for this purpose. Seems to have been added along with pmap in #728.

Marked as draft here until such a rule exists elsewhere. (It's possible that, after that, Zygote should keep the dep. & not load it, for a while, just all Pkg to bound versions.)

The rule might also be simplified, noted here #838 (comment), as I don't think vmap can guarantee that the elements will be acted on in order. Since pmap also can't guarantee to work in order, it also seems to have no need to reverse the order on the reverse pass.

@CarloLucibello
Copy link
Member

CarloLucibello commented Dec 1, 2020

having a second look at the code, I now see we have

_tryreverse(m, backs, Δ) = backs, Δ
function _tryreverse(m::typeof(map), backs, Δ::Union{AbstractVector, Tuple})
  return reverse(backs), reverse(Δ)
end
_tryreverse(m, x) = x
_tryreverse(m::typeof(map), x::Union{AbstractVector, Tuple}) = reverse(x)

so vmap and pmap are not affected

@CarloLucibello
Copy link
Member

Before doing this we should file a PR to LoopVectorization with the corresponding rule using ChainRulesCore.
We could also get rid of the pmap adjoint here and add it to ChainRules

@mcabbott
Copy link
Member Author

mcabbott commented Dec 8, 2020

I think this is ready to be merged, with the plan being to make a breaking release with this and #824. These need not wait for the corresponding LoopVectorization / NNlib releases.

@CarloLucibello CarloLucibello added this to the v0.6 milestone Dec 8, 2020
@CarloLucibello
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Dec 8, 2020
842: Remove LoopVectorization.jl r=CarloLucibello a=mcabbott

Zygote depends on LoopVectorization only in order to add one gradient definition, for `vmap`. That doesn't seem like a good idea. Surely the rule can live elsewhere, via ZygoteRules ~~or ChainRulesCore~~ which exist for this purpose. Seems to have been added along with `pmap` in #728. 

Marked as draft here until such a rule exists elsewhere. (It's possible that, after that, Zygote should keep the dep. & not load it, for a while, just all Pkg to bound versions.)

The rule might also be simplified, noted here #838 (comment), as I don't think `vmap` can guarantee that the elements will be acted on in order. ~~Since `pmap` also can't guarantee to work in order, it also seems to have no need to reverse the order on the reverse pass.~~

Co-authored-by: Michael Abbott <me@escbook>
@bors
Copy link
Contributor

bors bot commented Dec 8, 2020

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

@willtebbutt
Copy link
Member

I'm a little confused as to why this is breaking -- it doesn't appear that the user will see any changes. Am I missing something?

@mcabbott
Copy link
Member Author

mcabbott commented Dec 8, 2020

If anyone was using this gradient, and it no longer exists, that will break things right?

And if they were locked to LoopVectorization 0.8, Zygote 0.5, then they will stay broken, as the rule's new home will only be in LoopVectorization 0.9.x.

@willtebbutt
Copy link
Member

willtebbutt commented Dec 8, 2020

And if they were locked to LoopVectorization 0.8, Zygote 0.5, then they will stay broken, as the rule's new home will only be in LoopVectorization 0.9.x.

I see. So the argument is essentially that changing a lower-bound on a dependency corresponds to a breaking change?

@mcabbott
Copy link
Member Author

mcabbott commented Dec 8, 2020

No, we are dropping a dependency, so can no longer bound it at all.

But if someone was using it, their code will break.

@willtebbutt
Copy link
Member

willtebbutt commented Dec 8, 2020

Ahh understood. Would it not be possible to avoid the breaking release by using Requires and some static versioning stuff to avoid a breaking release? e.g. what we do with Distances.jl and StatsFuns.jl?

edit: or maybe there's not a way to get at what version of a package is loaded?

@simeonschaub
Copy link
Member

One thing I proposed before was manually adding an upper bound on LoopVectorization/NNlib for all previous Zygote versions in the registry.

@mcabbott
Copy link
Member Author

mcabbott commented Dec 8, 2020

Would it not be possible to avoid the breaking release by using Requires ... e.g. what we do with Distances.jl

Other plans are possible, we could add gradients for many packages here via Requires. That wouldn't be a breaking change here I think, although it would mean this package could no longer bound versions of the others, so it might be fragile to future changes.

But I thought the plan was to go the other way, ideally let many packages contain their own gradients via ChainRulesCore. Maybe #835 is meant to track this. JuliaMath/SpecialFunctions.jl#238 is another step.

manually adding an upper bound on LoopVectorization/NNlib for all previous Zygote versions

Don't you mean a bound on Zygote's version, applied retrospectively to all versions of LoopVectorization/NNlib? It's important that old NNlib never loads new Zygote.

@willtebbutt
Copy link
Member

But I thought the plan was to go the other way, ideally let many packages contain their own gradients via ChainRulesCore. Maybe #835 is meant to track this. JuliaMath/SpecialFunctions.jl#238 is another step.

Yes -- I'm very much on board with this plan, and am glad to see this kind of moving-stuff-out-of-Zygote work going on. My motivation for asking was to see if it's possible to avoid making a breaking change for a bit longer, since they tend to be quite disruptive. I'm completely on board with removing this stuff from this package (similarly for Distances and StatsFuns, when / if we can get the corresponding package authors on board), but IMHO the longer that we can avoid making a breaking release the better.

I can't think of an obvious solution though, and it looks like there's been quite a bit of thought put into this, so I now agree that this probably has to be breaking.

@mcabbott
Copy link
Member Author

mcabbott commented Dec 8, 2020

OK, I agree it would be ideal to gang more of them together. I'm lazy to make special temporary versions with Requires just to solve this, though.

We could look into killing the Distances & StatsFuns deps quickly too, I guess? If using ZygoteRules it would be just copy-paste, and then later it could be upgraded without breaking anything.

@willtebbutt
Copy link
Member

willtebbutt commented Dec 8, 2020

We could look into killing the Distances & StatsFuns deps quickly too, I guess? If using ZygoteRules it would be just copy-paste, and then later it could be upgraded without breaking anything.

I agree that's what we would ideally be doing, but maintainers are still wary of having ChainRulesCore as a dependency. See e.g. this issue

@mcabbott
Copy link
Member Author

mcabbott commented Dec 8, 2020

OK, I hadn't see that. Then it doesn't look imminent.

Another step to consider would be moving the code loaded via Requires from here to ChainRules. Maybe they won't be too keen either but better there than duplicated in each AD package depending on ChainRules, eventually, right? And that move sideways can be done without breaking, as Zygote depends directly on ChainRules.

@willtebbutt
Copy link
Member

Good point. I have mixed feelings about that, but can certainly see the benefit. @oxinabox what are your thoughts on the above?

@bors
Copy link
Contributor

bors bot commented Dec 8, 2020

Build succeeded:

@bors bors bot merged commit bb4ea42 into FluxML:master Dec 8, 2020
@mcabbott mcabbott deleted the nolv branch December 8, 2020 19:08
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.

4 participants