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

Make @adjoint unthunk pullback inputs #17

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

oschulz
Copy link
Contributor

@oschulz oschulz commented May 8, 2021

Required to support FluxML/Zygote.jl#966

Replaced my internal macro _adjoint_keepthunks in Zygote.
@oschulz
Copy link
Contributor Author

oschulz commented May 14, 2021

See FluxML/Zygote.jl#966 for discussion.

@oschulz oschulz changed the title Make @adjoint unthunk pullback inputs and add adjoint_keepthunks Make @adjoint unthunk pullback inputs May 16, 2021
@oschulz
Copy link
Contributor Author

oschulz commented May 16, 2021

Note: the keepthunks = false argument is intended for Zygote-internal use only.

@oschulz
Copy link
Contributor Author

oschulz commented May 17, 2021

@oxinabox and @DhairyaLGandhi - if you have a moment, could you check if we can move forward with this in the current form? We'll need a release with this before FluxML/Zygote.jl#966 can go through full CI. I would suggest that this can be released as as patch release, since it's non-breaking and doesn't introduce any new public features.

@oxinabox
Copy link
Member

I approve this.
But I won't merge til @DhairyaLGandhi says the plan in the other PR is going ahead

@oschulz
Copy link
Contributor Author

oschulz commented May 17, 2021

Sure, thanks!

@oschulz
Copy link
Contributor Author

oschulz commented May 22, 2021

@DhairyaLGandhi will you give us your blessing? :-)

@CarloLucibello
Copy link
Member

bump

@oschulz
Copy link
Contributor Author

oschulz commented Aug 7, 2021

@DhairyaLGandhi , @oxinabox , now that ChainRulesCore v1.0 is out, can we proceed with this, so that we can merge FluxML/Zygote.jl#966 ?

Copy link
Member

@DhairyaLGandhi DhairyaLGandhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we have since wanted to move to not unthunk quite so aggressively. Let me revisit FluxML/Zygote.jl#966 too!

src/adjoint.jl Show resolved Hide resolved
@oschulz
Copy link
Contributor Author

oschulz commented Aug 16, 2021

Sorry to be a pest - good to merge now, @DhairyaLGandhi ?

@DhairyaLGandhi
Copy link
Member

I think this is alright for now. I do wonder about some of the longer term effects of 966 (we should discuss those there), but seeing what implicit changes happen with generating gradients and the behaviour of rrules in general for a fairly spread out system. I have wanted to run some benchmarks with some more dP kinds of tools like Flux3D, ChemistryFeaturization and all too, which is what I've also been up to over the past couple weeks :)

@oschulz
Copy link
Contributor Author

oschulz commented Aug 16, 2021

I do wonder about some of the longer term effects of 966 (we should discuss those there) [...] run some benchmarks

Oh sure! Can we have a release of ZygoteRules with this PR first though (it's non-breaking), so that we can run full CI on FluxML/Zygote.jl#966?

@oschulz
Copy link
Contributor Author

oschulz commented Aug 29, 2021

Gente bump regarding merge, @DhairyaLGandhi

@oschulz
Copy link
Contributor Author

oschulz commented Sep 21, 2021

I don't mean to nag @DhairyaLGandhi, but it's been a while. Can we move forward with this?

@DhairyaLGandhi
Copy link
Member

I think I'm fine with merging this but be careful about 966, this has been on my radar :)

@oxinabox
Copy link
Member

@DhairyaLGandhi I don't think @oschulz has merge rights. So if you are fine with this then you will have to merge it?

@oschulz
Copy link
Contributor Author

oschulz commented Sep 21, 2021

I don't think @oschulz has merge rights

No, I don't, you guys will have to merge and make a release, so we can run full CI on FluxML/Zygote.jl#966:

I think I'm fine with merging this but be careful about 966, this has been on my radar

@oxinabox
Copy link
Member

oxinabox commented Sep 21, 2021

It seems I don't have merge rights either.
Which is odd, I thought I had rights on the FluxML/AutoDiff team which had Zygote, Tracker and ZygoteRules.

@CarloLucibello
Copy link
Member

I don't have merge rights as well @DhairyaLGandhi

@DhairyaLGandhi
Copy link
Member

Ah missed this. Thanks all

@DhairyaLGandhi DhairyaLGandhi merged commit 37dc97a into FluxML:master Sep 22, 2021
@oschulz
Copy link
Contributor Author

oschulz commented Sep 23, 2021

Thanks @DhairyaLGandhi ! Could you release a ZygoteRules v0.2.2, so I can finish up FluxML/Zygote.jl#966 with full CI?

@oschulz
Copy link
Contributor Author

oschulz commented Oct 1, 2021

@DhairyaLGandhi , could you tag a new release of ZygoteRules (should be v0.2.2, I guess)? It would make it much easier to proceed with FluxML/Zygote.jl#966.

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