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

Adjust docs & Flux.@functor for Functors.jl v0.5, plus misc. depwarns #2509

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

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Oct 28, 2024

After FluxML/Functors.jl#51 we should stop saying that you need either @layer or @functor for Flux to find your parameters.

This PR:

  • defines a new macro Flux.@functor which prints a depwarn & then calls @layer
  • removes the code from @layer which printed other depwarns
  • removes the code from @layer which defines functor

Fixes #2533

WIP, more doc changes needed.

PR Checklist

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

@mcabbott mcabbott added this to the v0.15 milestone Oct 28, 2024
@CarloLucibello
Copy link
Member

Functors v0.5 is out

@CarloLucibello
Copy link
Member

We can continue with this now that Functors v0.5 is in with #2525
This is the last item in the v0.15 milestone

@CarloLucibello
Copy link
Member

I subsumed most of this into #2528 in order to speed up the 0.15 release.
I think only the Flux.@functor macro for smoothing the transition to @layer is left to be implemented.

@CarloLucibello
Copy link
Member

I think we should not implement a Flux.@functor macro. It could cause some confusion, burden us with extra code, and just add pretty printing functionality

@mcabbott
Copy link
Member Author

It's not so much to add functionality, as to avoid this:

julia> Flux.@functor 1
ERROR: LoadError: UndefVarError: `@functor` not defined

IMO that's an extremely unfriendly upgrade path. It would be OK to just print an error message, but doing what you ask as well is pretty easy.

@CarloLucibello
Copy link
Member

Ah no, that shouldn't happen, we should have using Functors: @functor in Flux so that we don't have an error

@mcabbott
Copy link
Member Author

Yea, but better to prod people to update. Hence this PR.

and the explicit `gradient(m -> loss(m, x, y), model)` for gradient computation.
""", :params)
@warn """`Flux.params(m...)` is deprecated. Use `Flux.trainable(model)` for parameter collection,
and the explicit `gradient(m -> loss(m, x, y), model)` for gradient computation.""" maxlog=1
Copy link
Member Author

Choose a reason for hiding this comment

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

Base.depwarn is silent except in tests. IMO that's ideal if you are marking something deprecated before a breaking change, when the replacement is available.

However, my impression is that we really want people to change this, not to silently live with old code during 0.15. So I'd like something to be printed in interactive use.

Maybe the same goes for more of the depwarns in this file?

Comment on lines -122 to -124
# Enable these when 0.16 is released, and delete const ClipGrad = Optimise.ClipValue etc:
# Base.@deprecate_binding Optimiser OptimiserChain
# Base.@deprecate_binding ClipValue ClipGrad
Copy link
Member Author

Choose a reason for hiding this comment

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

These const definitions have already been deleted:

julia> Flux.ClipValue
ERROR: UndefVarError: `ClipValue` not defined
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base.jl:31
 [2] top-level scope
   @ REPL[6]:1

julia> Flux.Optimiser
ERROR: UndefVarError: `Optimiser` not defined
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base.jl:31
 [2] top-level scope
   @ REPL[7]:1

@mcabbott mcabbott changed the title Adjust docs & @layer for Functors.jl v0.5 Adjust docs & Flux.@functor for Functors.jl v0.5, plus misc. depwarns Nov 25, 2024
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 61.38%. Comparing base (e2b3f06) to head (130e04c).

Files with missing lines Patch % Lines
src/deprecations.jl 0.00% 7 Missing ⚠️
src/optimise/train.jl 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2509       +/-   ##
===========================================
+ Coverage   33.54%   61.38%   +27.84%     
===========================================
  Files          31       31               
  Lines        1881     1919       +38     
===========================================
+ Hits          631     1178      +547     
+ Misses       1250      741      -509     

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


🚨 Try these New Features:

@CarloLucibello CarloLucibello marked this pull request as ready for review November 26, 2024 05:59
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.

Flux.params is broken
2 participants