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

v0.13 deprecations #1751

Merged
merged 4 commits into from
Feb 5, 2022
Merged

v0.13 deprecations #1751

merged 4 commits into from
Feb 5, 2022

Conversation

CarloLucibello
Copy link
Member

@CarloLucibello CarloLucibello commented Oct 17, 2021

  • Removing datasets (have been deprecated for a year or so)
  • unexporting flatten and params
  • fixes a doctest issue failing doc tests #1739
  • removed deprecated getproperty for recurrent layers.
  • deprecated weight keyword argument in Conv constructor

I guess the only controversial point is if we want to unexport params. That could cause some (maybe a lot) of breakage, although we have been using it mostly in a qualified way in docs and model-zoo examples. On one hand, I wish it was never exported (it's a generic and common name, and it clashes with the exported Distributions.params). On the other hand, I'm not sure it is worth causing breakage.

Replaces #1735, Fix #1739, fix #629, fix #1779

@CarloLucibello CarloLucibello added this to the v0.13 milestone Oct 17, 2021
@mcabbott
Copy link
Member

There's no way to make a 12.X release with a depwarn for Flux.params, right?

@mcabbott
Copy link
Member

We could remove deprecations for Dense's getproperty, ref FluxML/Zygote.jl#1094 (comment) . And constructors too.

@CarloLucibello
Copy link
Member Author

We could remove deprecations for Dense's getproperty, ref FluxML/Zygote.jl#1094 (comment) . And constructors too.

removed all v0.12 deprecations introduced more than 6 months ago

@CarloLucibello
Copy link
Member Author

There's no way to make a 12.X release with a depwarn for Flux.params, right?

not that I know

@CarloLucibello
Copy link
Member Author

@mcabbott there is also this

# This can be made an error in Flux v0.13, for now just a warning

We have to decide if we want to make it an error already. It's been there 4 months, maybe we can wait the next release.

@mcabbott
Copy link
Member

mcabbott commented Nov 30, 2021

Could go either way on that. As a middle path, can upgrade @warn to @error to make it louder?

Other things from a quick look today...

@ToucheSir
Copy link
Member

ToucheSir commented Nov 30, 2021

No. 1 [Edit: surely 2, Juno] is already covered by #1643, just needs a quick bump to resolve VCS conflicts.

@mkschleg
Copy link
Contributor

I didn’t see this in the list, but I think we wanted to remove the getproperty for recur as well for v0.13.

@CarloLucibello CarloLucibello force-pushed the cl/v013 branch 2 times, most recently from 99136d9 to 2133fd5 Compare December 13, 2021 18:57
@CarloLucibello
Copy link
Member Author

Implemented two of the @mcabbott's suggestions

  • Remove Juno.jl dependency Remove Juno.jl dependency #1779 -- if making a breaking release, could we just kill this, and add back other progress bars later, if someone is inspired?
  • Some layer constructors have accumulated redundant methods. Letting Conv((2,-99), 3=>4; weight=rand(2,3,4,5)) work without error doesn't sound like a good design, but removing it is breaking.

@mcabbott you want to add the other stuff to the v0.13 milestone?

PS I don't know why CI is being stupid complaining about the Manifest

Copy link
Member

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

For other changes:

I had a commit trying out fast_tanh on top of #1761, which we should merge. Some quite large forward speedups, modest with the gradient though, IIRC.

Parallel would also be good to sort out, haven't looked lately.

Edit: not on the list above, but

Also, Juno is loaded here: I pushed 2 commits here, hope you don't mind.

src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Show resolved Hide resolved
@CarloLucibello
Copy link
Member Author

ok I think this is good to go

@mcabbott
Copy link
Member

One possibly evil idea is just to remove destructure since it's a breaking release.

Then we can make a PR adding it back, which must pass reasonably quality tests, i.e. It must have clear semantics, described in the docs; it must have tests, covering just about everything, and some attempt to invent adversarial examples. If you must have the old one, don't upgrade, until the new one is ready.

@darsnack
Copy link
Member

Isn't destructure critical for SciML stuff? Seems like a large "don't upgrade" umbrella. Instead of removing it, if we want to skip addressing it on this release, then let's just agree that a fast v0.14 is fine too (like @ToucheSir suggested on Slack).

@mcabbott
Copy link
Member

mcabbott commented Jan 12, 2022

Well in my evil plan this desire to upgrade will inspire SciML types who need it to chip in to figuring out the solid PR adding this functionality -- to approximately ChainRules's standards. Instead of the thing being grandfathered in and intractable to work on without upsetting someone. But possibly this is too weird.

Edit: And perhaps it should move, too: FluxML/Functors.jl#5

Copy link
Member

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

Since #1859 is merged, perhaps master is now sort-of 0.13-dev, and we should start merging things.

@ToucheSir
Copy link
Member

Another PR I'd like to submit for consideration is #1778. Making that non-breaking is going to be more of a pain than the alternative.

@CarloLucibello CarloLucibello force-pushed the cl/v013 branch 2 times, most recently from cd6e13d to 8310daf Compare February 5, 2022 19:26
src/Flux.jl Outdated Show resolved Hide resolved
@CarloLucibello CarloLucibello force-pushed the cl/v013 branch 2 times, most recently from 8cca3e6 to 5395db9 Compare February 5, 2022 19:27
@codecov-commenter
Copy link

Codecov Report

Merging #1751 (019f832) into master (9b21e2c) will increase coverage by 9.97%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1751      +/-   ##
==========================================
+ Coverage   74.50%   84.47%   +9.97%     
==========================================
  Files          28       21       -7     
  Lines        1706     1475     -231     
==========================================
- Hits         1271     1246      -25     
+ Misses        435      229     -206     
Impacted Files Coverage Δ
src/deprecations.jl 0.00% <ø> (-22.23%) ⬇️
src/functor.jl 86.66% <ø> (ø)
src/layers/recurrent.jl 85.54% <ø> (+9.86%) ⬆️
src/optimise/optimisers.jl 93.64% <ø> (ø)
src/layers/basic.jl 80.53% <100.00%> (+3.60%) ⬆️
src/layers/conv.jl 80.54% <100.00%> (+0.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b21e2c...019f832. Read the comment docs.

@CarloLucibello CarloLucibello merged commit a45cc05 into master Feb 5, 2022
@CarloLucibello CarloLucibello deleted the cl/v013 branch April 7, 2022 07:01
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.

Remove Juno.jl dependency failing doc tests Flux type piracy breaks REPL completions
6 participants