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

Use Buffers.jl #645

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Use Buffers.jl #645

wants to merge 12 commits into from

Conversation

sethaxen
Copy link
Contributor

@sethaxen sethaxen commented May 14, 2020

This PR fixes FluxML/ZygoteRules.jl#6 by removing Buffer and necessary methods that use it and instead using Buffers.jl, which contains only the code removed in this PR, the corresponding tests, and the a few additional tests. This enables users to use Buffer in their methods/adjoints without depending on Zygote. This should not be a breaking change.

A few things to do, pending feedback on this approach from @MikeInnes and others:

@sethaxen
Copy link
Contributor Author

sethaxen commented May 14, 2020

I think test on 1.0 only fail because of the manifest (locally it passes).

@sethaxen
Copy link
Contributor Author

@MikeInnes is there anything missing or can we proceed as discussed (registering and moving to FluxML)?

@MikeInnes
Copy link
Member

Sounds good to me. Do I need to give you owner on FluxML so you can transfer it?

bors try

@bors
Copy link
Contributor

bors bot commented Jun 3, 2020

try

Merge conflict.

@DhairyaLGandhi
Copy link
Member

I think this needs a rebase

@sethaxen
Copy link
Contributor Author

Sounds good to me. Do I need to give you owner on FluxML so you can transfer it?

I think I just need permissions to create a public repo in FluxML (see here). Alternatively, it might work to give you full admin rights on Buffers.jl, and then you could make the transfer

@sethaxen
Copy link
Contributor Author

sethaxen commented Jul 7, 2020

@MikeInnes how would you like to proceed?

@ToucheSir
Copy link
Member

Seems like this fell through the cracks. @sethaxen would you still be interested in bringing Buffers.jl into the Flux org?

@sethaxen
Copy link
Contributor Author

@ToucheSir, I'm happy to transfer the repo to the FluxML org; but given how much time has passed and how little I use Zygote now, it would probably take someone more familiar with Zygote to 1) make sure any changes that have been made in the last 1.5 years to Zygote's Buffer implementations are reflected in Buffers.jl, and 2) finish this integration.

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.

Moving Buffer to ZygoteRules
4 participants