-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
fix Zygote on 1.6, fix #851 #848
Conversation
Ok, besides LoopVectorization, the remaining failures should be fixed by JuliaLang/julia#38764. @mcabbott What's the status of #842? With that merged, we would be pretty close to getting all tests to pass on 1.6. |
#842 is fine, I think, but it wants JuliaSIMD/LoopVectorization.jl#167. If the version of Zygote which has #842 is 0.6, then it need not wait --- anyone needing |
Ah, I see! We should coordinate that with #824 then, since that needs a breaking release as well. |
Note that this doesn't need to hold up a 0.6 release, the changes in this PR should not be breaking. |
7ee7954
to
0e9b890
Compare
OK, I was still missing some things, but I have now verified this on top of JuliaLang/julia#38764 and all tests pass! 🎉 |
Relies on ZygoteRules.jl#13. The test I marked as broken doesn't fail if run in an interpreter, so this might be a Julia bug. Other than that, I still see failures related to LoopVectorization, might be due to llvmcall. @chriselrod, do you have any ideas?
0e9b890
to
786d129
Compare
Once the compat issues here are fixed, you should also comment out the |
What's still holding this up is that FluxML/ZygoteRules.jl#13 hasn't been released yet, but I know @oxinabox had some doubts about the approach there, so maybe we don't want to rush this. |
Yeah, i think that it is confusing and inconsistent with all other uses of adjoint. |
My thought process was that someone might have defined an adjoint for |
According to JuliaHub code search the things to test/check are:
I think it should only matter if they were directly using |
OK, I updated this PR accordingly and tested that this still works. Since master is now at 0.6 anyways, I think this should be fine. |
98ec621
to
ca8cd24
Compare
@test_broken begin | ||
a3, pb3 = Zygote.pullback(h, 1) | ||
((1,),) == pb3(1) | ||
end |
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.
what is broken here?
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.
Really not sure. That's the one I mentioned in the description, which doesn't occur when run with JuliaInterpreter. I didn't have time yet to really look into it, but this might be a Julia bug.
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.
Nested AD is hard.
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.
Yeah. Diffractor.jl to the rescue! :)
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.
Not entirely.
But a but.
The code will be less gross and a bit faster.
But the problem is just legit hard.
what is left to do to make CI happy? |
Merging FluxML/ZygoteRules.jl#15 and tagging a new ZygoteRules release should make tests pass. I don't know whether all the CUDA fixes are released yet, but IIUC, those tests should only get run by bors. |
Ok, this should be good to go now. It wouldn't hurt to have another set of eyes look this over, but not sure who would be best to do this. |
Maybe @oxinabox or @willtebbutt could approve this? |
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.
Broadly LGTM, just a couple of queries I would like addressed before I approve.
|
||
@adjoint function literal_getproperty(x, ::Val{f}) where f | ||
val = getproperty(x, f) | ||
@adjoint function literal_getfield(x, ::Val{f}) where f |
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.
Might be cleaner to have separate adjoints for literal_getproperty and literal_getfield
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.
Could you explain what you mean here? We now don't define a default adjoint for literal_getproperty
, but instead fall back to recursing into getproperty
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.
LGTM barring on thing with the inconsistency.
Once that is fixed feel free to merge.
@oxinabox Did I address your comments? |
You did looks great |
This has regressed quite a bit due to FluxML#848. With this PR, we should be able to get back the same performance as before, assuming there is no custom implementation or pullback for `getproperty`. Still need to add tests.
This has regressed quite a bit due to FluxML#848. With this PR, we should be able to get back the same performance as before, assuming there is no custom implementation or pullback for `getproperty`. Still need to add tests.
909: improve inference for getproperty r=willtebbutt a=simeonschaub This has regressed quite a bit due to #848. With this PR, we should be able to get back the same performance as before, assuming there is no custom implementation or pullback for `getproperty`. Still need to add tests. Co-authored-by: Simeon David Schaub <schaub@mit.edu>
Relies on
FluxML/ZygoteRules.jl#13FluxML/ZygoteRules.jl#15. The test I marked as broken doesn't fail if run in an interpreter, so this might be a Julia bug.Other than that, I still see failures related to LoopVectorization, might be due to llvmcall. @chriselrod, do you have any ideas?fixes #851