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 create_bias a public API? #2049

Closed
ToucheSir opened this issue Aug 25, 2022 · 4 comments · Fixed by #2081
Closed

Make create_bias a public API? #2049

ToucheSir opened this issue Aug 25, 2022 · 4 comments · Fixed by #2081

Comments

@ToucheSir
Copy link
Member

This was recently marked private in #1998, but looking at a recent downstream test run (my mistake for not enabling them on that PR) it appears that downstream libraries are relying on it. If we want to dissuade them from continuing to use it, perhaps we could push a hotfix with non-underscore create_bias marked deprecated? Otherwise we should just restore it and render the docstring, I think.

@Saransh-cpp
Copy link
Member

The tests are also failing due to the rng_from_array() -> default_rng_value() renaming -

config = small: Error During Test at /home/runner/work/Flux.jl/Flux.jl/downstream/test/convnets.jl:332
  Got exception outside of a @test
  MethodError: no method matching rng_from_array()
  Closest candidates are:
    rng_from_array(::CUDA.CuArray) at ~/work/Flux.jl/Flux.jl/src/utils.jl:47
    rng_from_array(::AbstractArray) at ~/work/Flux.jl/Flux.jl/src/utils.jl:46
  Stacktrace:
    [1] DropPath(p::Float64)
      @ Metalhead.Layers ~/work/Flux.jl/Flux.jl/downstream/src/layers/drop.jl:159
    [2] convnextblock(planes::Int64, drop_path_rate::Float64, layerscale_init::Float32)
      @ Metalhead ~/work/Flux.jl/Flux.jl/downstream/src/convnets/convnext.jl:14
    [3] (::Metalhead.var"#187#188"{Float32, Vector{Int64}, LinRange{Float64, Int64}, Int64})(j::Int64)
      @ Metalhead ./none:0
    [4] iterate
      @ ./generator.jl:47 [inlined]
    [5] collect
      @ ./array.jl:787 [inlined]
    [6] convnext(depths::Vector{Int64}, planes::Vector{Int64}; drop_path_rate::Float64, layerscale_init::Float32, inchannels::Int64, nclasses::Int64)
      @ Metalhead ~/work/Flux.jl/Flux.jl/downstream/src/convnets/convnext.jl:60
    [7] #ConvNeXt#191
      @ ~/work/Flux.jl/Flux.jl/downstream/src/convnets/convnext.jl:99 [inlined]
    [8] ConvNeXt
      @ ~/work/Flux.jl/Flux.jl/downstream/src/convnets/convnext.jl:97 [inlined]
    [9] macro expansion
      @ ~/work/Flux.jl/Flux.jl/downstream/test/convnets.jl:333 [inlined]
   [10] macro expansion
      @ /opt/hostedtoolcache/julia/1.8.0/x64/share/julia/stdlib/v1.8/Test/src/Test.jl:1433 [inlined]
   [11] macro expansion
      @ ~/work/Flux.jl/Flux.jl/downstream/test/convnets.jl:332 [inlined]
   [12] macro expansion
      @ /opt/hostedtoolcache/julia/1.8.0/x64/share/julia/stdlib/v1.8/Test/src/Test.jl:1357 [inlined]
   [13] top-level scope
      @ ~/work/Flux.jl/Flux.jl/downstream/test/convnets.jl:332
   [14] include(fname::String)
      @ Base.MainInclude ./client.jl:476
   [15] macro expansion
      @ ~/work/Flux.jl/Flux.jl/downstream/test/runtests.jl:62 [inlined]
   [16] macro expansion
      @ /opt/hostedtoolcache/julia/1.8.0/x64/share/julia/stdlib/v1.8/Test/src/Test.jl:1357 [inlined]
   [17] top-level scope
      @ ~/work/Flux.jl/Flux.jl/downstream/test/runtests.jl:62
   [18] include(fname::String)
      @ Base.MainInclude ./client.jl:476
   [19] top-level scope
      @ none:6
   [20] eval
      @ ./boot.jl:368 [inlined]
   [21] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:276
   [22] _start()
      @ Base ./client.jl:522

I think rng_from_array() should be marked as deprecated. For create_bias, I think reverting it and adding it to the manual should be preferred?

@theabhirath
Copy link
Member

theabhirath commented Aug 25, 2022

Is the default_rng_value() function in a release yet? I can change the Metalhead API and bump compat if it is

@Saransh-cpp
Copy link
Member

Saransh-cpp commented Aug 25, 2022

default_rng_value() was added in 4773a69, which is not a part of any release. These tests must be failing on the master branch.

Edit: More generally, should the downstream tests run even on the internal API changes (or the ambiguous API changes, used internally but marked as public)?

@ToucheSir
Copy link
Member Author

There's no rule for when downstream tests should run. Some libraries run them unconditionally on every PR, but I think historically they haven't for Flux because of how long some take.

internal API changes (or the ambiguous API changes, used internally but marked as public)?

This is kind of none of the above. There is no real "marked as public" outside of exporting, so downstream libraries were just using a function with a docstring that seemed convenient. Depending on who you ask, the presence/absence of a docstring, leading underscore or even existence of a function are all markers of something being part of the public API...

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

Successfully merging a pull request may close this issue.

3 participants