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

Add NewRecur for Blocked RNNs #2316

Closed
wants to merge 2 commits into from
Closed

Conversation

mkschleg
Copy link
Contributor

@mkschleg mkschleg commented Aug 24, 2023

This adds the NewRecur functionality from Fluxperimental.jl. There are some road bumps that need to be ironed out.

  1. The rrule for scan_full needs to be made type stable. I thought this had to do w/ the hobbits being a vector of Any, but when I separated out the first element and the other elements (this is where the issue is) I still see type instability when using Cthulu. I've included the new rrule in this pull request and below it is a commented out version of the Fluxperimental.jl one. I'm not sure where else to look for type instability as the stack to call this is pretty deep. I think at some point the type instability starts coming from Zygote.ZBacks in the stack. (I need help knowing how to iterate on this more).
  2. Naming scheme. Does it make sense to name this NewRecur, or should we try and make this the standard for v0.15 while deprecating the old recur in v0.14 and changing the name? Another idea is to follow Lux.jl and call this Recurrent.
  3. Tests. I have the tests I made in Fluxperimental for this, but maybe there are more we should do?

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable

@ToucheSir
Copy link
Member

@darsnack and I had a long discussion about this on last week's ML call, and came to the conclusion that it would really help to co-develop this with the apply functionality in FluxML/Fluxperimental.jl#7. Since the latter isn't fully implemented yet and the interplay between the two isn't fully established, it does make sense to keep prototyping and testing both in Fluxperimental. That would also let us kick tasks such as figuring out type stability further down the road.

Now, we also discussed a lot of ideas on what the eventual APIs should look like as a coherent whole. At some point this should be turned into a writeup, but since it's mostly nascent ideas we thought it might help to have a call to flesh out some of the details. Let us know on Zulip/Slack if you're interested and we can work out the logistics :)

@ToucheSir ToucheSir mentioned this pull request Feb 12, 2024
14 tasks
@CarloLucibello CarloLucibello added this to the v0.15 milestone Oct 14, 2024
@CarloLucibello
Copy link
Member

What is the status of this?

@mkschleg
Copy link
Contributor Author

I lost steam on working on Julia due to issues I was having w/ Autodiff/Zygote back in 2023, without any vision on when an alternative would be available. I couldn't really justify the time I was putting in anymore.

From what I remember, this was in a working state and likely dealt with the gradient issues from #2185.

No work was done on playing w/ apply with this interface. I expect it to be relatively straightforward though.

@CarloLucibello
Copy link
Member

I'm going to close since with the redesign in #2500 the state is handled explicitly and we don't have a Recur layer.
I'm sorry for all the issues we faced, we should have backed off from mutation years ago.

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 79.68750% with 13 lines in your changes missing coverage. Please review.

Project coverage is 7.33%. Comparing base (1348828) to head (d070150).
Report is 183 commits behind head on master.

Files with missing lines Patch % Lines
src/layers/recurrent.jl 79.68% 13 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (1348828) and HEAD (d070150). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (1348828) HEAD (d070150)
3 1
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #2316       +/-   ##
==========================================
- Coverage   79.47%   7.33%   -72.14%     
==========================================
  Files          31      31               
  Lines        1749    1799       +50     
==========================================
- Hits         1390     132     -1258     
- Misses        359    1667     +1308     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mkschleg
Copy link
Contributor Author

mkschleg commented Nov 5, 2024

I'm glad this is getting fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants