-
-
Notifications
You must be signed in to change notification settings - Fork 13
Hotfix: remove extra unthunk_tangent
methods
#31
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
Conversation
@ToucheSir could you please conclude this hotfix? This issue is affecting various packages I maintain, making it hard to release new versions in packages that depend on Zygote.jl |
There were two blockers to getting this merged:
|
As far as I understand it, the changes you introduced today broke downstream packages that were not broken before. So undoing these changes could be the safest hotfix. A new PR could then be started to reintroduce the changes with more coverage tests. That way people can continue their work downstream while ZygoteRules.jl gets fixed. |
Just to clarify, that is precisely what this PR does. The problem is not a lack of coverage, but that existing downstream coverage tests started failing. However, after much digging I've now determined that there were no downstream CI runs post Zygote 0.7 which would've shown the error we see here. CompatHelper is partly to blame because it doesn't run GH Actions CI by default. So I'm merging this as-is. Let me know if you still run into precompilation issues. |
I am still unable to precompile packages that depend on Zygote.jl: ] activate --temp
] add CoordRefSystems WARNING: Method definition unthunk_tangent(ChainRulesCore.AbstractThunk) in module ZygoteRules at /home/juliohm/.julia/packages/ZygoteRules/KF1B6/src/adjoint.jl:40 overwritten in module Zygote at /home/juliohm/.julia/packages/Zygote/59YyM/src/compiler/chainrules.jl:4.
│ ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation. |
Can you wait until after JuliaRegistries/General#123376 has been merged (+ some time for the usual package server propagation) and try? |
Sorry, I thought that the latest release from 3 hours ago already included the fix. Will wait and retry. Thanks. |
Oh sorry, I see where the confusion came from. That release was me getting TagBot to work again, hence the very recent timestamp which made it look like it was for this PR instead. I just tested your MWE of |
Thank you! I confirm the issue has been resolved in various downstream packages. |
Fixes #30.
PR Checklist
[ ] Tests are added[ ] Documentation, if applicable