Skip to content

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

Merged
merged 2 commits into from
Jan 20, 2025
Merged

Conversation

ToucheSir
Copy link
Member

Fixes #30.

PR Checklist

  • [ ] Tests are added
  • [ ] Documentation, if applicable

@juliohm
Copy link

juliohm commented Jan 20, 2025

@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

@ToucheSir
Copy link
Member Author

There were two blockers to getting this merged:

  1. The TagBot and Documenter configuration here had completely bitrotted. I had to spend a good amount of time updating those to work, otherwise any new releases would be incomplete.
  2. There's a mysterious downstream failure with DiffEqFlux that didn't happen before this PR. I have no idea why, and if re-running CI doesn't fix it then I need help figuring out the root cause. Otherwise we'll end up merging a bugfix release which creates different bugs.

@juliohm
Copy link

juliohm commented Jan 20, 2025

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.

@ToucheSir
Copy link
Member Author

So undoing these changes could be the safest hotfix.

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.

@ToucheSir ToucheSir merged commit 8c18f06 into master Jan 20, 2025
13 of 15 checks passed
@ToucheSir ToucheSir deleted the bc/unthunk-hotfix branch January 20, 2025 23:06
@juliohm
Copy link

juliohm commented Jan 20, 2025

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.

@ToucheSir
Copy link
Member Author

Can you wait until after JuliaRegistries/General#123376 has been merged (+ some time for the usual package server propagation) and try?

@juliohm
Copy link

juliohm commented Jan 20, 2025

Sorry, I thought that the latest release from 3 hours ago already included the fix. Will wait and retry. Thanks.

@ToucheSir
Copy link
Member Author

ToucheSir commented Jan 21, 2025

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 ] adding CoordRefSystems and it works locally for me without errors, so hopefully this has resolved that issue.

@juliohm
Copy link

juliohm commented Jan 21, 2025

Thank you! I confirm the issue has been resolved in various downstream packages.

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.

Compile error on latest commit/version 0.2.6, incompatible with Zygote
2 participants