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

remove Manifest #1725

Merged
merged 2 commits into from
Oct 13, 2021
Merged

remove Manifest #1725

merged 2 commits into from
Oct 13, 2021

Conversation

CarloLucibello
Copy link
Member

hundredth attempt to remove the manifest

@DhairyaLGandhi
Copy link
Member

DhairyaLGandhi commented Sep 29, 2021

As with previous cases, it is not something users would be affected by and we can test out different cogs in the ecosystem reliably before we release them.

We could have avoided many issues and releases had tests been performed with the relevant changes at different sites before registering.

@CarloLucibello
Copy link
Member Author

it affects us, we are not aware when tests are not passing with up-to-date packages as you can see

@DhairyaLGandhi
Copy link
Member

The issue isn't in the Flux repo though, it's elsewhere. And the issue would have been caught had we tested before releases

@CarloLucibello
Copy link
Member Author

The issue isn't in the Flux repo though, it's elsewhere

and we want to be aware of a bad interaction with Flux sooner or later?

And the issue would have been caught had we tested before releases

Zygote and ChainRules release frequently, and that's a good thing, we should do the same here. Add more downstream testing automation if needed. Not an argument for keeping the manifest in any case.

@CarloLucibello
Copy link
Member Author

I filed FluxML/Zygote.jl#1086 for the huber_loss failing test. Commenting out that test, I encounter another error (I don't have time to reduce it):

BPTT-1D: Error During Test at /home/carlo/.julia/dev/Flux/test/layers/recurrent.jl:2
  Got exception outside of a @test
  MethodError: no method matching reshape(::ChainRulesCore.NoTangent, ::Tuple{Int64, Int64})
  Closest candidates are:
    reshape(::FillArrays.AbstractFill, ::Tuple{Vararg{Int64, N}} where N) at /home/carlo/.julia/packages/FillArrays/DA7rk/src/FillArrays.jl:235
    reshape(::FillArrays.AbstractFill, ::Tuple{Vararg{Union{Colon, Int64}, N} where N}) at /home/carlo/.julia/packages/FillArrays/DA7rk/src/FillArrays.jl:231
    reshape(::FillArrays.AbstractFill, ::Tuple{Integer, Vararg{Integer, N} where N}) at /home/carlo/.julia/packages/FillArrays/DA7rk/src/FillArrays.jl:236
    ...
  Stacktrace:
    [1] (::Zygote.var"#494#496"{Matrix{Float32}, Tuple{Colon}})(Δ::ChainRulesCore.NoTangent)
      @ Zygote ~/.julia/packages/Zygote/EPhp6/src/lib/array.jl:115
    [2] (::Zygote.var"#2480#back#498"{Zygote.var"#494#496"{Matrix{Float32}, Tuple{Colon}}})(Δ::ChainRulesCore.NoTangent)
      @ Zygote ~/.julia/packages/ZygoteRules/OjfTt/src/adjoint.jl:59
    [3] (::Zygote.var"#215#216"{Tuple{Tuple{Nothing, Nothing}, Tuple{}}, Zygote.var"#2480#back#498"{Zygote.var"#494#496"{Matrix{Float32}, Tuple{Colon}}}})(Δ::ChainRulesCore.NoTangent)
      @ Zygote ~/.julia/packages/Zygote/EPhp6/src/lib/lib.jl:203
    [4] (::Zygote.var"#1754#back#217"{Zygote.var"#215#216"{Tuple{Tuple{Nothing, Nothing}, Tuple{}}, Zygote.var"#2480#back#498"{Zygote.var"#494#496"{Matrix{Float32}, Tuple{Colon}}}}})(Δ::ChainRulesCore.NoTangent)
      @ Zygote ~/.julia/packages/ZygoteRules/OjfTt/src/adjoint.jl:59
    [5] Pullback
      @ ~/.julia/dev/Flux/src/layers/recurrent.jl:107 [inlined]
    [6] (::typeof((λ)))(Δ::Tuple{Matrix{Float32}, ChainRulesCore.NoTangent})
      @ Zygote ~/.julia/packages/Zygote/EPhp6/src/compiler/interface2.jl:0
    [7] Pullback
      @ ~/.julia/dev/Flux/src/layers/recurrent.jl:47 [inlined]
    [8] (::typeof((λ)))(Δ::ChainRulesCore.NoTangent)
      @ Zygote ~/.julia/packages/Zygote/EPhp6/src/compiler/interface2.jl:0
    [9] #572
      @ ~/.julia/packages/Zygote/EPhp6/src/lib/array.jl:218 [inlined]
   [10] (::Base.var"#4#5"{Zygote.var"#572#578"})(a::Tuple{Tuple{Vector{Float32}, typeof(∂(λ))}, ChainRulesCore.NoTangent})
      @ Base ./generator.jl:36
   [11] iterate
      @ ./generator.jl:47 [inlined]
   [12] collect_to!(dest::Vector{Tuple{Base.RefValue{Any}, Vector{Float32}}}, itr::Base.Generator{Base.Iterators.Zip{Tuple{Vector{Tuple{Vector{Float32}, typeof((λ))}}, Vector{Any}}}, Base.var"#4#5"{Zygote.var"#572#578"}}, offs::Int64, st::Tuple{Int64, Int64})
      @ Base ./array.jl:724
   [13] collect_to_with_first!(dest::Vector{Tuple{Base.RefValue{Any}, Vector{Float32}}}, v1::Tuple{Base.RefValue{Any}, Vector{Float32}}, itr::Base.Generator{Base.Iterators.Zip{Tuple{Vector{Tuple{Vector{Float32}, typeof((λ))}}, Vector{Any}}}, Base.var"#4#5"{Zygote.var"#572#578"}}, st::Tuple{Int64, Int64})
      @ Base ./array.jl:702
   [14] collect(itr::Base.Generator{Base.Iterators.Zip{Tuple{Vector{Tuple{Vector{Float32}, typeof((λ))}}, Vector{Any}}}, Base.var"#4#5"{Zygote.var"#572#578"}})
      @ Base ./array.jl:683
   [15] map
      @ ./abstractarray.jl:2383 [inlined]
   [16] (::Zygote.var"#569#575"{Flux.Recur{Flux.RNNCell{typeof(tanh), Matrix{Float32}, Vector{Float32}, Matrix{Float32}}, Matrix{Float32}}, Tuple{Vector{Vector{Float32}}}, Vector{Tuple{Vector{Float32}, typeof((λ))}}})(Δ::Vector{Any})
      @ Zygote ~/.julia/packages/Zygote/EPhp6/src/lib/array.jl:218
   [17] (::Flux.var"#477#back#243"{Zygote.var"#569#575"{Flux.Recur{Flux.RNNCell{typeof(tanh), Matrix{Float32}, Vector{Float32}, Matrix{Float32}}, Matrix{Float32}}, Tuple{Vector{Vector{Float32}}}, Vector{Tuple{Vector{Float32}, typeof((λ))}}}})(Δ::Vector{Any})
      @ Flux ~/.julia/packages/ZygoteRules/OjfTt/src/adjoint.jl:59
   [18] Pullback
      @ ~/.julia/dev/Flux/test/layers/recurrent.jl:8 [inlined]
   [19] (::typeof((λ)))(Δ::Float32)
      @ Zygote ~/.julia/packages/Zygote/EPhp6/src/compiler/interface2.jl:0
   [20] (::Zygote.var"#96#97"{Params, typeof((λ)), Zygote.Context})(Δ::Float32)
      @ Zygote ~/.julia/packages/Zygote/EPhp6/src/compiler/interface.jl:357
   [21] gradient(f::Function, args::Params)
      @ Zygote ~/.julia/packages/Zygote/EPhp6/src/compiler/interface.jl:76
   [22] macro expansion
      @ ~/.julia/dev/Flux/test/layers/recurrent.jl:7 [inlined]
   [23] macro expansion
      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1151 [inlined]
   [24] top-level scope
      @ ~/.julia/dev/Flux/test/layers/recurrent.jl:3
   [25] include(fname::String)
      @ Base.MainInclude ./client.jl:444
   [26] macro expansion
      @ ~/.julia/dev/Flux/test/runtests.jl:37 [inlined]
   [27] macro expansion
      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1151 [inlined]
   [28] top-level scope
      @ ~/.julia/dev/Flux/test/runtests.jl:34
   [29] include(fname::String)
      @ Base.MainInclude ./client.jl:444
   [30] top-level scope
      @ none:6
   [31] eval
      @ ./boot.jl:360 [inlined]
   [32] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:261
   [33] _start()
      @ Base ./client.jl:485

Pinging ChainRules people for some insight @mcabbott @oxinabox @willtebbutt

@DhairyaLGandhi
Copy link
Member

Might be a side effect of projection.

@oxinabox
Copy link
Member

This looks like ChainRules types escaping into Zygote. Which is a problem.
I don't think it is a side-effect of the projection step itself.
But I think it was a bug introduced in the PR that was using more projection. (FluxML/Zygote.jl#1044)
Just because it showed up around that time somewhere else.

@darsnack
Copy link
Member

darsnack commented Oct 4, 2021

I think having a Manifest.toml actually prevents us from testing our interaction with the rest of the ecosystem well. Ultimately, fixing packages in a way that is out of sync with what users might experience just means that we are always testing the wrong thing.

@CarloLucibello
Copy link
Member Author

If anyone can help with the error in #1725 (comment) it would be highly appreciated. We have a severe regression in recurrent layers and broken CI

@mcabbott
Copy link
Member

I don't know what causes this, tests seem to pass if you add

Base.reshape(z::ChainRulesCore.AbstractZero, ::Tuple) = z

which is something CRC plausibly ought to do, right?

@mcabbott mcabbott closed this Oct 12, 2021
@mcabbott mcabbott reopened this Oct 12, 2021
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.

Tests pass with ChainRulesCore v1.8.0, so this should be fine now.

@CarloLucibello
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Oct 12, 2021
1725: remove Manifest r=CarloLucibello a=CarloLucibello

hundredth attempt to remove the manifest


Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
@mcabbott
Copy link
Member

Strange that buildkite picks ChainRulesCore v1.7.2, hence I think Bors will fail here. Github CI seems to find the latest.

@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

Build failed:

@mcabbott
Copy link
Member

mcabbott commented Oct 12, 2021

bors r+

Still gets 1.7.x

bors bot added a commit that referenced this pull request Oct 12, 2021
1725: remove Manifest r=mcabbott a=CarloLucibello

hundredth attempt to remove the manifest


Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

Build failed:

@CarloLucibello
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 13, 2021

Build succeeded:

@bors bors bot merged commit fd6b423 into master Oct 13, 2021
@CarloLucibello CarloLucibello deleted the cl/rmmanifest 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.

5 participants